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 [off-list ref] wrote:

quoted

On Tue, Jan 28, 2020 at 03:14:45PM -0800, Doug Anderson wrote:

quoted

On Tue, Jan 28, 2020 at 2:15 PM Guenter Roeck [off-list ref] wrote:

quoted

--- 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?

Brian