linux-kernel - Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Dec 2019 15:32:43 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        linux-mmc@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi

On Tue, Dec 10, 2019 at 05:15:58PM +0300, Dmitry Osipenko wrote:
> 10.12.2019 15:52, Thierry Reding пишет:
> > On Tue, Dec 10, 2019 at 04:40:11AM +0300, Dmitry Osipenko wrote:
> >> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
> >> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
> >> In a result high-speed mode isn't enabled for the WiFi card and this
> >> results in a malfunctioning SDIO communication.
> >>
> >>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
> >>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
> >>
> >> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
> >> the problem, let's do the same in upstream.
> >>
> >> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
> >> which overrides card's info for the TI wl1251 WiFi.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> >> ---
> >>  drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> > 
> > This seems like the wrong place to do this. If this is specific to this
> > WiFi SDIO chip this should be handled at the SDIO card or function
> > level. It seems like the SDIO infrastructure doesn't currently allow
> > this because the OF nodes are attached to the card after
> > mmc_sdio_init_card(), whereas it seems like the quirk is already needed
> > during mmc_sdio_init_card().
> > 
> > That said, I think we could have some common code that's executed as
> > part of mmc_attach_sdio() (and before mmc_sdio_init_card()).
> > 
> > Actually, it looks like we already have something like that.
> > mmc_sdio_init_card() calls mmc_fixup_device() with sdio_fixup_methods
> > after doing some very basic initialization. Do you know if things start
> > to go wrong before or after that point? It might be worth looking at
> > that SDIO fixup array and add something that would override the CCCR
> > support. That would fix things in a more generic way rather than
> > requiring every host controller driver to duplicate this quirk.
> 
> Hello Thierry,
> 
> Thank you very much for the suggestion, looks like indeed it is possible
> to make workaround in a generic way.
> 
> Ulf / Adrian, will something like this be acceptable:
> 
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 7bd392d55cfa..a6001f210b9e 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -150,6 +150,12 @@ static inline void __maybe_unused
> add_limit_rate_quirk(struct mmc_card *card,
>  	card->quirk_max_rate = data;
>  }
> 
> +static inline void __maybe_unused add_high_speed_quirk(struct mmc_card
> *card,
> +						       int data)
> +{
> +	card->cccr.high_speed = data;
> +}
> +
>  /*
>   * Quirk add/remove for MMC products.
>   */
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index 3dba15bccce2..a824c0caa7fb 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -142,6 +142,9 @@ static const struct mmc_fixup sdio_fixup_methods[] = {
>  	SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
>  		   add_limit_rate_quirk, 150000000),
> 
> +	SDIO_FIXUP(SDIO_VENDOR_ID_BROADCOM, SDIO_DEVICE_ID_BROADCOM_4329,
> +		   add_high_speed_quirk, 1),
> +
>  	END_FIXUP
>  };
> 

Looks good to me:

Reviewed-by: Thierry Reding <treding@...dia.com>

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)