linux-kernel - Re: [RFC PATCH V3 04/21] mmc: core: UHS-II support, try to select UHS-II interface

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]

Date:   Fri, 21 Aug 2020 17:28:08 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        Ben Chuang <benchuanggli@...il.com>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ben Chuang <ben.chuang@...esyslogic.com.tw>,
        Takahiro Akashi <takahiro.akashi@...aro.org>,
        greg.tu@...esyslogic.com.tw
Subject: Re: [RFC PATCH V3 04/21] mmc: core: UHS-II support, try to select
 UHS-II interface

On 21/08/20 3:25 pm, Ulf Hansson wrote:
> [...]
> 
>>>> @@ -2300,6 +2304,33 @@ void mmc_rescan(struct work_struct *work)
>>>>                 goto out;
>>>>         }
>>>>
>>>> +       if (host->caps & MMC_CAP_UHS2) {
>>>> +               /*
>>>> +                * Start to try UHS-II initialization from 52MHz to 26MHz
>>>> +                * (RCLK range) per spec.
>>>> +                */
>>>> +               for (i = 0; i < ARRAY_SIZE(uhs2_freqs); i++) {
>>>> +                       unsigned int freq = uhs2_freqs[i];
>>>> +                       int err;
>>>> +
>>>> +                       err = mmc_uhs2_rescan_try_freq(host,
>>>> +                                                      max(freq, host->f_min));
>>>> +                       if (!err) {
>>>> +                               mmc_release_host(host);
>>>> +                               goto out;
>>>> +                       }
>>>> +
>>>> +                       if (err == UHS2_PHY_INIT_ERR)
>>>> +                               /* UHS2 IF detect or Lane Sync error.
>>>> +                                * Try legacy interface.
>>>> +                                */
>>>> +                               break;
>>>> +
>>>> +                       if (freq <= host->f_min)
>>>> +                               break;
>>>> +               }
>>>
>>> Assuming we change the initialization order, trying to initialize the
>>> legacy SD interface first to figure out if UHS-II is supported, then I
>>> think we should be able to move the entire code above into a the
>>> UHS-II specific code/path.
>>
>> If the host tries to use the SD interface first,
>> some failure status needs to be considered.
>>
>> For example, first run in SD mode, try UHS-II interface failure,
>>  and then return to SD flow again.
>> I don't know a good way to go back to SD flow again.
> 
> Right, a re-try path for the legacy SD interface is a very good idea!
> However, I don't think the initial support for UHS-II needs to cover
> it. Instead we can add that on top, don't you think?
> 
> As a matter of fact, we could even use something like that for
> different legacy SD speed modes. For example, if UHS-I SDR104 fails we
> could try with UHS-I SDR25.
> 
>>
>>>
>>>> +       }
>>>> +
>>>>         for (i = 0; i < ARRAY_SIZE(freqs); i++) {
>>>>                 unsigned int freq = freqs[i];
>>>>                 if (freq > host->f_max) {
>>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>>> index 5a2210c25aa7..c5b071bd98b3 100644
>>>> --- a/drivers/mmc/core/sd.c
>>>> +++ b/drivers/mmc/core/sd.c
>>>> @@ -901,6 +901,20 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>>>>                 err = mmc_read_switch(card);
>>>>                 if (err)
>>>>                         return err;
>>>> +               if (host->flags & MMC_UHS2_INITIALIZED) {
>>>
>>> Rather than using host->flags, to tweak the behavior of
>>> mmc_sd_setup_card() to support UHS-II, I would prefer to give
>>> mmc_sd_setup_card() a new in-parameter that it can look at instead.
>>
>> Do you mean that adding a new in-parameter to mmc_sd_setup_card() likes this
>> mmc_sd_setup_card(struct mmc_host *, struct mmc_card *, boot reinit,
>> boot uhsii); ?
> 
> Correct.
> 
> [...]
> 
> Looks like we have covered most of the review for the mmc core
> changes. But please tell me, if there is anything else you want me to
> look at - at any time. Otherwise I am deferring to wait for a new
> version of the series.
> 
> If I get some time, I may start to help with hacking some code.
> Perhaps I can do some preparations, so it makes it easier for you to
> add the UHS-II specific code. If so, I will let you know about it, of
> course.
> 
> When it comes to the changes to SDHCI, I am relying on Adrian to give
> his opinions.

I have made some comments.  The thrust of which is:
- get all the UHS-II code into sdhci-uhs2 (not sdhci.c)
- make the driver (i.e sdhci-pci-gli) set existing mmc host ops callbacks
and sdhci host ops callbacks as much as possible to provide UHS-II
functionality i.e. avoid adding new hooks if possible
- refactoring and exporting functions from sdhci.c where that can be done
logically, but otherwise writing separate code for UHS-II