linux-kernel - Re: [PATCH v3 6/7] mmc: sdhci_am654: Fix SDHCI_RESET

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]

Message-ID: <Y1higmSUMLsxvXyq@google.com>
Date:   Tue, 25 Oct 2022 15:26:10 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        linux-mmc@...r.kernel.org, Al Cooper <alcooperx@...il.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Sowjanya Komatineni <skomatineni@...dia.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        NXP Linux Team <linux-imx@....com>,
        Thierry Reding <thierry.reding@...il.com>,
        Fabio Estevam <festevam@...il.com>,
        Michal Simek <michal.simek@...inx.com>,
        linux-kernel@...r.kernel.org, Shawn Guo <shawnguo@...nel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        linux-arm-msm@...r.kernel.org, Haibo Chen <haibo.chen@....com>,
        Andy Gross <agross@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        Faiz Abbas <faiz_abbas@...com>,
        Jonathan Hunter <jonathanh@...dia.com>
Subject: Re: [PATCH v3 6/7] mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI

On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote:
> On 10/25/22 14:45, Brian Norris wrote:
> > On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote:
> > > On 24/10/22 20:55, Brian Norris wrote:
> > > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> > > > index 8f1023480e12..6a282c7a221e 100644
> > > > --- a/drivers/mmc/host/sdhci_am654.c
> > > > +++ b/drivers/mmc/host/sdhci_am654.c
> > 
> > > > @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
> > > >   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > >   	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> > > > -	sdhci_reset(host, mask);
> > > > +	sdhci_and_cqhci_reset(host, mask);
> > > >   	if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
> > > >   		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> > > 
> > > What about sdhci_reset in sdhci_am654_ops ?
> > 
> > Oops, I think you caught a big fallacy in some of my patches: I assumed
> > there was a single reset() implementation in a given driver (an unwise
> > assumption, I realize). I see at least sdhci-brcmstb.c also has several
> > variant ops that call sdhci_reset(), and I should probably convert them
> > too.
> 
> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the
> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible
> which implies using brcmstb_reset().

I don't see any in-tree device trees for these chips (which is OK), and
that's not what the Documentation/ says, and AFAICT nothing in the
driver is limiting other variants from specifying the "supports-cqe"
flag in their (out-of-tree) device tree. The closest thing I see is that
an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on
brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I
missing something?

Now of course, you probably know behind the scenes that there are no
other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT
I have no way of knowing that a priori. The driver and bindings give
(too much?) flexibility.

Poking around, I think the only other one I might have missed would be
gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is
otherwise relying on the default sdhci_pci_ops. So I'd either have to
change the common sdhci_pci_ops, or else start a new copy/paste/modify
'struct sdhci_ops' for it... This really does start to get messy when
poking around on drivers I can't test. As in, it shouldn't be harmful
to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they
aren't using some other CQE implementation), but the more invasive it
gets (say, rewriting a bunch of other ops), the easier it is to get
something wrong.

Thoughts welcome.

Brian