| Message ID | 20240215-wilc_fix_rcu_usage-v1-0-f610e46c6f82@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | wifi: wilc1000: fix RCU usage | expand |
Alexis Lothoré <alexis.lothore@bootlin.com> writes: > This small series aims to fix multiple warnings observed when enabling > CONFIG_PROVE_RCU_LIST: > - add missing locks to create corresponding critical read sections > - fix mix between RCU and SRCU API usage > > While at it, since SRCU API is already in use in the driver, any fix done > on RCU usage was also done with the SRCU variant of RCU API. I do not > really get why we are using SRCU in this driver instead of classic RCU, as > it seems to be done in any other wireless driver. And even more so, no other driver in drivers/net use SRCU. > My understanding is that primary SRCU use case is for compatibility > with realtime kernel, which needs to be preemptible everywhere. Has > the driver been really developped with this constraint in mind ? If > you have more details about this, feel free to educate me. Alexis, if you have the time I recommend submitting a patchset converting wilc1000 to use classic RCU. At least I have a hard time understanding why SRCU is needed, especially after seeing the warning you found.
On 2/19/24 17:19, Kalle Valo wrote: > Alexis Lothoré <alexis.lothore@bootlin.com> writes: > >> This small series aims to fix multiple warnings observed when enabling >> CONFIG_PROVE_RCU_LIST: >> - add missing locks to create corresponding critical read sections >> - fix mix between RCU and SRCU API usage >> >> While at it, since SRCU API is already in use in the driver, any fix done >> on RCU usage was also done with the SRCU variant of RCU API. I do not >> really get why we are using SRCU in this driver instead of classic RCU, as >> it seems to be done in any other wireless driver. > > And even more so, no other driver in drivers/net use SRCU. > >> My understanding is that primary SRCU use case is for compatibility >> with realtime kernel, which needs to be preemptible everywhere. Has >> the driver been really developped with this constraint in mind ? If >> you have more details about this, feel free to educate me. > > Alexis, if you have the time I recommend submitting a patchset > converting wilc1000 to use classic RCU. At least I have a hard time > understanding why SRCU is needed, especially after seeing the warning > you found. If nobody else comes in with a strong argument in favor of keeping SRCU, yes I can certainly add that to my backlog :)
Alexis Lothoré <alexis.lothore@bootlin.com> writes: > On 2/19/24 17:19, Kalle Valo wrote: > >> Alexis Lothoré <alexis.lothore@bootlin.com> writes: >> >>> This small series aims to fix multiple warnings observed when enabling >>> CONFIG_PROVE_RCU_LIST: >>> - add missing locks to create corresponding critical read sections >>> - fix mix between RCU and SRCU API usage >>> >>> While at it, since SRCU API is already in use in the driver, any fix done >>> on RCU usage was also done with the SRCU variant of RCU API. I do not >>> really get why we are using SRCU in this driver instead of classic RCU, as >>> it seems to be done in any other wireless driver. >> >> And even more so, no other driver in drivers/net use SRCU. >> >>> My understanding is that primary SRCU use case is for compatibility >>> with realtime kernel, which needs to be preemptible everywhere. Has >>> the driver been really developped with this constraint in mind ? If >>> you have more details about this, feel free to educate me. >> >> Alexis, if you have the time I recommend submitting a patchset >> converting wilc1000 to use classic RCU. At least I have a hard time >> understanding why SRCU is needed, especially after seeing the warning >> you found. > > If nobody else comes in with a strong argument in favor of keeping > SRCU And emphasis on the word "strong"... > yes I can certainly add that to my backlog :) Thanks! Your wilc1000 backlog is getting long, I hope your todo software won't overload ;)
This small series aims to fix multiple warnings observed when enabling CONFIG_PROVE_RCU_LIST: - add missing locks to create corresponding critical read sections - fix mix between RCU and SRCU API usage While at it, since SRCU API is already in use in the driver, any fix done on RCU usage was also done with the SRCU variant of RCU API. I do not really get why we are using SRCU in this driver instead of classic RCU, as it seems to be done in any other wireless driver. My understanding is that primary SRCU use case is for compatibility with realtime kernel, which needs to be preemptible everywhere. Has the driver been really developped with this constraint in mind ? If you have more details about this, feel free to educate me. To: <linux-wireless@vger.kernel.org> Cc: Ajay Singh <ajay.kathat@microchip.com> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> Cc: Kalle Valo <kvalo@kernel.org> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Cc: <linux-kernel@vger.kernel.org> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> --- Ajay Singh (1): wifi: wilc1000: add missing read critical sections around vif list traversal Alexis Lothoré (3): wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper wifi: wilc1000: use SRCU instead of RCU for vif list traversal wifi: wilc1000: fix declarations ordering drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +- drivers/net/wireless/microchip/wilc1000/hif.c | 70 ++++++++++++---------- drivers/net/wireless/microchip/wilc1000/netdev.c | 51 +++++++++------- drivers/net/wireless/microchip/wilc1000/netdev.h | 6 ++ drivers/net/wireless/microchip/wilc1000/wlan.c | 2 +- 5 files changed, 75 insertions(+), 56 deletions(-) --- base-commit: f4adde5c2f875c491670bc19f6abae91ae364ed6 change-id: 20240131-wilc_fix_rcu_usage-e60ecdffee25 Best regards,