| Message ID | 1c6684f6-d3a8-4eaa-842d-c21fa2dd81c1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data | expand |
On July 7, 2024 2:22:49 PM Nikolay Nikolov <dobrev666@gmail.com> wrote: > Use mutex to prevent sdio bus to be put to sleep by the sdio_bus_watchdog > while sdio dataworker handles sdio_dpc data transfers. Any reason for sending 3 identical patches within the hour. As to the patch itself I would like to know if there is a reported issue being fixed here. What is the motivation behind this patch. Looking at the code I do not think the mutex is needed so please elaborate. Regards, Arend > Signed-off-by: Nikolay Nikolov <nikolay.nikolov@bench.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 ++++++ > 1 file changed, 6 insertions(+)
Hi, I am really sorry for the spamming ! I have not sent a patch to the Linux kernel mailing list for more than 20 years and mail clients do not behave as I expect. My first email was rejected from the mailing lists as it contained HTML. Indentation is not correct in the second one. I hope third one is correct. Just to make it clear this email I am using now is my private and nikolay.nikolov@bench.com is my corporate. I am involved in embedded development of a 1LV wifi module and we are using highly patched driver from infineon: https://github.com/Infineon/ifx-backports And we have problems with this driver. I see some flags are used to prevent a race conditions between brcmf_sdio_bus_watchdog() and brcmf_sdio_dataworker() Infineon added some debug messages: WARN: Read operation when bus is in sleep state WARN: Write operation when bus is in sleep state These messages appear from time to time in our embedded system. It turns out the flags used to prevent such condition are not enough - dpc_running, dpc_triggered. What happens in SMP system is watchdog thread checks these flags and finds them set as false and proceeds to set the sdio bus to sleep. Exactly at the same moment while watchdog is setting sdio bus mode to sleep brcmf_sdio_dataworker is started, sets these flags to true and continues to transmit data. Watchdog unaware of this as flags were already checked sets the sdio bus to sleep. As dataworker continues to work, it finds the sdio bus sleeping which is a problem. We are using this patch at the moment and we do not experience this issue anymore. I will also send the patch to Infineon next week. We communicate through a reseller with them so it will take some time and effort. I hope I explained it good enough. If still not clear, please let me know. I will try to explain it better if I have missed something above. I think using mutex makes at least dpc_running flag irrelevant and could be removed. But for now I did not want to add more complexity in my patch. If you agree I can try to remove it in a new patch. Regards, Nikolay Nikolov On Sun, Jul 7, 2024 at 3:21 PM Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > > On July 7, 2024 2:22:49 PM Nikolay Nikolov <dobrev666@gmail.com> wrote: > > > Use mutex to prevent sdio bus to be put to sleep by the sdio_bus_watchdog > > while sdio dataworker handles sdio_dpc data transfers. > > Any reason for sending 3 identical patches within the hour. > > As to the patch itself I would like to know if there is a reported issue > being fixed here. What is the motivation behind this patch. Looking at the > code I do not think the mutex is needed so please elaborate. > > Regards, > Arend > > > Signed-off-by: Nikolay Nikolov <nikolay.nikolov@bench.com> > > --- > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > >
Nikolay Nikolov <dobrev666@gmail.com> writes: > I am really sorry for the spamming ! > I have not sent a patch to the Linux kernel mailing list for more than > 20 years and mail clients do not behave as I expect. My first email > was rejected from the mailing lists as it contained HTML. Indentation > is not correct in the second one. I hope third one is correct. BTW I recommend reading the documentation from our wiki, link below. That should save both your and our time.
On Tue, Jul 9, 2024 at 12:12 PM Kalle Valo <kvalo@kernel.org> wrote: > > Nikolay Nikolov <dobrev666@gmail.com> writes: > > > I am really sorry for the spamming ! > > I have not sent a patch to the Linux kernel mailing list for more than > > 20 years and mail clients do not behave as I expect. My first email > > was rejected from the mailing lists as it contained HTML. Indentation > > is not correct in the second one. I hope third one is correct. > > BTW I recommend reading the documentation from our wiki, link below. > That should save both your and our time. > I was reading for several days this and the kernel mailing list wiki before sending the patch. The problem was the email clients. Anyway I want to mention something which is not present here. There are some memory barriers around this dpc_running which I think are supposed to prevent such an issue. But looks like this do not help much. With the usage of mutex those memory barriers could be removed as well. I am already using such a patch in our embedded system. Please, let me know if I should submit this patch. I promise I will do my best not to spam anymore. Regards, Nikolay Nikolov > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 6b38d9de71af..26d0bce6cedc 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -524,6 +524,7 @@ struct brcmf_sdio { bool txglom; /* host tx glomming enable flag */ u16 head_align; /* buffer pointer alignment */ u16 sgentry_align; /* scatter-gather buffer alignment */ + struct mutex dpc_mutex; }; /* clkstate */ @@ -3724,6 +3725,7 @@ static void brcmf_sdio_bus_watchdog(struct brcmf_sdio *bus) } #endif /* DEBUG */ + mutex_lock(&bus->dpc_mutex); /* On idle timeout clear activity flag and/or turn off clock */ if (!bus->dpc_triggered) { rmb(); @@ -3748,6 +3750,7 @@ static void brcmf_sdio_bus_watchdog(struct brcmf_sdio *bus) } else { bus->idlecount = 0; } + mutex_unlock(&bus->dpc_mutex); } static void brcmf_sdio_dataworker(struct work_struct *work) @@ -3755,6 +3758,7 @@ static void brcmf_sdio_dataworker(struct work_struct *work) struct brcmf_sdio *bus = container_of(work, struct brcmf_sdio, datawork); + mutex_lock(&bus->dpc_mutex); bus->dpc_running = true; wmb(); while (READ_ONCE(bus->dpc_triggered)) { @@ -3763,6 +3767,7 @@ static void brcmf_sdio_dataworker(struct work_struct *work) bus->idlecount = 0; } bus->dpc_running = false; + mutex_unlock(&bus->dpc_mutex); if (brcmf_sdiod_freezing(bus->sdiodev)) { brcmf_sdiod_change_state(bus->sdiodev, BRCMF_SDIOD_DOWN); brcmf_sdiod_try_freeze(bus->sdiodev); @@ -4525,6 +4530,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) /* SR state */ bus->sr_enabled = false; + mutex_init(&bus->dpc_mutex); brcmf_dbg(INFO, "completed!!\n");
Use mutex to prevent sdio bus to be put to sleep by the sdio_bus_watchdog while sdio dataworker handles sdio_dpc data transfers. Signed-off-by: Nikolay Nikolov <nikolay.nikolov@bench.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 ++++++ 1 file changed, 6 insertions(+) ---