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

[Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>]

On 11/28/13 15:06, Adrian Hunter wrote:
> 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.

Now the idea is clear, thank you.

>> 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
>>>>
>>>>
>>>
>>
>>
>
> --
> 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