| Message ID | 20250114-wilc1000_modprobe-v1-1-ad19d46f0c07@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 208dea9107e80a33dfeb029bdb93cb53eccf005d |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: wilc1000: unregister wiphy only after netdev registration | expand |
Alexis Lothoré <alexis.lothore@bootlin.com> 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@bootlin.com> > --- > 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. So if it's ok for you, I would like to add the Fixes tag but I can't find commit 89a7616e1715 anywhere.
On 1/14/25 13:20, Kalle Valo wrote: > Alexis Lothoré <alexis.lothore@bootlin.com> 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@bootlin.com> >> --- >> 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. 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, Alexis
Alexis Lothoré <alexis.lothore@bootlin.com> writes: > On 1/14/25 13:20, Kalle Valo wrote: > >> Alexis Lothoré <alexis.lothore@bootlin.com> 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@bootlin.com> >>> --- >>> 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?
On 1/14/25 14:20, Kalle Valo wrote: > Alexis Lothoré <alexis.lothore@bootlin.com> writes: [...] >>>> @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). ACK >> 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? Ok for me, thanks ! Alexis
Alexis Lothoré <alexis.lothore@bootlin.com> wrote: > 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(). > > Fixes: 1be94490b6b8 ("wifi: wilc1000: unregister wiphy only if it has been registered") > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> Patch applied to wireless-next.git, thanks. 208dea9107e8 wifi: wilc1000: unregister wiphy only after netdev registration
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c index 3751e2ee1ca95918b589ba9be032984705fa822a..af970f9991110807ebd880681ad0e8aaf8a0b9bc 100644 --- a/drivers/net/wireless/microchip/wilc1000/sdio.c +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c @@ -225,8 +225,8 @@ static void wilc_sdio_remove(struct sdio_func *func) struct wilc *wilc = sdio_get_drvdata(func); struct wilc_sdio *sdio_priv = wilc->bus_data; - wiphy_unregister(wilc->wiphy); wilc_netdev_cleanup(wilc); + wiphy_unregister(wilc->wiphy); wiphy_free(wilc->wiphy); kfree(sdio_priv->cmd53_buf); kfree(sdio_priv); diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c index 31219fd0cfb3fc6df071b3e9be5af17087ccf583..5bcabb7decea0fc8d0065a240f4acefabca3346a 100644 --- a/drivers/net/wireless/microchip/wilc1000/spi.c +++ b/drivers/net/wireless/microchip/wilc1000/spi.c @@ -285,8 +285,8 @@ static void wilc_bus_remove(struct spi_device *spi) struct wilc *wilc = spi_get_drvdata(spi); struct wilc_spi *spi_priv = wilc->bus_data; - wiphy_unregister(wilc->wiphy); wilc_netdev_cleanup(wilc); + wiphy_unregister(wilc->wiphy); wiphy_free(wilc->wiphy); kfree(spi_priv); }
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@bootlin.com> --- 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) @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. --- drivers/net/wireless/microchip/wilc1000/sdio.c | 2 +- drivers/net/wireless/microchip/wilc1000/spi.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) --- base-commit: 062792ae63cf5fd141d7f6f12cb0108e9ba6f88e change-id: 20250114-wilc1000_modprobe-95d930b2a67e Best regards,