[PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[Nikolay Nikolov]

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(+)
---
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");
  

Re: [PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

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(+)



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[Nikolay Nikolov]

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(+)
>
>

Re: [PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[Nikolay Nikolov]

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

[PATCH 1/1] wifi: brcm80211: brcmfmac: Prevent sdio bus going to sleep while transfering data

[Nikolay Nikolov]

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(+)

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");