lkml.org 
[lkml]   [2020]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] brcmfmac: abort and release host after error
    On Wed, Jan 29, 2020 at 10:04 AM Brian Norris <briannorris@chromium.org> wrote:
    >
    > Hi Franky,
    >
    > [I'm very unfamiliar with this driver, but I had the same questions as
    > Guenter, I think:]
    >
    > On Tue, Jan 28, 2020 at 04:57:59PM -0800, Franky Lin wrote:
    > > On Tue, Jan 28, 2020 at 4:05 PM Guenter Roeck <linux@roeck-us.net> wrote:
    > > > On Tue, Jan 28, 2020 at 03:14:45PM -0800, Doug Anderson wrote:
    > > > > On Tue, Jan 28, 2020 at 2:15 PM Guenter Roeck <linux@roeck-us.net> wrote:
    > > > > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
    > > > > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
    > > > > > @@ -1938,6 +1938,8 @@ static uint brcmf_sdio_readframes(struct brcmf_sdio *bus, uint maxframes)
    > > > > > if (brcmf_sdio_hdparse(bus, bus->rxhdr, &rd_new,
    > > > > > BRCMF_SDIO_FT_NORMAL)) {
    > > > > > rd->len = 0;
    > > > > > + brcmf_sdio_rxfail(bus, true, true);
    > > > > > + sdio_release_host(bus->sdiodev->func1);
    > > > >
    > > > > I don't know much about this driver so I don't personally know if
    > > > > "true, true" is the correct thing to pass to brcmf_sdio_rxfail(), but
    > > > > it seems plausible. Definitely the fix to call sdio_release_host() is
    > > > > sane.
    > > > >
    > > > > Thus, unless someone knows for sure that brcmf_sdio_rxfail()'s
    > > > > parameters should be different:
    > > > >
    > > > Actually, looking at brcmf_sdio_hdparse() and its other callers,
    > > > I think it may not be needed at all - other callers don't do it, and
    > > > there already are some calls to brcmf_sdio_rxfail() in that function.
    > > > It would be nice though to get a confirmation before I submit v2.
    > >
    > > I think invoking rxfail with both abort and NACK set to true is the
    > > right thing to do here so that the pipeline can be properly purged.
    >
    > Thanks for looking here. I'm not sure I totally understand your answer:
    > brcmf_sdio_hdparse() already calls brcmf_sdio_rxfail() in several error
    > cases. Is it really OK to call it twice in a row?

    Yes. brcmf_sdio_rxglom does the same thing that calls
    brcmf_sdio_rxfail again in error handling. For this instance I think
    it's better using the same logic as the length mismatch block below (
    calling brcmf_sdio_rxfail with true ture).

    Thanks,
    - Franky

    \
     
     \ /
      Last update: 2020-01-30 00:13    [W:6.108 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site