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: <[email protected]>
Cc: Ajay Singh <[email protected]>
Cc: Claudiu Beznea <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: <[email protected]>
Signed-off-by: Alexis Lothoré <[email protected]>
---
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,
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Fix reverse-christmas tree order in some functions before adding more
variables
Signed-off-by: Alexis Lothoré <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/hif.c | 16 ++++++++--------
drivers/net/wireless/microchip/wilc1000/netdev.c | 6 +++---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index f3800aa9e9f8..c42859a727c3 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -1567,11 +1567,11 @@ int wilc_deinit(struct wilc_vif *vif)
void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
{
- int result;
- struct host_if_msg *msg;
- int id;
struct host_if_drv *hif_drv;
+ struct host_if_msg *msg;
struct wilc_vif *vif;
+ int result;
+ int id;
id = get_unaligned_le32(&buffer[length - 4]);
vif = wilc_get_vif_from_idx(wilc, id);
@@ -1608,11 +1608,11 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
{
- int result;
- struct host_if_msg *msg;
- int id;
struct host_if_drv *hif_drv;
+ struct host_if_msg *msg;
struct wilc_vif *vif;
+ int result;
+ int id;
mutex_lock(&wilc->deinit_lock);
@@ -1654,10 +1654,10 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
{
- int result;
- int id;
struct host_if_drv *hif_drv;
struct wilc_vif *vif;
+ int result;
+ int id;
id = get_unaligned_le32(&buffer[length - 4]);
vif = wilc_get_vif_from_idx(wilc, id);
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 96f239adc078..092801d33915 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -814,12 +814,12 @@ static int wilc_mac_close(struct net_device *ndev)
void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
u32 pkt_offset)
{
- unsigned int frame_len = 0;
- int stats;
unsigned char *buff_to_send = NULL;
- struct sk_buff *skb;
struct net_device *wilc_netdev;
+ unsigned int frame_len = 0;
struct wilc_vif *vif;
+ struct sk_buff *skb;
+ int stats;
if (!wilc)
return;
--
2.43.0
On 2/19/24 17:19, Kalle Valo wrote:
> Alexis Lothoré <[email protected]> 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é, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com