linux-kernel - Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

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:   Wed, 04 Jul 2018 15:18:51 +0200
From:   Stefan Agner <stefan@...er.ch>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Fabio Estevam <fabio.estevam@....com>,
        Haibo Chen <haibo.chen@....com>,
        Aisheng Dong <aisheng.dong@....com>,
        Michael Trimarchi <michael@...rulasolutions.com>,
        Russell King <rmk+kernel@...linux.org.uk>,
        linux-mmc@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

On 04.07.2018 13:16, Ulf Hansson wrote:
> On 4 July 2018 at 12:55, Stefan Agner <stefan@...er.ch> wrote:
>> On 04.07.2018 12:07, Ulf Hansson wrote:
>>> On 3 July 2018 at 10:48, Stefan Agner <stefan@...er.ch> wrote:
>>>> On 02.07.2018 16:36, Ulf Hansson wrote:
>>>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@...er.ch> wrote:
>>>>>> Some hosts are capable of running higher speed modes but do not
>>>>>> have the board support for it. Introduce a quirk which prevents
>>>>>> the stack from using modes running at 100MHz or faster.
>>>>>
>>>>> To cap the freq, use the DT property "max-frequency". To enable
>>>>> certain speed modes, use the corresponding speed mode binding. For
>>>>> example "sd-uhs-sdr*" and "mmc-hs200*".
>>>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt
>>>>
>>>> I had bad experience with max-frequency: Some higher speed modes seem
>>>> not to work reliably if constraint to low frequencies. E.g. we had lots
>>>> of devices fail in practise with HS400@...MHz... So it is doing what it
>>>> should, but it just seems that higher speed modes do not necessarily run
>>>> well with lower frequencies...
>>>>
>>>> So I'd rather prefer to limit speed modes as it is done right now.
>>>>
>>>>>
>>>>> In case the sdhci cap register, doesn't reflect the board support
>>>>> properly, such that you may want to disable some speed modes, then you
>>>>> may benefit from using the DT properties "sdhci-caps*.
>>>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt
>>>>
>>>> Hm, yeah I guess something like
>>>>
>>>> sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
>>>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>
>>>>
>>>> would come close.
>>>>
>>>> But it does not restrict MMC modes such as HS200/HS400. There seem to be
>>>> no mmc-caps...
>>>
>>> Right.
>>>
>>> The solution to fix this, should be to *not* set those DT properties,
>>> like "mmc-hs*" for example. That should work, no?
>>>
>>
>> The controller does not make use of the dt modes so far, so I can't not
>> set those properties...
> 
> Then where are the corresponding caps for the eMMC speed modes being set?
> 
> Can't you just avoid setting them?
> 

That was the right question:

On closer look, the higher speed MMC modes are actually not set at all!

So just using sdhci-caps-mask = <0x7 0x0>; (which masks
SDR104/SDR50/DDR50) is doing the job as far as I can tell.

>>
>>>>
>>>>
>>>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
>>>> restrict modes correctly. Currently the driver checks whether >=100MHz
>>>> pinctrl settings are available, and if not uses the quirk to restrict
>>>> higher speed modes. Removing that would break device tree backward
>>>> compatibility...
>>>
>>> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather
>>> how the pinctrl setting becomes interpreted when setting the quirk.
>>>
>>
>> Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just
>> not the quirk this driver needs.
>>
>> I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl
>> state according to uhs mode") chose the wrong quirk from the
>> beginning...
> 
> That's seems reasonable! But it's been there since 3.13, so I guess we
> have to think about backwards compatibility issues, as you stated.
> 

It seems that the driver already fakes
SDHCI_CAPABILITIES/SDHCI_CAPABILITIES_1 in esdhc_readl_le, so instead
using the sdhci-caps-mask we could implement the work around there. Not
very clean, but backward compatible and everything self contained in the
driver.

--
Stefan

>>
>> Afaict, the quirk needed here does not exist.
> 
> [...]
> 
> Kind regards
> Uffe