Skip to content

brcmfmac: Fix AP mode#184

Closed
pblass wants to merge 572 commits intoAsahiLinux:asahi-wipfrom
pblass:asahi-wip
Closed

brcmfmac: Fix AP mode#184
pblass wants to merge 572 commits intoAsahiLinux:asahi-wipfrom
pblass:asahi-wip

Conversation

@pblass
Copy link

@pblass pblass commented Sep 3, 2023

Fix access point mode by bringing firmware into appropriate state before setting up the device.

hoshinolina and others added 30 commits July 20, 2023 11:33
Commit reference: 3dfc5eb

Signed-off-by: Asahi Lina <lina@asahilina.net>
This abstraction enables Rust drivers to walk Device Tree nodes and
query their properties.

Signed-off-by: Asahi Lina <lina@asahilina.net>
…ation

In order for modpost to work and correctly generate module aliases from
device ID tables, it needs those tables to exist as global symbols with
a specific name. Additionally, modpost checks the size of the symbol, so
it cannot contain trailing data.

To support this, split IdArrayIds out of IdArray. The former contains
just the IDs. Then split out the device table definition macro from the
macro that defines the device table for a given bus driver, and add
another macro to declare a device table as a module device table.
Drivers can now define their ID table once, and then specify that it
should be used for both the driver and the module:

// Generic OF Device ID table.
kernel::define_of_id_table! {ASAHI_ID_TABLE, &'static hw::HwConfig, [
    (of::DeviceId::Compatible(b"apple,agx-t8103"), Some(&hw::t8103::HWCONFIG)),
    (of::DeviceId::Compatible(b"apple,agx-t8112"), Some(&hw::t8112::HWCONFIG)),
    // ...
]}

/// Platform Driver implementation for `AsahiDriver`.
impl platform::Driver for AsahiDriver {
    /// Data associated with each hardware ID.
    type IdInfo = &'static hw::HwConfig;

    // Assign the above OF ID table to this driver.
    kernel::driver_of_id_table!(ASAHI_ID_TABLE);

    // ...
}

// Export the OF ID table as a module ID table, to make modpost/autoloading work.
kernel::module_of_id_table!(MOD_TABLE, ASAHI_ID_TABLE);

Signed-off-by: Asahi Lina <lina@asahilina.net>
This patch adds a logic similar to `devm_platform_ioremap_resource`
function adding:
  - `IoResource` enumerated type that groups the `IORESOURCE_*` macros.
  - `get_resource()` method that is a binding of `platform_get_resource`
  - `ioremap_resource` that is newly written method similar to
    `devm_platform_ioremap_resource`.

Lina: Removed `bit` dependency and rebased

Co-developed-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Maciej Falkowski <m.falkowski@samsung.com>
Allows drivers to configure the DMA masks for a device. Implemented
here, not in device, because it requires a mutable platform device
reference this way (device::Device is a safely clonable reference).

Signed-off-by: Asahi Lina <lina@asahilina.net>
Apple SoCs require non-posted mappings for MMIO, and this is
automatically handled by devm_ioremap_resource() and friends via a
resource flag. Implement the same logic in kernel::io_mem, so it can
work the same way.

Signed-off-by: Asahi Lina <lina@asahilina.net>
TODO: This isn't abstracted properly yet

Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Apple Silicon SoCs (M1, M2, etc.) have a GPU with an ARM64 firmware
coprocessor. The firmware and the GPU share page tables in the standard
ARM64 format (the firmware literally sets the base as its TTBR0/1
registers). TTBR0 covers the low half of the address space and is
intended to be per-GPU-VM (GPU user mappings and kernel-managed
buffers), while TTBR1 covers the upper half and is global (firmware
code, data, management structures shared with the AP, and a few
GPU-accessible data structures).

In typical Apple fashion, the permissions are interpreted differently
from traditional ARM PTEs. By default, firmware mappings use Apple SPRR
permission remapping. The firmware only uses that for its own
code/data/MMIO mappings, and those pages are not accessible by the GPU
hardware. We never need to touch/manage these mappings, so this patch
does not support them.

When a specific bit is set in the PTEs, permissions switch to a
different scheme which supports various combinations of firmware/GPU
access. This is the mode intended to be used by AP GPU drivers, and what
we implement here.

The prot bits are interpreted as follows:

- IOMMU_READ and IOMMU_WRITE have the usual meaning.

- IOMMU_PRIV creates firmware-only mappings (no GPU access)
- IOMMU_NOEXEC creates GPU-only structures (no FW access)
- Otherwise structures are accessible by both GPU and FW

- IOMMU_MMIO creates Device mappings for firmware
- IOMMU_CACHE creates Normal-NC mappings for firmware (cache-coherent
  from the point of view of the AP, but slower)
- Otherwise creates Normal mappings for firmware (this requires manual
  cache management on the firmware side, as it is not coherent with the
  SoC fabric)

GPU-only mappings (textures/etc) are expected to use IOMMU_CACHE and are
seemingly coherent with the CPU (or otherwise the firmware/GPU already
issue the required cache management operations when correctly
configured).

There is a GPU-RO/FW-RW mode, but it is not currently implemented (it
doesn't seem to be very useful for the driver). There seems to be no
real noexec control (i.e. for shaders) on the GPU side. All of these
mappings are implicitly noexec for the firmware.

Drivers are expected to fully manage per-user (TTBR0) page tables, but
ownership of shared kernel (TTBR1) page tables is shared between the
firmware and the AP OS. We handle this by simply using a smaller IAS to
drop down one level of page tables, so the driver can install a PTE in
the top-level (firmware-initialized) page table directly and just add an
offset to the VAs passed into the io_pgtable code. This avoids having to
have any special handling for this here. The firmware-relevant data
structures are small, so we do not expect to ever require more VA space
than one top-level PTE covers (IAS=36 for the next level, 64 GiB).

Only 16K page mode is supported. The coprocessor MMU supports huge pages
as usual for ARM64, but the GPU MMU does not, so we do not enable them.

Signed-off-by: Asahi Lina <lina@asahilina.net>
This is ugly, we need a better way of expressing this.

Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
drm_sched_fini() currently leaves any pending jobs dangling, which
causes segfaults and other badness when job completion fences are
signaled after the scheduler is torn down.

Explicitly detach all jobs from their completion callbacks and free
them. This makes it possible to write a sensible safe abstraction for
drm_sched, without having to externally duplicate the tracking of
in-flight jobs.

This shouldn't regress any existing drivers, since calling
drm_sched_fini() with any pending jobs is broken and this change should
be a no-op if there are no pending jobs.

Signed-off-by: Asahi Lina <lina@asahilina.net>
A signaled scheduler fence can outlive its scheduler, since fences are
independencly reference counted. Therefore, we can't reference the
scheduler in the get_timeline_name() implementation.

Fixes oopses on `cat /sys/kernel/debug/dma_buf/bufinfo` when shared
dma-bufs reference fences from GPU schedulers that no longer exist.

Signed-off-by: Asahi Lina <lina@asahilina.net>
DRM drivers need to be able to declare which driver-specific ioctls they
support. This abstraction adds the required types and a helper macro to
generate the ioctl definition inside the DRM driver.

Note that this macro is not usable until further bits of the
abstraction are in place (but it will not fail to compile on its own, if
not called).

Signed-off-by: Asahi Lina <lina@asahilina.net>
Add the initial abstractions for DRM drivers and devices. These go
together in one commit since they are fairly tightly coupled types.

A few things have been stubbed out, to be implemented as further bits of
the DRM subsystem are introduced.

Signed-off-by: Asahi Lina <lina@asahilina.net>
A DRM File is the DRM counterpart to a kernel file structure,
representing an open DRM file descriptor. Add a Rust abstraction to
allow drivers to implement their own File types that implement the
DriverFile trait.

Signed-off-by: Asahi Lina <lina@asahilina.net>
Switch from being a refcount wrapper itself to a transparent wrapper
around `bindings::drm_device`. The refcounted type then becomes
ARef<Device<T>>.

Signed-off-by: Asahi Lina <lina@asahilina.net>
The DRM GEM subsystem is the DRM memory management subsystem used by
most modern drivers. Add a Rust abstraction to allow Rust DRM driver
implementations to use it.

Signed-off-by: Asahi Lina <lina@asahilina.net>
Switch from being a refcount wrapper itself to a transparent wrapper
around `bindings::drm_device`. The refcounted type then becomes
ARef<Device<T>>.

Signed-off-by: Asahi Lina <lina@asahilina.net>
This requires type_alias_impl_trait.

Signed-off-by: Asahi Lina <lina@asahilina.net>
There doesn't seem to be a way for the Rust bindings to get a
compile-time constant reference to drm_gem_shmem_vm_ops, so we need to
duplicate that structure in Rust... this isn't nice...

Signed-off-by: Asahi Lina <lina@asahilina.net>
The DRM shmem helper includes common code useful for drivers which
allocate GEM objects as anonymous shmem. Add a Rust abstraction for
this. Drivers can choose the raw GEM implementation or the shmem layer,
depending on their needs.

Signed-off-by: Asahi Lina <lina@asahilina.net>
@jannau
Copy link
Member

jannau commented Sep 10, 2023

This doesn't seem to work on the M1 Mac mini, PCI id 14e4:4425, subsystem: 106b:4378

[  952.837442] ieee80211 phy0: brcmf_fweh_call_event_handler: no interface object
[  952.923506] IPv6: ADDRCONF(NETDEV_CHANGE): wlp1s0f0: link becomes ready
[  984.462888] ieee80211 phy0: brcmf_vif_set_mgmt_ie: vndr ie set error : -52
[  984.464284] ieee80211 phy0: brcmf_vif_set_mgmt_ie: vndr ie set error : -52
[ 1018.289793] ieee80211 phy0: brcmf_fweh_call_event_handler: no interface object
[ 1018.357895] IPv6: ADDRCONF(NETDEV_CHANGE): wlp1s0f0: link becomes ready
[ 1050.787938] ieee80211 phy0: brcmf_vif_set_mgmt_ie: vndr ie set error : -52
[ 1050.789201] ieee80211 phy0: brcmf_vif_set_mgmt_ie: vndr ie set error : -52

That's the result of setting a shared wlan through KDE's NetworkManager setting dialog.

@jannau
Copy link
Member

jannau commented Sep 11, 2023

The errors seem unrelated. AP mode works on the M1 Mac Mini. BCM4378 and BCM4387 are the only 2 supported models on Apple silicon macs. The WiFi 6E variant BCM4388 on most models released 2023 is not working.

Any idea on which chipsets this is required? On the Cypress CYW43455 used on Raspberry Pi 4s it is apparently not needed.

@pblass
Copy link
Author

pblass commented Sep 15, 2023

Any idea on which chipsets this is required? On the Cypress CYW43455 used on Raspberry Pi 4s it is apparently not needed.

No, unfortunately not. This is based on bcmdhd-4359 which does not differentiate between 4378/4387/4388 chipsets for this as far as I can tell. It only checks if AP mode is configured and then triggers the role change. There are some weird exceptions for different chipsets though.

@marcan marcan force-pushed the asahi-wip branch 4 times, most recently from fea774d to b486fd3 Compare September 22, 2023 14:05
@marcan
Copy link

marcan commented Oct 5, 2023

FWIW, I tried emailing the Cypress people with questions (to avoid breaking their chips) and they ignored me, so as far as I'm concerned it's okay if we break them. If someone wants to step up to maintain support for CYW parts, they can do so, but it's not our job to care about them if the "official" maintainers don't.

Manually pulled this into the bits branch, should be in the next tag. Thanks!

@marcan marcan closed this Oct 5, 2023
chadmed pushed a commit to chadmed/linux that referenced this pull request Jan 5, 2025
commit 7cd1f5f upstream.

When executing mm selftests run_vmtests.sh, there is such an error:

 BUG: Bad page state in process uffd-unit-tests  pfn:00000
 page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x0
 flags: 0xffff0000002000(reserved|node=0|zone=0|lastcpupid=0xffff)
 raw: 00ffff0000002000 ffffbf0000000008 ffffbf0000000008 0000000000000000
 raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
 page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
 Modules linked in: snd_seq_dummy snd_seq snd_seq_device rfkill vfat fat
    virtio_balloon efi_pstore virtio_net pstore net_failover failover fuse
    nfnetlink virtio_scsi virtio_gpu virtio_dma_buf dm_multipath efivarfs
 CPU: 2 UID: 0 PID: 1913 Comm: uffd-unit-tests Not tainted 6.12.0 AsahiLinux#184
 Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 2/2/2022
 Stack : 900000047c8ac000 0000000000000000 9000000000223a7c 900000047c8ac000
         900000047c8af690 900000047c8af698 0000000000000000 900000047c8af7d8
         900000047c8af7d0 900000047c8af7d0 900000047c8af5b0 0000000000000001
         0000000000000001 900000047c8af698 10b3c7d53da40d26 0000010000000000
         0000000000000022 0000000fffffffff fffffffffe000000 ffff800000000000
         000000000000002f 0000800000000000 000000017a6d4000 90000000028f8940
         0000000000000000 0000000000000000 90000000025aa5e0 9000000002905000
         0000000000000000 90000000028f8940 ffff800000000000 0000000000000000
         0000000000000000 0000000000000000 9000000000223a94 000000012001839c
         00000000000000b0 0000000000000004 0000000000000000 0000000000071c1d
         ...
 Call Trace:
 [<9000000000223a94>] show_stack+0x5c/0x180
 [<9000000001c3fd64>] dump_stack_lvl+0x6c/0xa0
 [<900000000056aa08>] bad_page+0x1a0/0x1f0
 [<9000000000574978>] free_unref_folios+0xbf0/0xd20
 [<90000000004e65cc>] folios_put_refs+0x1a4/0x2b8
 [<9000000000599a0c>] free_pages_and_swap_cache+0x164/0x260
 [<9000000000547698>] tlb_batch_pages_flush+0xa8/0x1c0
 [<9000000000547f30>] tlb_finish_mmu+0xa8/0x218
 [<9000000000543cb8>] exit_mmap+0x1a0/0x360
 [<9000000000247658>] __mmput+0x78/0x200
 [<900000000025583c>] do_exit+0x43c/0xde8
 [<9000000000256490>] do_group_exit+0x68/0x110
 [<9000000000256554>] sys_exit_group+0x1c/0x20
 [<9000000001c413b4>] do_syscall+0x94/0x130
 [<90000000002216d8>] handle_syscall+0xb8/0x158
 Disabling lock debugging due to kernel taint
 BUG: non-zero pgtables_bytes on freeing mm: -16384

On LoongArch system, invalid huge pte entry should be invalid_pte_table
or a single _PAGE_HUGE bit rather than a zero value. And it should be
the same with invalid pmd entry, since pmd_none() is called by function
free_pgd_range() and pmd_none() return 0 by huge_pte_clear(). So single
_PAGE_HUGE bit is also treated as a valid pte table and free_pte_range()
will be called in free_pmd_range().

  free_pmd_range()
        pmd = pmd_offset(pud, addr);
        do {
                next = pmd_addr_end(addr, end);
                if (pmd_none_or_clear_bad(pmd))
                        continue;
                free_pte_range(tlb, pmd, addr);
        } while (pmd++, addr = next, addr != end);

Here invalid_pte_table is used for both invalid huge pte entry and
pmd entry.

Cc: stable@vger.kernel.org
Fixes: 09cfefb ("LoongArch: Add memory management")
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
asdfugil pushed a commit to HoolockLinux/linux that referenced this pull request Dec 4, 2025
xfs/286 produced this report on my test fleet:

 ==================================================================
 BUG: KFENCE: out-of-bounds read in memcpy_orig+0x54/0x110

 Out-of-bounds read at 0xffff88843fe9e038 (184B right of kfence-AsahiLinux#184):
  memcpy_orig+0x54/0x110
  xrep_symlink_salvage_inline+0xb3/0xf0 [xfs]
  xrep_symlink_salvage+0x100/0x110 [xfs]
  xrep_symlink+0x2e/0x80 [xfs]
  xrep_attempt+0x61/0x1f0 [xfs]
  xfs_scrub_metadata+0x34f/0x5c0 [xfs]
  xfs_ioc_scrubv_metadata+0x387/0x560 [xfs]
  xfs_file_ioctl+0xe23/0x10e0 [xfs]
  __x64_sys_ioctl+0x76/0xc0
  do_syscall_64+0x4e/0x1e0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 kfence-AsahiLinux#184: 0xffff88843fe9df80-0xffff88843fe9dfea, size=107, cache=kmalloc-128

 allocated by task 3470 on cpu 1 at 263329.131592s (192823.508886s ago):
  xfs_init_local_fork+0x79/0xe0 [xfs]
  xfs_iformat_local+0xa4/0x170 [xfs]
  xfs_iformat_data_fork+0x148/0x180 [xfs]
  xfs_inode_from_disk+0x2cd/0x480 [xfs]
  xfs_iget+0x450/0xd60 [xfs]
  xfs_bulkstat_one_int+0x6b/0x510 [xfs]
  xfs_bulkstat_iwalk+0x1e/0x30 [xfs]
  xfs_iwalk_ag_recs+0xdf/0x150 [xfs]
  xfs_iwalk_run_callbacks+0xb9/0x190 [xfs]
  xfs_iwalk_ag+0x1dc/0x2f0 [xfs]
  xfs_iwalk_args.constprop.0+0x6a/0x120 [xfs]
  xfs_iwalk+0xa4/0xd0 [xfs]
  xfs_bulkstat+0xfa/0x170 [xfs]
  xfs_ioc_fsbulkstat.isra.0+0x13a/0x230 [xfs]
  xfs_file_ioctl+0xbf2/0x10e0 [xfs]
  __x64_sys_ioctl+0x76/0xc0
  do_syscall_64+0x4e/0x1e0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 CPU: 1 UID: 0 PID: 1300113 Comm: xfs_scrub Not tainted 6.18.0-rc4-djwx #rc4 PREEMPT(lazy)  3d744dd94e92690f00a04398d2bd8631dcef1954
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-4.module+el8.8.0+21164+ed375313 04/01/2014
 ==================================================================

On further analysis, I realized that the second parameter to min() is
not correct.  xfs_ifork::if_bytes is the size of the xfs_ifork::if_data
buffer.  if_bytes can be smaller than the data fork size because:

(a) the forkoff code tries to keep the data area as large as possible
(b) for symbolic links, if_bytes is the ondisk file size + 1
(c) forkoff is always a multiple of 8.

Case in point: for a single-byte symlink target, forkoff will be
8 but the buffer will only be 2 bytes long.

In other words, the logic here is wrong and we walk off the end of the
incore buffer.  Fix that.

Cc: stable@vger.kernel.org # v6.10
Fixes: 2651923 ("xfs: online repair of symbolic links")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cem@kernel.org>
jannau pushed a commit that referenced this pull request Dec 7, 2025
[ Upstream commit 678e1cc ]

xfs/286 produced this report on my test fleet:

 ==================================================================
 BUG: KFENCE: out-of-bounds read in memcpy_orig+0x54/0x110

 Out-of-bounds read at 0xffff88843fe9e038 (184B right of kfence-#184):
  memcpy_orig+0x54/0x110
  xrep_symlink_salvage_inline+0xb3/0xf0 [xfs]
  xrep_symlink_salvage+0x100/0x110 [xfs]
  xrep_symlink+0x2e/0x80 [xfs]
  xrep_attempt+0x61/0x1f0 [xfs]
  xfs_scrub_metadata+0x34f/0x5c0 [xfs]
  xfs_ioc_scrubv_metadata+0x387/0x560 [xfs]
  xfs_file_ioctl+0xe23/0x10e0 [xfs]
  __x64_sys_ioctl+0x76/0xc0
  do_syscall_64+0x4e/0x1e0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 kfence-#184: 0xffff88843fe9df80-0xffff88843fe9dfea, size=107, cache=kmalloc-128

 allocated by task 3470 on cpu 1 at 263329.131592s (192823.508886s ago):
  xfs_init_local_fork+0x79/0xe0 [xfs]
  xfs_iformat_local+0xa4/0x170 [xfs]
  xfs_iformat_data_fork+0x148/0x180 [xfs]
  xfs_inode_from_disk+0x2cd/0x480 [xfs]
  xfs_iget+0x450/0xd60 [xfs]
  xfs_bulkstat_one_int+0x6b/0x510 [xfs]
  xfs_bulkstat_iwalk+0x1e/0x30 [xfs]
  xfs_iwalk_ag_recs+0xdf/0x150 [xfs]
  xfs_iwalk_run_callbacks+0xb9/0x190 [xfs]
  xfs_iwalk_ag+0x1dc/0x2f0 [xfs]
  xfs_iwalk_args.constprop.0+0x6a/0x120 [xfs]
  xfs_iwalk+0xa4/0xd0 [xfs]
  xfs_bulkstat+0xfa/0x170 [xfs]
  xfs_ioc_fsbulkstat.isra.0+0x13a/0x230 [xfs]
  xfs_file_ioctl+0xbf2/0x10e0 [xfs]
  __x64_sys_ioctl+0x76/0xc0
  do_syscall_64+0x4e/0x1e0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 CPU: 1 UID: 0 PID: 1300113 Comm: xfs_scrub Not tainted 6.18.0-rc4-djwx #rc4 PREEMPT(lazy)  3d744dd94e92690f00a04398d2bd8631dcef1954
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-4.module+el8.8.0+21164+ed375313 04/01/2014
 ==================================================================

On further analysis, I realized that the second parameter to min() is
not correct.  xfs_ifork::if_bytes is the size of the xfs_ifork::if_data
buffer.  if_bytes can be smaller than the data fork size because:

(a) the forkoff code tries to keep the data area as large as possible
(b) for symbolic links, if_bytes is the ondisk file size + 1
(c) forkoff is always a multiple of 8.

Case in point: for a single-byte symlink target, forkoff will be
8 but the buffer will only be 2 bytes long.

In other words, the logic here is wrong and we walk off the end of the
incore buffer.  Fix that.

Cc: stable@vger.kernel.org # v6.10
Fixes: 2651923 ("xfs: online repair of symbolic links")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Carlos Maiolino <cem@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comments