[U-Boot] [PATCH 1/2] mmc: dw_mmc: Increase timeout to 20 seconds

[Pantelis Antoniou]

Hi Tom,

> On Sep 18, 2015, at 22:27 , Tom Rini <trini at konsulko.com> wrote:
> 
> On Fri, Sep 18, 2015 at 09:32:47AM +0200, Lukasz Majewski wrote:
>> Hi Tom,
>> 
>>> On Thu, Sep 17, 2015 at 04:43:33PM +0200, Lukasz Majewski wrote:
>>> 
>>>> Hi Tom,
>>>> 
>>>>> On Monday, September 14, 2015 at 01:22:20 PM, Lukasz Majewski
>>>>> wrote:
>>>>>> Hi Alexey,
>>>>>> 
>>>>>>> Hi Marek, Lukasz,
>>>>>>> 
>>>>>>> On Sun, 2015-09-13 at 16:00 +0200, Marek Vasut wrote:
>>>>>>>> On Sunday, September 13, 2015 at 12:03:18 PM, Lukasz
>>>>>>>> Majewski wrote:
>>>>>>>>> Hi Marek,
>>>>>>>> 
>>>>>>>> Hi,
>>>>>>>> 
>>>>>>>> [...]
>>>>>>>> 
>>>>>>>>>>>> Still we need to fix regression first with virtually
>>>>>>>>>>>> infinite timeout :) I would even thing that simple
>>>>>>>>>>>> revert of Marek's patch may make sense for now.
>>>>>>>>>>> 
>>>>>>>>>>> +1 - unfortunately there were some other patches
>>>>>>>>>>> applied to this particular patch. Simple revert might
>>>>>>>>>>> be a bit tricky here.
>>>>>>>>>> 
>>>>>>>>>> -1 - In case the card gets removed during the DMA
>>>>>>>>>> transfer and the board doesn't have a watchdog, it will
>>>>>>>>>> get stuck indefinitelly.
>>>>>>>>> 
>>>>>>>>> I'm just wondering here - why the indefinite loop was
>>>>>>>>> working previously? Was anybody complaining (on the ML)
>>>>>>>>> about the problem of removing the SD card when some
>>>>>>>>> operation is ongoing?
>>>>>>>> 
>>>>>>>> It worked for me for all the workloads I used. Noone was
>>>>>>>> complaining.
>>>>>>> 
>>>>>>> The same story here - previous code with infinite loop was
>>>>>>> working for my boards. And now I do see a problem with pretty
>>>>>>> simple scenario that we do use in our products.
>>>>>>> 
>>>>>>>>> The problem with potential removal of SD card (after
>>>>>>>>> booting the board) is with us for quite long time. Even
>>>>>>>>> with indefinite loop (without your patch) we also could
>>>>>>>>> "hang" the board if the SD card was removed during a
>>>>>>>>> transfer.
>>>>>>>> 
>>>>>>>> Which is why we should weed out the unbounded loops.
>>>>>>>> 
>>>>>>>>>> We
>>>>>>>>>> absolutelly don't want this sort of behavior in U-Boot.
>>>>>>>>>> I understand that this is the easiest way for everyone
>>>>>>>>>> to achieve some sort of "working" solution, but it is
>>>>>>>>>> definitelly not the correct one. While I do agree to
>>>>>>>>>> increasing the timeout, I do not agree to unbounded
>>>>>>>>>> loops, sorry.
>>>>>>>>> 
>>>>>>>>> We have agreed to not agree :-)
>>>>>>>> 
>>>>>>>> Yes :-)
>>>>>>> 
>>>>>>> The first thing I care is working U-Boot v2015.10 out of the
>>>>>>> box on my boards. And so I may agree on any temporary
>>>>>>> solution. I see it as timeout value either being infinite or
>>>>>>> obviously very high like 60 seconds.
>>>>>>> 
>>>>>>> 60 seconds might sound stupid but my thought behind this is to
>>>>>>> make sure even long transfers succeed. Imagine 100 Mb rootfs
>>>>>>> or update file downloaded from slow SD-card.
>>>>>> 
>>>>>> Transfer of rootfs to SD-card (downloaded to memory via tftp) is
>>>>>> definitely valid scenario.
>>>>>> 
>>>>>>>>>>>> From both points of view for keeping history
>>>>>>>>>>>> clean (compared to stacked fixes/workarounds) and
>>>>>>>>>>>> from removal of regression root cause.
>>>>>>>>>>> 
>>>>>>>>>>> As I said before - +1 from me.
>>>>>>>>>> 
>>>>>>>>>> As I said before, -1 from me. Btw. did anything regress
>>>>>>>>>> in here? To me, this seems like a newly discovered
>>>>>>>>>> bug ...
>>>>>>>>> 
>>>>>>>>> Yes, this is a bug. We had similar problem with Samsung's
>>>>>>>>> SDHCI, before we switched to dw_mmc. This issue is new at
>>>>>>>>> dw_mmc.
>>>>>>>>> 
>>>>>>>>>>>> It's not that I like to have infinite loops but
>>>>>>>>>>>> given previous implementation worked fine for
>>>>>>>>>>>> people in the previous U-Boot release.
>>>>>>>>>>> 
>>>>>>>>>>> Good justification
>>>>>>>>>> 
>>>>>>>>>> It is never a justified to return to a potentially
>>>>>>>>>> problematic version
>>>>>>>>> 
>>>>>>>>> IMHO revering the change (before the release) is from the
>>>>>>>>> software development point of view better solution than
>>>>>>>>> adding some heuristic delta to timeout.
>>>>>>>>> 
>>>>>>>>>> for the sake of getting some sort of crappy hardware
>>>>>>>>>> operational.
>>>>>>>>> 
>>>>>>>>> Unfortunately this "crappy hardware" is pervasive and we
>>>>>>>>> cannot do anything about it.
>>>>>>>>> 
>>>>>>>>> To sum up (my point of view):
>>>>>>>>> 1. The best would be to revert the patch - but if simple
>>>>>>>>> "git revert" is not working then,
>>>>>>> 
>>>>>>> Well even if clean revert won't work we may do manual tweaks
>>>>>>> so that functionally it is "revert". If of any interest I may
>>>>>>> come up with that sort of patch.
>>>>>>> 
>>>>>>>>> 2. We should increase the timeout (with my patch) for
>>>>>>>>> v2015.10 release
>>>>>>> 
>>>>>>> If everybody is OK with that let's go do it. Because release
>>>>>>> is around the corner and I don't want to explain each and
>>>>>>> every user how to fix their new problem.
>>>>>> 
>>>>>> v2015.10 correctness is crucial here.
>>> 
>>> Yes.
>>> 
>>>>>>>> Let's do this for the sake of crappy cards.
>>>>>>>> 
>>>>>>>>> 3. After release we can devise some kind of solution
>>>>>>>>> 4. It is a good topic for U-boot's minisummit discussion
>>>>>>>>> at Dublin
>>>>>>>>> 
>>>>>>>>> Marek, Alexey, Tom, Pantelis what do you think?
>>>>>>>> 
>>>>>>>> I think yes.
>>>>>>> 
>>>>>>> What's important we need to make sure Tom is aware of this
>>>>>>> situation and he won't cut a release until our fix is in place
>>>>>>> and all involved parties reported their happiness.
>>>>>> 
>>>> 
>>>> 
>>>>>> I also think that Tom should speak up on this issue.
>>>> 
>>>> Tom, could you give your opinion on this issue?
>>> 
>>> Well, is there a concensus patch now?
>>> 
>> 
>> There isn't a consensus patch.
>> 
>> There are two options:
>> 1. Try to revert changes (which remove endless loop)
>> 
>> 2. Increase the delay to e.g. 4 minutes (as it is done in Linux) before
>> v2015.10 and provide correct solution (based on internal SD card
>> information) after the release.
> 
> OK, lets go with #2.
> 

Send me a patch with the delay to 4mins please (blergh).

> -- 
> Tom

Regards

— Pantelis