Re: [PATCH] mmc: sdhci: don't limit discard timeout by data line timeout

[Adrian Hunter <adrian.hunter@intel.com>]

On 28/11/13 13:48, Vladimir Zapolskiy wrote:
> On 11/28/13 09:12, Adrian Hunter wrote:
>> On 27/11/13 16:57, Vladimir Zapolskiy wrote:
>>> On 11/27/13 10:21, Adrian Hunter wrote:
>>>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>>>> On 22.11.2013 14:04, Adrian Hunter wrote:
>>>>>>>>>> On 22/11/13 14:24, Vladimir Zapolskiy wrote:
>>>>>>>>>>> On 22.11.2013 12:38, Adrian Hunter wrote:
>>>>>>>>>>>> On 21/11/13 17:07, Vladimir Zapolskiy wrote:
>>>>>>>>>>>>> JEDEC specification defines quite high erase timeout value for
>>>>>>>>>>>>> 300ms
>>>>>>>>>>>>> multiplied by erase group number, and SD Host Controller
>>>>>>>>>>>>> specification
>>>>>>>>>>>>> data line timeout may be much less, e.g. 2^13 / 52MHz ~ 160us.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>          From block layer and MMC perfromance perspective it is
>>>>>>>>>>>>>> desirable
>>>>>>>>>>>>>> that
>>>>>>>>>>>>> millions of erase groups are discarded at once, so there is no
>>>>>>>>>>>>> much
>>>>>>>>>>>>> sense to limit maximum erase timeout by data line timeout, if a
>>>>>>>>>>>>> controller handles correctly erase operation without indication of
>>>>>>>>>>>>> data line timeout.
>>>>>>>>>>>>
>>>>>>>>>>>> Would you explain that some more.  Do you mean that:
>>>>>>>>>>>>          a) it does not have a timeout
>>>>>>>>>>>
>>>>>>>>>>> JEDEC defines a timeout on erase/trim operations, also in
>>>>>>>>>>> drivers/mmc/core/core.c
>>>>>>>>>>> there is a reasonable enough 10 minutes limitation for discard
>>>>>>>>>>> operations.
>>>>>>>>>>>
>>>>>>>>>>>>          b) it has a timeout which is less than the timeout
>>>>>>>>>>>> specified
>>>>>>>>>>>> by the
>>>>>>>>>>>> standard but the operation nevertheless completes
>>>>>>>>>>>
>>>>>>>>>>> SDHC data line timeout is enormously less than erase group timeout,
>>>>>>>>>>> and
>>>>>>>>>>> trivial testing shows that those two timeouts are independent,
>>>>>>>>>>> probably
>>>>>>>>>>> except some particular cases of controllers not known before commits
>>>>>>>>>>> 58d1246db3 and e056a1b5b.
>>>>>>>>>>>
>>>>>>>>>>> According to the currently implemented logic, mmc_do_erase()
>>>>>>>>>>> commonly is
>>>>>>>>>>> instructed to discard 1-2 erase groups at maximum, however it tends
>>>>>>>>>>> to be
>>>>>>>>>>> capable to successfully discard millions of erase groups at once
>>>>>>>>>>> ignoring
>>>>>>>>>>> that SDHC data line timeout limitation.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> You seem to be trying to say that the SDHCI spec. says that the host
>>>>>>>>>> controller does not timeout erase operations or uses a different
>>>>>>>>>> timeout
>>>>>>>>>> than the one programmed in the "Timeout Control Register".  Where is
>>>>>>>>>> that is
>>>>>>>>>> the SDHCI spec?
>>>>>>>>>
>>>>>>>>> According to the spec a host controller timeouts erase operations
>>>>>>>>> like any
>>>>>>>>> other R1B command.
>>>>>>>>>
>>>>>>>>> So in your opinion, should there be SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>>>>>> instead
>>>>>>>>> of the new quirk?
>>>>>>>>
>>>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It
>>>>>>>> just
>>>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>>>>
>>>>>>> Here I meant to do something like:
>>>>>>>
>>>>>>>        if (host->quirks&    SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>>>            mmc->max_discard_to = 0;
>>>>>>>
>>>>>>> Again I'm not sure that this applies well to all
>>>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>>>> controllers, therefore a new quirk might be better.
>>>>>>>
>>>>>>>> As I understand it you don't want to limit the discard size, either
>>>>>>>> because
>>>>>>>> your controller does not timeout, or because you are happy that the
>>>>>>>> maximum
>>>>>>>> timeout is enough for your users and their use-cases.
>>>>>>>>
>>>>>>>> If that is the case then the original patch just needs the quirk the
>>>>>>>> other
>>>>>>>> way around. i.e.
>>>>>>>>
>>>>>>>>         if (host->quirks2&     SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>>>             mmc->max_discard_to = 0;
>>>>>>>>         else
>>>>>>>>             mmc->max_discard_to = (1<<     27) / host->timeout_clk;
>>>>>>>
>>>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>>>> this.
>>>>>>>
>>>>>>> Also I'd like to pay your attention to (1<<    27) / host->timeout_clk
>>>>>>> part of
>>>>>>> calculation, following the spec it might be better to account the actual
>>>>>>> value of Data Timeout Counter, otherwise a controller may get
>>>>>>> unintentional
>>>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>>>>
>>>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>>>> erase operations to ones that have a timeout that is less than or
>>>>>> equal to
>>>>>> that.
>>>>>
>>>>> That's clear. But it's not obvious why do you prefer (1<<   27) numerator
>>>>> instead
>>>>> of secure (1<<   13) or (1<<   (13 + sdhci_readl(host,
>>>>> SDHCI_TIMEOUT_CONTROL))).
>>>>
>>>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>>>> Register" is 14 and the maximum timeout is therefore (1<<   27).
>>>
>>> So, from this perspective I assume this is a potential theoretical maximum
>>> timeout for a controller, which may be 16384 times more than the maximum
>>> guaranteed timeout before getting a DAT timeout. Why is the theoretical
>>> maximum
>>
>> Where do you get the notion of "maximum guaranteed timeout"?  The timeout is
>> what is programmed in "Data Timeout Counter Value".
> 
> And exactly this "Data Timeout Counter Value" is not used in your code to
> predict controller's data line timeout.
> 
>>> supposed to be used in calculations of a guaranteed discard operation
>>> timeout
>>> instead of promised DAT timeout by a controller?
>>
>> What is "promised DAT timeout"?
> 
> This is a timeout with respect to "Data Timeout Counter Value".
> 
> According to your words max_discard_to is the maximum timeout that the host
> controller supports, but such a parameter is useless, because nobody sets
> the host controller SDHCI_TIMEOUT_CONTROL register to maximum supported value,

sdhci_prepare_data() -> sdhci_calc_timeout() sets the timeout based on what
the upper layers specify, up to and including the maximum value.

So what max_discard_to does is to limit the erase size so that when
sdhci_calc_timeout() is called it won't exceed the maximum.

> so there is a probability that you greatly overestimate Data Timeout value,
> and therefore block layer or other subsystem can't rely on it. Please correct
> me here.
> 
>>>
>>>>>
>>>>>> Currently, the limit gets applied by the block layer before the mmc
>>>>>> layer is
>>>>>> involved so there is no possibility to take the actual timeout into
>>>>>> account.
>>>>>>     However if you have erase_group_def set, then it won't make any
>>>>>> difference
>>>>>> i.e. the limit will be the same.
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Potentially the change may break some of the SDHCs on discard of
>>>>>>>>>>>>> mmc,
>>>>>>>>>>>>> and for backward compatibility a new quirk is introduced, which
>>>>>>>>>>>>> is NOT
>>>>>>>>>>>>> set by default.
>>>>>>>>>>>>
>>>>>>>>>>>> It sounds to me that what you want to do is not standard so the
>>>>>>>>>>>> quirk
>>>>>>>>>>>> should
>>>>>>>>>>>> be the other way around.
>>>>>>>>>>>
>>>>>>>>>>> Please take a look at commits 58d1246db3 and e056a1b5b, I'd be
>>>>>>>>>>> glad, if
>>>>>>>>>>> you
>>>>>>>>>>> could elaborate to which "some host controllers" the quirk in my
>>>>>>>>>>> definition
>>>>>>>>>>> applies, I believe all other host controllers present at that
>>>>>>>>>>> time in
>>>>>>>>>>> drivers/mmc/host/* are capable to discard without introduced
>>>>>>>>>>> limitation.
>>>>>>>>>>
>>>>>>>>>> "some host controllers" == SDHCI i.e. to all of the ones you are
>>>>>>>>>> applying
>>>>>>>>>> the change.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Vladimir Zapolskiy<vladimir_zapolskiy@mentor.com>
>>>>>>>>>>>>> Reported-by: Ed Sutter<ed.sutter@alcatel-lucent.com>
>>>>>>>>>>>>> Cc: Chris Ball<cjb@laptop.org>
>>>>>>>>>>>>> Cc: Adrian Hunter<adrian.hunter@intel.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>        drivers/mmc/host/sdhci.c  |    5 ++++-
>>>>>>>>>>>>>        include/linux/mmc/sdhci.h |    1 +
>>>>>>>>>>>>>        2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>>>>>>>>>> index bd8a098..b1fdddb 100644
>>>>>>>>>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>>>>>>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>>>>>>>>>> @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>>>>>>>>>>            if (host->quirks&
>>>>>>>>>>>>> SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>>>>>>>>>>>>>                host->timeout_clk = mmc->f_max / 1000;
>>>>>>>>>>>>>
>>>>>>>>>>>>> -    mmc->max_discard_to = (1<<       27) / host->timeout_clk;
>>>>>>>>>>>>> +    if (host->quirks2&      
>>>>>>>>>>>>> SDHCI_QUIRK2_DATA_TIMEOUT_ON_DISCARD)
>>>>>>>>>>>>> +        mmc->max_discard_to = (1<<       27) / host->timeout_clk;
>>>>>>>>>>>>> +    else
>>>>>>>>>>>>> +        mmc->max_discard_to = 0;
>>>>>>>>>>>>>
>>>>>>>>>>>>>            mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE |
>>>>>>>>>>>>> MMC_CAP_CMD23;
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>>>>>>>>>>>>> index 3e781b8..e7f6bd2 100644
>>>>>>>>>>>>> --- a/include/linux/mmc/sdhci.h
>>>>>>>>>>>>> +++ b/include/linux/mmc/sdhci.h
>>>>>>>>>>>>> @@ -98,6 +98,7 @@ struct sdhci_host {
>>>>>>>>>>>>>        #define SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON        (1<<4)
>>>>>>>>>>>>>        /* Controller has a non-standard host control register */
>>>>>>>>>>>>>        #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL        (1<<5)
>>>>>>>>>>>>> +#define SDHCI_QUIRK2_DATA_TIMEOUT_ON_DISCARD        (1<<6)
>>>>>>>>>>>>>
>>>>>>>>>>>>>            int irq;        /* Device IRQ */
>>>>>>>>>>>>>            void __iomem *ioaddr;    /* Mapped address */
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>>
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
> 
>