[v3,3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
diff mbox series

Message ID 20190607223716.119277-4-dianders@chromium.org
State New
Headers show
Series
  • brcmfmac: sdio: Deal better w/ transmission errors related to idle
Related show

Commit Message

Doug Anderson June 7, 2019, 10:37 p.m. UTC
There are certain cases, notably when transitioning between sleep and
active state, when Broadcom SDIO WiFi cards will produce errors on the
SDIO bus.  This is evident from the source code where you can see that
we try commands in a loop until we either get success or we've tried
too many times.  The comment in the code reinforces this by saying
"just one write attempt may fail"

Unfortunately these failures sometimes end up causing an "-EILSEQ"
back to the core which triggers a retuning of the SDIO card and that
blocks all traffic to the card until it's done.

Let's disable retuning around the commands we expect might fail.

Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Expect errors for all of brcmf_sdio_kso_control() (Adrian).

Changes in v2: None

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Adrian Hunter June 10, 2019, 8:56 a.m. UTC | #1
> -----Original Message-----
> From: Douglas Anderson [mailto:dianders@chromium.org]
> Sent: Saturday, June 8, 2019 1:37 AM
> To: Ulf Hansson <ulf.hansson@linaro.org>; Kalle Valo
> <kvalo@codeaurora.org>; Hunter, Adrian <adrian.hunter@intel.com>; Arend
> van Spriel <arend.vanspriel@broadcom.com>
> Cc: brcm80211-dev-list.pdl@broadcom.com; linux-
> rockchip@lists.infradead.org; Double Lo <double.lo@cypress.com>;
> briannorris@chromium.org; linux-wireless@vger.kernel.org; Naveen Gupta
> <naveen.gupta@cypress.com>; Madhan Mohan R
> <madhanmohan.r@cypress.com>; mka@chromium.org; Wright Feng
> <wright.feng@cypress.com>; Chi-Hsien Lin <chi-hsien.lin@cypress.com>;
> netdev@vger.kernel.org; brcm80211-dev-list@cypress.com; Douglas
> Anderson <dianders@chromium.org>; Franky Lin
> <franky.lin@broadcom.com>; linux-kernel@vger.kernel.org; Madhan Mohan
> R <MadhanMohan.R@cypress.com>; Hante Meuleman
> <hante.meuleman@broadcom.com>; YueHaibing
> <yuehaibing@huawei.com>; David S. Miller <davem@davemloft.net>
> Subject: [PATCH v3 3/5] brcmfmac: sdio: Disable auto-tuning around
> commands expected to fail
> 
> There are certain cases, notably when transitioning between sleep and active
> state, when Broadcom SDIO WiFi cards will produce errors on the SDIO bus.
> This is evident from the source code where you can see that we try
> commands in a loop until we either get success or we've tried too many
> times.  The comment in the code reinforces this by saying "just one write
> attempt may fail"
> 
> Unfortunately these failures sometimes end up causing an "-EILSEQ"
> back to the core which triggers a retuning of the SDIO card and that blocks all
> traffic to the card until it's done.
> 
> Let's disable retuning around the commands we expect might fail.
> 
> Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Expect errors for all of brcmf_sdio_kso_control() (Adrian).
> 
> Changes in v2: None
> 
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 4a750838d8cd..4040aae1f9ed 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -16,6 +16,7 @@
>  #include <linux/mmc/sdio_ids.h>
>  #include <linux/mmc/sdio_func.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/core.h>

SDIO function drivers should not really include linux/mmc/core.h
(Also don't know why linux/mmc/card.h is included)

>  #include <linux/semaphore.h>
>  #include <linux/firmware.h>
>  #include <linux/module.h>
> @@ -667,6 +668,8 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool
> on)
> 
>  	brcmf_dbg(TRACE, "Enter: on=%d\n", on);
> 
> +	mmc_expect_errors_begin(bus->sdiodev->func1->card->host);
> +
>  	wr_val = (on << SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
>  	/* 1st KSO write goes to AOS wake up core if device is asleep  */
>  	brcmf_sdiod_writeb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR,
> wr_val, &err); @@ -727,6 +730,8 @@ brcmf_sdio_kso_control(struct
> brcmf_sdio *bus, bool on)
>  	if (try_cnt > MAX_KSO_ATTEMPTS)
>  		brcmf_err("max tries: rd_val=0x%x err=%d\n", rd_val, err);
> 
> +	mmc_expect_errors_end(bus->sdiodev->func1->card->host);
> +
>  	return err;
>  }
> 
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
Doug Anderson June 10, 2019, 4:50 p.m. UTC | #2
Hi,

On Mon, Jun 10, 2019 at 1:56 AM Hunter, Adrian <adrian.hunter@intel.com> wrote:
>
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/mmc/sdio_ids.h>
> >  #include <linux/mmc/sdio_func.h>
> >  #include <linux/mmc/card.h>
> > +#include <linux/mmc/core.h>
>
> SDIO function drivers should not really include linux/mmc/core.h
> (Also don't know why linux/mmc/card.h is included)

OK, so I guess you're requesting an extra level of "sdio_" wrappers
for all the functions I need to call.  I don't think the wrappers buy
us a ton other than to abstract things a little bit and make it look
prettier.  :-)  ...but certainly I can code that up if that's what
everyone wants.

Just to make sure, I looked in "drivers/net/wireless/" and I do see
quite a few instances of "mmc_" functions being used.  That doesn't
mean all these instances are correct but it does appear to be
commonplace.  Selected examples:

drivers/net/wireless/ath/ath10k/sdio.c:
  ret = mmc_hw_reset(ar_sdio->func->card->host);

drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
  mmc_set_data_timeout(md, func->card);
  mmc_wait_for_req(func->card->host, mr);

drivers/net/wireless/marvell/mwifiex/sdio.c:
  mmc_hw_reset(func->card->host);

drivers/net/wireless/rsi/rsi_91x_sdio.c:
  err = mmc_wait_for_cmd(host, &cmd, 3);


...anyway, I'll give it a few days and if nobody else chimes in then
I'll assume you indeed want "sdio_" wrappers for things and I'll post
a v4.  If patch #1 happens to land in the meantime then I won't
object.  ;-)


-Doug
Adrian Hunter June 11, 2019, 7:17 a.m. UTC | #3
On 10/06/19 7:50 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jun 10, 2019 at 1:56 AM Hunter, Adrian <adrian.hunter@intel.com> wrote:
>>
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/mmc/sdio_ids.h>
>>>  #include <linux/mmc/sdio_func.h>
>>>  #include <linux/mmc/card.h>
>>> +#include <linux/mmc/core.h>
>>
>> SDIO function drivers should not really include linux/mmc/core.h
>> (Also don't know why linux/mmc/card.h is included)
> 
> OK, so I guess you're requesting an extra level of "sdio_" wrappers
> for all the functions I need to call.  I don't think the wrappers buy
> us a ton other than to abstract things a little bit and make it look
> prettier.  :-)  ...but certainly I can code that up if that's what
> everyone wants.

I guess it is really up to Ulf.

> 
> Just to make sure, I looked in "drivers/net/wireless/" and I do see
> quite a few instances of "mmc_" functions being used.  That doesn't
> mean all these instances are correct but it does appear to be
> commonplace.  Selected examples:
> 
> drivers/net/wireless/ath/ath10k/sdio.c:
>   ret = mmc_hw_reset(ar_sdio->func->card->host);
> 
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
>   mmc_set_data_timeout(md, func->card);
>   mmc_wait_for_req(func->card->host, mr);
> 
> drivers/net/wireless/marvell/mwifiex/sdio.c:
>   mmc_hw_reset(func->card->host);
> 
> drivers/net/wireless/rsi/rsi_91x_sdio.c:
>   err = mmc_wait_for_cmd(host, &cmd, 3);
> 
> 
> ...anyway, I'll give it a few days and if nobody else chimes in then
> I'll assume you indeed want "sdio_" wrappers for things and I'll post
> a v4.  If patch #1 happens to land in the meantime then I won't
> object.  ;-)
> 
> 
> -Doug
>
Ulf Hansson June 12, 2019, 10:10 a.m. UTC | #4
On Mon, 10 Jun 2019 at 18:50, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jun 10, 2019 at 1:56 AM Hunter, Adrian <adrian.hunter@intel.com> wrote:
> >
> > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/mmc/sdio_ids.h>
> > >  #include <linux/mmc/sdio_func.h>
> > >  #include <linux/mmc/card.h>
> > > +#include <linux/mmc/core.h>
> >
> > SDIO function drivers should not really include linux/mmc/core.h
> > (Also don't know why linux/mmc/card.h is included)
>
> OK, so I guess you're requesting an extra level of "sdio_" wrappers
> for all the functions I need to call.  I don't think the wrappers buy
> us a ton other than to abstract things a little bit and make it look
> prettier.  :-)  ...but certainly I can code that up if that's what
> everyone wants.

Are the new code you refer to going to be used for anything else but
SDIO? If not, please put them in the sdio specific headers instead.

BTW, apologize for not looking at this series any earlier, but I will
come to it soon.

>
> Just to make sure, I looked in "drivers/net/wireless/" and I do see
> quite a few instances of "mmc_" functions being used.  That doesn't
> mean all these instances are correct but it does appear to be
> commonplace.  Selected examples:
>
> drivers/net/wireless/ath/ath10k/sdio.c:
>   ret = mmc_hw_reset(ar_sdio->func->card->host);

mmc_hw_reset() is already an exported function, used by the mmc block
layer. So I think this is okay.

>
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
>   mmc_set_data_timeout(md, func->card);
>   mmc_wait_for_req(func->card->host, mr);

These are not okay, none of these things calls should really be done
from an SDIO func driver.

It tells me that the func driver is a doing workaround for something
that should be managed in a common way.

>
> drivers/net/wireless/marvell/mwifiex/sdio.c:
>   mmc_hw_reset(func->card->host);

Okay.

>
> drivers/net/wireless/rsi/rsi_91x_sdio.c:
>   err = mmc_wait_for_cmd(host, &cmd, 3);

Not okay.

>
>
> ...anyway, I'll give it a few days and if nobody else chimes in then
> I'll assume you indeed want "sdio_" wrappers for things and I'll post
> a v4.  If patch #1 happens to land in the meantime then I won't
> object.  ;-)

Adrian has a very good point. We need to strive to avoid exporting
APIs to here and there and just trust that they will be used wisely.

If the above calls to mmc_wait_for_req|cmd() and
mmc_set_data_timeout() could have been avoided, we would probably have
a more proper solution by now.

Kind regards
Uffe
Arend Van Spriel June 12, 2019, 11:11 a.m. UTC | #5
On 6/12/2019 12:10 PM, Ulf Hansson wrote:
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
>>    mmc_set_data_timeout(md, func->card);
>>    mmc_wait_for_req(func->card->host, mr);
> These are not okay, none of these things calls should really be done
> from an SDIO func driver.
> 
> It tells me that the func driver is a doing workaround for something
> that should be managed in a common way.

We are using some low-level functions passing chain of skbuff to the 
device using CMD53 with scatterlist. If I recall correctly Marvell made 
an attempt to have a similar function for it in the mmc stack. Not sure 
if that ever made it in. If so I can rework our driver using that API. 
If not, I can make a new attempt.

Regards,
Arend
Ulf Hansson June 12, 2019, 11:48 a.m. UTC | #6
On Wed, 12 Jun 2019 at 13:11, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 6/12/2019 12:10 PM, Ulf Hansson wrote:
> >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
> >>    mmc_set_data_timeout(md, func->card);
> >>    mmc_wait_for_req(func->card->host, mr);
> > These are not okay, none of these things calls should really be done
> > from an SDIO func driver.
> >
> > It tells me that the func driver is a doing workaround for something
> > that should be managed in a common way.
>
> We are using some low-level functions passing chain of skbuff to the
> device using CMD53 with scatterlist. If I recall correctly Marvell made
> an attempt to have a similar function for it in the mmc stack. Not sure
> if that ever made it in. If so I can rework our driver using that API.
> If not, I can make a new attempt.

I recall there were some patches, but not sure why we didn't merge them.

Anyway, if you want to move this forward, that would be awesome!

Kind regards
Uffe
Arend Van Spriel June 12, 2019, 1:58 p.m. UTC | #7
On 6/12/2019 1:48 PM, Ulf Hansson wrote:
> On Wed, 12 Jun 2019 at 13:11, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 6/12/2019 12:10 PM, Ulf Hansson wrote:
>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
>>>>     mmc_set_data_timeout(md, func->card);
>>>>     mmc_wait_for_req(func->card->host, mr);
>>> These are not okay, none of these things calls should really be done
>>> from an SDIO func driver.
>>>
>>> It tells me that the func driver is a doing workaround for something
>>> that should be managed in a common way.
>>
>> We are using some low-level functions passing chain of skbuff to the
>> device using CMD53 with scatterlist. If I recall correctly Marvell made
>> an attempt to have a similar function for it in the mmc stack. Not sure
>> if that ever made it in. If so I can rework our driver using that API.
>> If not, I can make a new attempt.
> 
> I recall there were some patches, but not sure why we didn't merge them.
> 
> Anyway, if you want to move this forward, that would be awesome!

Let's scope it before moving forward. Our use-case is to transfer a
chain of skbuff's. I am pretty sure that is not something we want to
deal with in mmc stack api. So I suppose passing a scatterlist is more
sensible, right? Maybe on sdio layer of the stack we could consider
dealing with skbuff's for network func drivers?

Let me see if I can find those Marvell patches. Might be a good start.

Regards,
Arend
Ulf Hansson June 13, 2019, 9:48 a.m. UTC | #8
On Wed, 12 Jun 2019 at 15:58, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
>
> On 6/12/2019 1:48 PM, Ulf Hansson wrote:
> > On Wed, 12 Jun 2019 at 13:11, Arend Van Spriel
> > <arend.vanspriel@broadcom.com> wrote:
> >>
> >> On 6/12/2019 12:10 PM, Ulf Hansson wrote:
> >>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
> >>>>     mmc_set_data_timeout(md, func->card);
> >>>>     mmc_wait_for_req(func->card->host, mr);
> >>> These are not okay, none of these things calls should really be done
> >>> from an SDIO func driver.
> >>>
> >>> It tells me that the func driver is a doing workaround for something
> >>> that should be managed in a common way.
> >>
> >> We are using some low-level functions passing chain of skbuff to the
> >> device using CMD53 with scatterlist. If I recall correctly Marvell made
> >> an attempt to have a similar function for it in the mmc stack. Not sure
> >> if that ever made it in. If so I can rework our driver using that API.
> >> If not, I can make a new attempt.
> >
> > I recall there were some patches, but not sure why we didn't merge them.
> >
> > Anyway, if you want to move this forward, that would be awesome!
>
> Let's scope it before moving forward. Our use-case is to transfer a
> chain of skbuff's. I am pretty sure that is not something we want to
> deal with in mmc stack api. So I suppose passing a scatterlist is more
> sensible, right? Maybe on sdio layer of the stack we could consider
> dealing with skbuff's for network func drivers?

Passing a scatter gather list seems reasonable. Ideally we should be
highly influenced with how buffers and dealt with for mmc block
requests.

Some information that may be needed by upper SDIO layers is the
segment/block constraints set by the MMC/SDIO host controller/driver.
The below is what we have today (see include/linux/mmc/host.h):

max_seg_size;   /* see blk_queue_max_segment_size */
max_segs;       /* see blk_queue_max_segments */
max_req_size;   /* maximum number of bytes in one req */
max_blk_size;   /* maximum size of one mmc block */
max_blk_count;  /* maximum number of blocks in one req */

Ideally we don't want SDIO func drivers to access these directly from
the ->host pointer, but rather via new SDIO func APIs.

>
> Let me see if I can find those Marvell patches. Might be a good start.

Great! Thanks!

Kind regards
Uffe

Patch
diff mbox series

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 4a750838d8cd..4040aae1f9ed 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -16,6 +16,7 @@ 
 #include <linux/mmc/sdio_ids.h>
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/core.h>
 #include <linux/semaphore.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
@@ -667,6 +668,8 @@  brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
 
 	brcmf_dbg(TRACE, "Enter: on=%d\n", on);
 
+	mmc_expect_errors_begin(bus->sdiodev->func1->card->host);
+
 	wr_val = (on << SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
 	/* 1st KSO write goes to AOS wake up core if device is asleep  */
 	brcmf_sdiod_writeb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR, wr_val, &err);
@@ -727,6 +730,8 @@  brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
 	if (try_cnt > MAX_KSO_ATTEMPTS)
 		brcmf_err("max tries: rd_val=0x%x err=%d\n", rd_val, err);
 
+	mmc_expect_errors_end(bus->sdiodev->func1->card->host);
+
 	return err;
 }