| Message ID | 20190607223716.119277-4-dianders@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: sdio: Deal better w/ transmission errors related to idle | expand |
> -----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
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
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 >
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
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
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
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
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
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; }
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(+)