linux-kernel - Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

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:   Mon, 30 Jan 2017 11:40:48 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Gregory CLEMENT <gregory.clement@...e-electrons.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Ziji Hu <huziji@...vell.com>, Jimmy Xu <zmxu@...vell.com>,
        Jisheng Zhang <jszhang@...vell.com>,
        Nadav Haklai <nadavh@...vell.com>, Ryan Gao <ygao@...vell.com>,
        Doug Jones <dougj@...vell.com>, Victor Gu <xigu@...vell.com>,
        "Wei(SOCP) Liu" <liuw@...vell.com>,
        Wilson Ding <dingwei@...vell.com>,
        Yehuda Yitschak <yehuday@...vell.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Hanna Hawa <hannah@...vell.com>,
        Kostya Porotchkin <kostap@...vell.com>
Subject: Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core
 functionality

On 30/01/17 11:10, Ulf Hansson wrote:
> On 28 January 2017 at 09:16, Adrian Hunter <adrian.hunter@...el.com> wrote:
>> On 27/01/2017 5:12 p.m., Ulf Hansson wrote:
>>>
>>> On 26 January 2017 at 13:39, Adrian Hunter <adrian.hunter@...el.com>
>>> wrote:
>>>>
>>>> On 26/01/17 12:50, Ulf Hansson wrote:
>>>>>
>>>>> On 11 January 2017 at 18:19, Gregory CLEMENT
>>>>> <gregory.clement@...e-electrons.com> wrote:
>>>>>>
>>>>>> +                       priv->init_card_type = MMC_TYPE_MMC;
>>>>>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>>>>>> +
>>>>>> +                       /*
>>>>>> +                        * Force to clear BUS_TEST to
>>>>>> +                        * skip bus_test_pre and bus_test_post
>>>>>> +                        */
>>>>>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>>>>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>>>>
>>>>>
>>>>> This cap is a bit strange. It was added several years ago by Adrian
>>>>> Hunter, but I am wondering about the reason to why it's needed.
>>>>>
>>>>
>>>> MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.
>>>>
>>>> I think it was added to enable people to choose whether they wanted a
>>>> large
>>>> or small erase granularity.  That probably doesn't matter if the card
>>>> supports TRIM.
>>>>
>>>
>>> Huh, the erase/trim/discard code in the mmc core is really hairy. :-)
>>>
>>> In mmc_calc_max_discard() the following code/comment exists:
>>>
>>> /*
>>>  * Without erase_group_def set, MMC erase timeout depends on clock
>>>  * frequence which can change.  In that case, the best choice is
>>>  * just the preferred erase size.
>>>  */
>>> if (mmc_card_mmc(card) && !(card->ext_csd.erase_group_def & 1))
>>>      return card->pref_erase;
>>>
>>>
>>> This makes me wonder.
>>>
>>> So, when we haven't enabled the high capacity erase groups in the
>>> EXT_CSD register (ext_csd.erase_group_def), we will use the pref_erase
>>> size.
>>>
>>> In the other case, as when having MMC_CAP2_HC_ERASE_SZ set (which will
>>> set ext_csd.erase_group_def),  we will instead  do some calculations
>>> to find out the max discards.
>>>
>>> Are you saying that these calculations doesn't matter much - or are
>>> you saying that we always want to do them?
>>
>>
>> No, I was saying that if you have TRIM then TRIM is preferred to ERASE so
>> the erase group size does not come into play when discarding, since ERASE
>> granularity is erase groups whereas TRIM granularity is sectors.
> 
> Right. Thanks for clarifying.
> 
>>
>> However ERASE_GROUP_DEF also affects the size of write protect groups.
> 
> In either case.
> 
> What do you think of removing MMC_CAP2_HC_ERASE_SZ? I don't like these
> kind of soft polices, it's better if we can decide on a common
> behaviour - whatever that is.

Changing the value of ERASE_GROUP_DEF could be a problem, for example the
spec. says:

"Similarly if the host set ERASE_GROUP_DEF bit for a device that the default
write protect was already set in some of the area in the previous power
cycle, then the device may show unknown behavior when host issue write
or erase commands to the device. In application, it is mandatory for host to
use same ERASE GROUP DEF value to the device all the time."

Whether or not there is anyone that would actually be affected is hard to know.