linux-kernel - Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: 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] [day] [month] [year] [list]

Message-ID: <eb0ca8d1-cf00-ae9f-5f35-79bb81da171b@intel.com>
Date:   Sun, 23 Oct 2022 19:47:52 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Brian Norris <briannorris@...omium.org>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        Haibo Chen <haibo.chen@....com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Michal Simek <michal.simek@...inx.com>,
        Faiz Abbas <faiz_abbas@...com>, linux-mmc@...r.kernel.org,
        Jonathan Hunter <jonathanh@...dia.com>,
        Al Cooper <alcooperx@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        Sowjanya Komatineni <skomatineni@...dia.com>,
        linux-kernel@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Sascha Hauer <s.hauer@...gutronix.de>, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for
 CQHCI

On 21/10/22 20:45, Florian Fainelli wrote:
> On 10/19/22 23:29, Adrian Hunter wrote:
>> On 20/10/22 01:19, Brian Norris wrote:
>>> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>>>> On 10/19/22 14:54, Brian Norris wrote:
>>>>> The same bug was already found and fixed for two other drivers, in v5.7
>>>>> and v5.9:
>>>>>
>>>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>>>
>>>>> The latter is especially prescient, saying "other drivers using CQHCI
>>>>> might benefit from a similar change, if they also have CQHCI reset by
>>>>> SDHCI_RESET_ALL."
>>>
>>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>>>        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>        struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>>> +    if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>>>> +        cqhci_deactivate(host->mmc);
>>>>> +
>>>>>        sdhci_reset(host, mask);
>>>>
>>>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>>>> be utilizing since you have access to the host and the mask to make that
>>>> decision?
>>>
>>> It potentially could.
>>>
>>> I don't know if this is a specified SDHCI behavior that really belongs
>>> in the common helper, or if this is just a commonly-shared behavior. Per
>>> the comments I quote above ("if they also have CQHCI reset by
>>> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
>>> behavior.
>>>
>>> I suppose it's not all that harmful to do this even if some SDHCI
>>> controller doesn't have the same behavior/quirk.
>>>
>>> I guess I also don't know if any SDHCI controllers will support command
>>> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
>>> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
>>> set MMC_CAP2_CQE.
>>
>> SDHCI and CQHCI are separate modules and are not dependent, so they cannot
>> call into each other directly (and should not).  A new CQE API would be
>> needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
>> and wrapped in mmc/host.h:
>>
>> static inline void mmc_cqe_notify_reset(struct mmc_host *host)
>> {
>>     if (host->cqe_ops->cqe_notify_reset)
>>         host->cqe_ops->cqe_notify_reset(host);
>> }
>>
>> Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
>> although in this case there is so little code it could be static inline and
>> added in a new include file instead, say sdhci-cqhci.h e.g.
>>
>> #include "cqhci.h"
>> #include "sdhci.h"
>>
>> static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
>> {
>>     if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
>>         host->mmc->cqe_private)
>>         cqhci_deactivate(host->mmc);
>>     sdhci_reset(host, mask);
>> }
>>
> 
> I like the simplicity of the inline helper, especially towards backports. May suggest to name it sdhci_and_cqhci_reset() to illustrate that it does both, and does not apply specifically CQHCI that would be "embedded" into SDHCI, but your call here.

sdhci_and_cqhci_reset() is fine by me