Alexis Lothoré <alexis.lothore@xxxxxxxxxxx> writes:
> On 1/14/25 13:20, Kalle Valo wrote:
>
>> Alexis Lothoré <alexis.lothore@xxxxxxxxxxx> writes:
>>
>>> wiphy_unregister/wiphy_free has been recently decoupled from
>>> wilc_netdev_cleanup to fix a faulty error path in sdio/spi probe
>>> functions. However this change introduced a new failure when simply
>>> loading then unloading the driver:
>>> $ modprobe wilc1000-sdio; modprobe -r wilc1000-sdio
>>> WARNING: CPU: 0 PID: 115 at net/wireless/core.c:1145 wiphy_unregister+0x904/0xc40 [cfg80211]
>>> Modules linked in: wilc1000_sdio(-) wilc1000 cfg80211 bluetooth ecdh_generic ecc
>>> CPU: 0 UID: 0 PID: 115 Comm: modprobe Not tainted 6.13.0-rc6+ #45
>>> Hardware name: Atmel SAMA5
>>> Call trace:
>>> unwind_backtrace from show_stack+0x18/0x1c
>>> show_stack from dump_stack_lvl+0x44/0x70
>>> dump_stack_lvl from __warn+0x118/0x27c
>>> __warn from warn_slowpath_fmt+0xcc/0x140
>>> warn_slowpath_fmt from wiphy_unregister+0x904/0xc40 [cfg80211]
>>> wiphy_unregister [cfg80211] from wilc_sdio_remove+0xb0/0x15c [wilc1000_sdio]
>>> wilc_sdio_remove [wilc1000_sdio] from sdio_bus_remove+0x104/0x3f0
>>> sdio_bus_remove from device_release_driver_internal+0x424/0x5dc
>>> device_release_driver_internal from driver_detach+0x120/0x224
>>> driver_detach from bus_remove_driver+0x17c/0x314
>>> bus_remove_driver from sys_delete_module+0x310/0x46c
>>> sys_delete_module from ret_fast_syscall+0x0/0x1c
>>> Exception stack(0xd0acbfa8 to 0xd0acbff0)
>>> bfa0: 0044b210 0044b210 0044b24c 00000800 00000000 00000000
>>> bfc0: 0044b210 0044b210 00000000 00000081 00000000 0044b210 00000000 00000000
>>> bfe0: 00448e24 b6af99c4 0043ea0d aea2e12c
>>> irq event stamp: 0
>>> hardirqs last enabled at (0): [<00000000>] 0x0
>>> hardirqs last disabled at (0): [<c01588f0>] copy_process+0x1c4c/0x7bec
>>> softirqs last enabled at (0): [<c0158944>] copy_process+0x1ca0/0x7bec
>>> softirqs last disabled at (0): [<00000000>] 0x0
>>>
>>> The warning is triggered by the fact that there is still a
>>> wireless_device linked to the wiphy we are unregistering, due to
>>> wiphy_unregister now being called after net device unregister (performed
>>> in wilc_netdev_cleanup). Fix this warning by moving wiphy_unregister
>>> after wilc_netdev_cleanup is nominal paths (ie: driver removal).
>>> wilc_netdev_cleanup ordering is left untouched in error paths in probe
>>> function because net device is not registered in those paths (so the
>>> warning can not trigger), yet the wiphy can still be registered, and we
>>> still some cleanup steps from wilc_netdev_cleanup.
>>>
>>> Signed-off-by: Alexis Lothoré <alexis.lothore@xxxxxxxxxxx>
>>> ---
>>> I clearly overlooked this simple scenario/misunderstood expected
>>> unregistration order when fixing some spi probe error path, my bad (see
>>> commit 89a7616e1715 ("ARM: dts: at91-sama5d27_wlsom1: update bluetooth
>>> chip description") in wireless-next)
>>
>> No worries, bugs are business as usual.
>>
>>> @Kalle: 89a7616e1715 (the faulty commit) is only in wireless-next for
>>> now IIUC, so I did not provide any Fixes: tag to prevent any faulty SHA1
>>> if those commits end up being picked in stable tree (however, the faulty
>>> commit _has_ a Fixes tag). Please let me know if we should proceed
>>> differently.
>>
>> Hmm, I don't really follow you here. I feel that always adding the Fixes
>> tag is the safest option, that way it's clear for everyone what commit
>> we are fixing.
>
> I was thinking about the fact that the faulty commit SHA1 may change because of
> a merge, and then break the Fixes: tag, but maybe I am overthinking.
Ah, now I understand. Actually commit id doesn't change during a merge
so we are safe in that regard. The commit id only changes if there's a
rebase in the tree and we don't rebase wireless trees (unless something
really drastic has happened).
> So if it's ok for you, I would like to add the Fixes tag
>> but I can't find commit 89a7616e1715 anywhere.
>
> Gaah, indeed that's not the correct SHA1. The faulty commit in wireless-next is
> in fact commit 1be94490b6b8 ("wifi: wilc1000: unregister wiphy only if it has
> been registered")
Thanks, so I'm planning to add this during commit:
Fixes: 1be94490b6b8 ("wifi: wilc1000: unregister wiphy only if it has been registered")
Is that ok?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
