From: "Alexis Lothoré" <alexis.lothore@bootlin.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: linux-wireless@vger.kernel.org, Kalle Valo <kvalo@kernel.org>,
"Ajay.Kathat" <Ajay.Kathat@microchip.com>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [bug report] wifi: wilc1000: convert list management to RCU
Date: Tue, 28 May 2024 11:31:16 +0200 [thread overview]
Message-ID: <0a9c592d-7db3-454e-a83f-901b1865b414@bootlin.com> (raw)
In-Reply-To: <3b46ec7c-baee-49fd-b760-3bc12fb12eaf@moroto.mountain>
Hello,
On 5/9/24 15:24, Dan Carpenter wrote:
> Hello Alexis Lothoré,
>
> Commit f236464f1db7 ("wifi: wilc1000: convert list management to
> RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
> static checker warning:
>
> drivers/net/wireless/microchip/wilc1000/mon.c:236 wilc_wfi_init_mon_interface()
> warn: sleeping in atomic context
My bad for the extensive delay to fix this. I have been in fact scratching my
head quite a lot around it. Adding Kalle and Ajay to keep them updated, and
possibly to get opinions.
This issue is indeed due to my recent series converting back SRCU to RCU in wilc
driver (submitted because at this time, there was no obvious reason nor
documentation about why SRCU was needed). By checking further the consequence, I
find in fact three issues, and I suspect those to be the original reason for
SRCU usage in the driver instead of classical RCU. All of them are reproducible
with runtime checkers enabled regarding RCU and sleeps in atomic sections,
either by triggering some heavy traffic for the first one, or raising an access
points for the two others:
- one issue is in wilc_wfi_init_mon_interface (the initial warning raised by
Dan). This one:
- parses the vif list (under RCU) to perform some checks, possibly canceling
the interface registration
- then registers the monitoring interface (has sleeping calls, especially a
register_netdevice call)
- then if registration is successful, updates appropriate vif to flag it as
monitoring interface (then leave RCU section)
I have a hotfix for this, but not a very satisfying one, which consists in
splitting the RCU section into two (first and third points), but additionally
using the vif list lock to make sure vif list has not been altered in-between.
IMHO this kind of fix just make things worse in the driver, just to tame RCU.
- one issue is in wilc_wlan_txq_filter_dup_tcp_ack (the second warning raised
by Dan), which calls wait_for_completion_timeout while being in RCU critical
read. The issue can be properly fixed by just counting the number of packets to
be dropped inside the critical section, then effectively applying the multiple
wait_for_completion_timeout _after_ the RCU section.
- finally, there is an issue in set_channel ops (cfg80211.c), which fetches
the first vif (under RCU), and then uses this vif to send appropriate channel
configuration command (which sleeps at some point). Since used vif here comes
from the vif list, I don't think it is safe to leave earlier RCU section (vif
pointer needs to remain valid until command is sent to chip).
Because of the second point which would bring a not-so-clean fix, and the third
one for which I still don't have a proper fix, I am considering to submit a
revert for my RCU conversion series, to come back to SRCU. I don´t know if those
issues do or do not make SRCU usage more legitimate, but at least I feel like
really fixing it need slightly larger rework. I will still search for better
options, but if I do not find any, I will submit the revert.
Thanks,
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
next prev parent reply other threads:[~2024-05-28 9:31 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 13:24 [bug report] wifi: wilc1000: convert list management to RCU Dan Carpenter
2024-05-09 13:33 ` Dan Carpenter
2024-05-13 14:16 ` Alexis Lothoré
2024-05-14 8:54 ` Dan Carpenter
2024-05-14 8:59 ` Alexis Lothoré
2024-05-28 9:31 ` Alexis Lothoré [this message]
2024-05-28 9:44 ` Kalle Valo
2024-05-28 9:57 ` Alexis Lothoré
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0a9c592d-7db3-454e-a83f-901b1865b414@bootlin.com \
--to=alexis.lothore@bootlin.com \
--cc=Ajay.Kathat@microchip.com \
--cc=dan.carpenter@linaro.org \
--cc=kvalo@kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.