mmc: dw_mmc: Increase timeout to 20 seconds

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 77b87e0..21a92d2 100644
@@ -213,7 +213,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct
 mmc_cmd *cmd, 
 	if (data) {
 		start = get_timer(0);
-		timeout = 1000;
+		timeout = 20000;
 		for (;;) {
 			mask = dwmci_readl(host, DWMCI_RINTSTS);
 			/* Error during data transfer. */

Hi,

Quoted 12 lines

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Quoted 4 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Are there any more questions regarding this patch or is it ready for submission as fix for v2015.10?

Quoted 14 lines

Hi,

Quoted 12 lines

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Are there any more questions regarding this patch or is it ready for submission as fix for v2015.10?

No comments, just apply this.

But this should really be fixed properly in the next MW.

Best regards,
Marek Vasut

Hi Marek, Lukasz,

Quoted 28 lines

Quoted 14 lines · Lukasz Majewski

Hi,

Quoted 12 lines

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Are there any more questions regarding this patch or is it ready for submission as fix for v2015.10?

No comments, just apply this.

But this should really be fixed properly in the next MW.

Quoted 2 lines

Best regards,
Marek Vasut

FWIW I faced similar problem even reading data.
At least on one of my boards reading of ~8Mb file took ~1.7 seconds and so 1 second timeout was interrupting data exchange.

So indeed we need to have some dirty hack for upcoming release like bumping timeout to something really huge but later we need to fix that problem properly.

I though proper solution would be to set timeout depending of amount of data to be exchanged... something like take slowest speed of SD/MMC that is allowed by spec and calculate delay based on how much time it might take for that slow device and for safety multiply it by say 2.

Now from this thread I see that there're other reasons that might affect length of at least write operation. In other words it could be complicated unfortunately.

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.

From both points of view for keeping history
clean (compared to stacked fixes/workarounds) and from removal of regression root cause.

It's not that I like to have infinite loops but given previous implementation worked fine for people in the previous U-Boot release.

-Alexey
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Alexey,

Quoted 31 lines

Hi Marek, Lukasz,

Quoted 29 lines

On Wednesday, September 09, 2015 at 09:01:30 AM, Lukasz Majewski

Quoted 14 lines · wrote:

Hi,

Quoted 12 lines

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Are there any more questions regarding this patch or is it ready for submission as fix for v2015.10?

No comments, just apply this.

But this should really be fixed properly in the next MW.

Quoted 7 lines

Quoted 2 lines

Best regards,
Marek Vasut

FWIW I faced similar problem even reading data.
At least on one of my boards reading of ~8Mb file took ~1.7 seconds and so 1 second timeout was interrupting data exchange.

Was it SD card or eMMC device?

Quoted 4 lines

So indeed we need to have some dirty hack for upcoming release like bumping timeout to something really huge but later we need to fix that problem properly.

Quoted 4 lines

I though proper solution would be to set timeout depending of amount of data to be exchanged... something like take slowest speed of SD/MMC that is allowed by spec and calculate delay based on how much time it might take for that slow device and for safety multiply it by say 2.

As fair as I remember, card provide this kind of information. We can try to investigate this possibility.

Quoted 3 lines

Now from this thread I see that there're other reasons that might affect length of at least write operation. In other words it could be complicated unfortunately.

My gut feeling is that proper handling of eMMC would require quite a fair mmc subsystem rework.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 1 line

Hi Alexey,

Hi,

Quoted 45 lines

Quoted 31 lines

Hi Marek, Lukasz,

Quoted 29 lines

On Wednesday, September 09, 2015 at 09:01:30 AM, Lukasz Majewski

Quoted 14 lines · wrote:

Hi,

Quoted 12 lines

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Are there any more questions regarding this patch or is it ready for submission as fix for v2015.10?

No comments, just apply this.

But this should really be fixed properly in the next MW.

Quoted 7 lines

Quoted 2 lines

Best regards,
Marek Vasut

FWIW I faced similar problem even reading data.
At least on one of my boards reading of ~8Mb file took ~1.7 seconds and so 1 second timeout was interrupting data exchange.

Was it SD card or eMMC device?

Quoted 4 lines

So indeed we need to have some dirty hack for upcoming release like bumping timeout to something really huge but later we need to fix that problem properly.

Quoted 8 lines

Quoted 5 lines

I though proper solution would be to set timeout depending of amount of data to be exchanged... something like take slowest speed of SD/MMC that is allowed by spec and calculate delay based on how much time it might take for that slow device and for safety multiply it by say 2.

As fair as I remember, card provide this kind of information. We can try to investigate this possibility.

Please do.

Quoted 6 lines

Quoted 3 lines

Now from this thread I see that there're other reasons that might affect length of at least write operation. In other words it could be complicated unfortunately.

My gut feeling is that proper handling of eMMC would require quite a fair mmc subsystem rework.

Can you please elaborate ?

Quoted 7 lines

Quoted 4 lines

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.

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.

Quoted 6 lines

Quoted 4 lines

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

Quoted 6 lines

Quoted 4 lines

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

for the sake of getting some sort of crappy hardware operational.

Best regards,
Marek Vasut

Hi Marek,

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Now from this thread I see that there're other reasons that might affect length of at least write operation. In other words it could be complicated unfortunately.

Quoted 4 lines

Quoted 2 lines

My gut feeling is that proper handling of eMMC would require quite a fair mmc subsystem rework.

Can you please elaborate ?

This is only my personal guess (when taking account my previous work done with dw_mmc on Exynos HW)
- I think Pantelis would have more knowledge to elaborate here.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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

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.

Quoted 6 lines

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

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 1 line

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,
2. We should increase the timeout (with my patch) for v2015.10 release

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?

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 24 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 1 line

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,

Quoted 1 line

2. We should increase the timeout (with my patch) for v2015.10 release

Let's do this for the sake of crappy cards.

Quoted 4 lines

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.

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 29 lines

Quoted 29 lines

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 2 lines

Quoted 2 lines

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.

Quoted 6 lines

Let's do this for the sake of crappy cards.

Quoted 3 lines

3. After release we can devise some kind of solution
4. It is a good topic for U-boot's minisummit discussion at Dublin

Quoted 3 lines

Quoted 1 line

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.

-Alexey

Hi Alexey,

Quoted 51 lines

Hi Marek, Lukasz,

Quoted 19 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 39 lines

Quoted 28 lines

Quoted 28 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 7 lines

Let's do this for the sake of crappy cards.

Quoted 4 lines

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?

Quoted 5 lines

Quoted 1 line

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.

Quoted 127 lines

Hi Alexey,

Quoted 54 lines

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 15 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 40 lines

Quoted 29 lines

Quoted 29 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 10 lines

Quoted 9 lines

Let's do this for the sake of crappy cards.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

btw. I think I know why SoCFPGA never triggered this issue, it has CONFIG_SYS_MMC_MAX_BLK_COUNT set to 256 so a large transfer was always broken to smaller chunks.

Hi Tom,

Quoted 117 lines · Lukasz Majewski

Quoted 116 lines

Hi Alexey,

Quoted 54 lines

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 15 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 40 lines

Quoted 29 lines

Quoted 29 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Let's do this for the sake of crappy cards.

Quoted 5 lines

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.

Quoted 1 line

Quoted 1 line

I also think that Tom should speak up on this issue.

Tom, could you give your opinion on this issue?

Quoted 123 lines

Hi Tom,

Quoted 120 lines

Quoted 118 lines · Lukasz Majewski

Hi Alexey,

Quoted 55 lines

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 16 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 41 lines

Quoted 30 lines

Quoted 30 lines

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 16 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Let's do this for the sake of crappy cards.

Quoted 5 lines

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.

Quoted 3 lines

Quoted 1 line

Quoted 1 line

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?

Hi Tom,

Quoted 143 lines · Lukasz Majewski

Quoted 123 lines

Hi Tom,

Quoted 120 lines

Quoted 118 lines · Lukasz Majewski

Hi Alexey,

Quoted 55 lines

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 16 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 41 lines

Quoted 30 lines

Quoted 30 lines

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 16 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Let's do this for the sake of crappy cards.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

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.

Hi Tom,

Quoted 143 lines · Lukasz Majewski

Quoted 123 lines

Hi Tom,

Quoted 120 lines

Quoted 118 lines · Lukasz Majewski

Hi Alexey,

Quoted 55 lines

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 16 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 41 lines

Quoted 30 lines

Quoted 30 lines

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 16 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Let's do this for the sake of crappy cards.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

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.

Quoted 154 lines · Lukasz Majewski

Hi Tom,

Quoted 143 lines · Lukasz Majewski

Quoted 123 lines

Hi Tom,

Quoted 120 lines

Quoted 118 lines · Lukasz Majewski

Hi Alexey,

Quoted 55 lines

Hi Marek, Lukasz,

Quoted 20 lines · Marek Vasut

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 16 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

We have agreed to not agree :-)

Yes :-)

Quoted 8 lines

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.

Quoted 41 lines

Quoted 30 lines

Quoted 30 lines

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

for the sake of getting some sort of crappy hardware operational.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 16 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Let's do this for the sake of crappy cards.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

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.

Quoted 6 lines

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.

Signature · 1 line

Tom

Hi Alexey,

Quoted 4 lines

FWIW I faced similar problem even reading data.
At least on one of my boards reading of ~8Mb file took ~1.7 seconds and so 1 second timeout was interrupting data exchange.

Was it SD card or eMMC device?

Quoted 5 lines

So indeed we need to have some dirty hack for upcoming release like bumping timeout to something really huge but later we need to fix that problem properly.

Quoted 4 lines

I though proper solution would be to set timeout depending of amount of data to be exchanged... something like take slowest speed of SD/MMC that is allowed by spec and calculate delay based on how much time it might take for that slow device and for safety multiply it by say 2.

As fair as I remember, card provide this kind of information. We can try to investigate this possibility.

Quoted 4 lines

Now from this thread I see that there're other reasons that might affect length of at least write operation. In other words it could be complicated unfortunately.

Quoted 8 lines · Lukasz Majewski

Hi Alexey,

Quoted 4 lines

FWIW I faced similar problem even reading data.
At least on one of my boards reading of ~8Mb file took ~1.7 seconds and so 1 second timeout was interrupting data exchange.

Was it SD card or eMMC device?

It was external SD-card.

Quoted 5 lines

Quoted 5 lines

So indeed we need to have some dirty hack for upcoming release like bumping timeout to something really huge but later we need to fix that problem properly.

Quoted 7 lines

Quoted 4 lines

I though proper solution would be to set timeout depending of amount of data to be exchanged... something like take slowest speed of SD/MMC that is allowed by spec and calculate delay based on how much time it might take for that slow device and for safety multiply it by say 2.

As fair as I remember, card provide this kind of information. We can try to investigate this possibility.

I'm pretty sure card may report a lot of info about itself. And if we look at what people do in Linux kernel we may see calculations of different timeouts in "drivers/mmc/core/core.c".

But keeping in mind we're not writing another OS kernel but we're just dealing with bootloader I'd prefer to keep things simple and do check only critical things like totally broken HW. I.e. something simple should be enough for U-Boot.

Quoted 4 lines

Quoted 4 lines

Now from this thread I see that there're other reasons that might affect length of at least write operation. In other words it could be complicated unfortunately.

-Alexey

Hi,

Quoted 8 lines

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

Quoted 8 lines

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Are there any more questions regarding this patch or is it ready for submission as fix for v2015.10?

Quoted 4 lines

---
  drivers/mmc/dw_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c static int dwmci_send_cmd(struct mmc *mmc, struct · line 219

Quoted 13 lines

drivers/mmc/dw_mmc.c +1 -1

index 77b87e0..21a92d2 100644
@@ -213,7 +213,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct
 mmc_cmd *cmd,
  	if (data) {
  		start = get_timer(0);
-		timeout = 1000;
+		timeout = 20000;
  		for (;;) {
  			mask = dwmci_readl(host, DWMCI_RINTSTS);
  			/* Error during data transfer. */

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90
"mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 77b87e0..21a92d2 100644
@@ -213,7 +213,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct
 mmc_cmd *cmd,
 
 	if (data) {
 		start = get_timer(0);
-		timeout = 1000;
+		timeout = 20000;
 		for (;;) {
 			mask = dwmci_readl(host, DWMCI_RINTSTS);

/* Error during data transfer. */

Quoted 8 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

Quoted 3 lines

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Also, where did you find out there is such "cleanup" mechanism please ?

Quoted 16 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +0 -0

index 77b87e0..21a92d2 100644
@@ -213,7 +213,7 @@ static int dwmci_send_cmd(struct mmc *mmc,
 struct mmc_cmd *cmd,
 

Quoted 7 lines

drivers/mmc/dw_mmc.c +1 -1

 	if (data) {
 		start = get_timer(0);
-		timeout = 1000;
+		timeout = 20000;
 		for (;;) {
 			mask = dwmci_readl(host, DWMCI_RINTSTS);

/* Error during data transfer. */

Best regards,
Marek Vasut

On Fri, 28 Aug 2015 23:55:17 +0200

Quoted 9 lines · Marek Vasut

Quoted 8 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

Quoted 6 lines

Quoted 3 lines

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Then we would wait 20 seconds :-) to proceed.

To be clear - the mentioned patch introduced regression.

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Please correct me if I'm wrong - but is seems like we are now using 1 second timeout for detection if SD card has been removed?

Shouldn't we use polling on the card detect IO pin instead? We could add such polling in several places in the MMC subsystem (like we do it with watchdog).

Marek, Pantelis, what do you think about this?

Quoted 2 lines

Also, where did you find out there is such "cleanup" mechanism please ?

Internally we did some tests with several SD cards. We were stunned when it turned out that for some workloads it took up to 15 seconds to end write operation for small data.

The culprit is the SD Card embedded controller responsible for FTL - flash translation layer.
It allows NAND memory on the card to be visible as the block device. More importantly it also takes care of wear leveling and bad block management.

Hence, we don't know when it would start housekeeping operations. We can only poll/wait until this controller finishes it work. The code as it was (with the indefinite loop) was taking this situation into account.

The 1 second timeout is apparently too short and makes using SD card non-deterministic and error prone in u-boot.

Even worse, many devices use SD card as the only storage device.

Quoted 1 line

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 17 lines

Quoted 15 lines · Marek Vasut

Quoted 11 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Quoted 1 line

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

Is this behavior
specific to Samsung SD cards ?

Quoted 2 lines

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version 4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

Quoted 8 lines

Please correct me if I'm wrong - but is seems like we are now using 1 second timeout for detection if SD card has been removed?

Shouldn't we use polling on the card detect IO pin instead? We could add such polling in several places in the MMC subsystem (like we do it with watchdog).

Quoted 1 line

Marek, Pantelis, what do you think about this?

If you implement board_mmc_getcd(), you can check if the card is present this way instead of waiting for command to time out. The infrastructure for that is already in place. Right ?

It'd be cool if the MMC subsystem could pull the wp-gpios and cd-gpios from DT though :)

Quoted 19 lines

Quoted 3 lines

Also, where did you find out there is such "cleanup" mechanism please ?

Internally we did some tests with several SD cards. We were stunned when it turned out that for some workloads it took up to 15 seconds to end write operation for small data.

The culprit is the SD Card embedded controller responsible for FTL - flash translation layer.
It allows NAND memory on the card to be visible as the block device. More importantly it also takes care of wear leveling and bad block management.

Hence, we don't know when it would start housekeeping operations. We can only poll/wait until this controller finishes it work. The code as it was (with the indefinite loop) was taking this situation into account.

Quoted 4 lines

The 1 second timeout is apparently too short and makes using SD card non-deterministic and error prone in u-boot.

Even worse, many devices use SD card as the only storage device.

Yes, horrible.

Best regards,
Marek Vasut

Hi Marek,

Quoted 22 lines · Lukasz Majewski

Quoted 1 line

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 17 lines

Quoted 15 lines · Marek Vasut

Quoted 11 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Quoted 6 lines

Quoted 1 line

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

I think that only the card manufacturer may reveal what can happen with the card (what policy have been implemented in their FW).

I guess that you may lost data in such case.

Quoted 2 lines

Is this behavior
specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata MicroSD HC (4GiB) cards.

Quoted 19 lines

Quoted 1 line

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Quoted 5 lines

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version 4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

I'll look into the spec and then comment :-).

For my boards the time was 1.2 seconds for storing 8 MiB file.

Quoted 7 lines

Quoted 7 lines

Please correct me if I'm wrong - but is seems like we are now using 1 second timeout for detection if SD card has been removed?

Shouldn't we use polling on the card detect IO pin instead? We could add such polling in several places in the MMC subsystem (like we do it with watchdog).

Quoted 5 lines

Quoted 1 line

Marek, Pantelis, what do you think about this?

If you implement board_mmc_getcd(), you can check if the card is present this way instead of waiting for command to time out. The infrastructure for that is already in place. Right ?

So you suggest adding board_mmc_getcd() in several places in the mmc subsystem driver to detect removal of the SD card?

Quoted 2 lines

It'd be cool if the MMC subsystem could pull the wp-gpios and cd-gpios from DT though :)

+1

Quoted 18 lines

Quoted 18 lines

Quoted 2 lines

Also, where did you find out there is such "cleanup" mechanism please ?

Internally we did some tests with several SD cards. We were stunned when it turned out that for some workloads it took up to 15 seconds to end write operation for small data.

The culprit is the SD Card embedded controller responsible for FTL - flash translation layer.
It allows NAND memory on the card to be visible as the block device. More importantly it also takes care of wear leveling and bad block management.

Hence, we don't know when it would start housekeeping operations. We can only poll/wait until this controller finishes it work. The code as it was (with the indefinite loop) was taking this situation into account.

Quoted 6 lines

Quoted 4 lines

The 1 second timeout is apparently too short and makes using SD card non-deterministic and error prone in u-boot.

Even worse, many devices use SD card as the only storage device.

Yes, horrible.

Good that we have agreed.

Quoted 1 line

Hi Marek,

Hi Lukasz,

Quoted 31 lines

Quoted 24 lines

Quoted 1 line · Lukasz Majewski

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 18 lines

Quoted 16 lines · Marek Vasut

Quoted 11 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Quoted 6 lines

Quoted 1 line

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

Quoted 4 lines

I think that only the card manufacturer may reveal what can happen with the card (what policy have been implemented in their FW).

I guess that you may lost data in such case.

Uuuurgh, that's seriously shitty.

Quoted 5 lines

Quoted 2 lines

Is this behavior
specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata MicroSD HC (4GiB) cards.

I had bad previous experience with Kingston too.

Quoted 24 lines

Quoted 24 lines

Quoted 1 line

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version
4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

Quoted 4 lines

I'll look into the spec and then comment :-).

For my boards the time was 1.2 seconds for storing 8 MiB file.

OK, but that's weird. The transfer should always be up to 512B long and not any longer, unless it's a multiblock transfer.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Please correct me if I'm wrong - but is seems like we are now using 1 second timeout for detection if SD card has been removed?

Shouldn't we use polling on the card detect IO pin instead? We could add such polling in several places in the MMC subsystem (like we do it with watchdog).

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Marek, Pantelis, what do you think about this?

If you implement board_mmc_getcd(), you can check if the card is present this way instead of waiting for command to time out. The infrastructure for that is already in place. Right ?

So you suggest adding board_mmc_getcd() in several places in the mmc subsystem driver to detect removal of the SD card?

Hmmmm, I'm not sure about this one. Panto ?

Quoted 23 lines

Quoted 2 lines

It'd be cool if the MMC subsystem could pull the wp-gpios and cd-gpios from DT though :)

+1

Quoted 18 lines

Quoted 18 lines

Quoted 2 lines

Also, where did you find out there is such "cleanup" mechanism please ?

Internally we did some tests with several SD cards. We were stunned when it turned out that for some workloads it took up to 15 seconds to end write operation for small data.

The culprit is the SD Card embedded controller responsible for FTL - flash translation layer.
It allows NAND memory on the card to be visible as the block device. More importantly it also takes care of wear leveling and bad block management.

Hence, we don't know when it would start housekeeping operations. We can only poll/wait until this controller finishes it work. The code as it was (with the indefinite loop) was taking this situation into account.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

The 1 second timeout is apparently too short and makes using SD card non-deterministic and error prone in u-boot.

Even worse, many devices use SD card as the only storage device.

Yes, horrible.

Good that we have agreed.

Heh :)

Best regards,
Marek Vasut

Hi Marek,

Quoted 45 lines · Lukasz Majewski

Quoted 1 line

Hi Marek,

Hi Lukasz,

Quoted 33 lines

Quoted 26 lines

On Saturday, August 29, 2015 at 01:55:36 PM, Lukasz Majewski

Quoted 1 line · wrote:

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 19 lines

Quoted 17 lines · Marek Vasut

On Friday, August 28, 2015 at 03:50:20 PM, Lukasz Majewski

Quoted 11 lines · wrote:

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Quoted 6 lines

Quoted 1 line

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

Quoted 4 lines

I think that only the card manufacturer may reveal what can happen with the card (what policy have been implemented in their FW).

I guess that you may lost data in such case.

Uuuurgh, that's seriously shitty.

Quoted 32 lines

Quoted 5 lines

Quoted 2 lines

Is this behavior
specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata MicroSD HC (4GiB) cards.

I had bad previous experience with Kingston too.

Quoted 24 lines

Quoted 24 lines

Quoted 1 line

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version
4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

The "timeout" error is for situation when you issue write command (either single or multiple block) and you don't receive any response from the card.

In our case we use multiblock transfer (CMD25) with either set number of block to transfer (CMD23) or explicit end of transmission (CMD12).

Let's consider the second case.

We setup data and issue CMD25. Then we check the CMD25 status (if we don't receive reply in 250 ms we would get timeout error). Afterwards data from buffer is transmitted to the card and flashed. This operation may take long time. During this process you can issue CMD13 (SEND_STATUS) to receive information about card state ([*] 4.10. - page 85).

Two notable fields of [*] to check are READY_FOR_DATA and CURRENT_STATE. Those two state what is the SD Card controller situation.

Then, you end the transfer with CMD12, which also provides some status information from the SDCard (like "prg"|"rcv", etc).

If you want to issue next command you must check READY_FOR_DATA and CURRENT_STATE. In the case of internal SD card controller operation you would not get READY_FOR_DATA until it ends.

Quoted 6 lines

Quoted 3 lines

I'll look into the spec and then comment :-).

For my boards the time was 1.2 seconds for storing 8 MiB file.

OK, but that's weird. The transfer should always be up to 512B long and not any longer, unless it's a multiblock transfer.

It is a multi block transfer.

Quoted 1 line

Hi Marek,

Hi!

Quoted 90 lines

Quoted 48 lines

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi Lukasz,

Quoted 34 lines

Quoted 26 lines

On Saturday, August 29, 2015 at 01:55:36 PM, Lukasz Majewski

Quoted 1 line · wrote:

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 19 lines

Quoted 17 lines · Marek Vasut

On Friday, August 28, 2015 at 03:50:20 PM, Lukasz Majewski

Quoted 11 lines · wrote:

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Quoted 7 lines

Quoted 1 line

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

Quoted 5 lines

I think that only the card manufacturer may reveal what can happen with the card (what policy have been implemented in their FW).

I guess that you may lost data in such case.

Uuuurgh, that's seriously shitty.

Quoted 33 lines

Quoted 5 lines

Quoted 2 lines

Is this behavior
specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata MicroSD HC (4GiB) cards.

I had bad previous experience with Kingston too.

Quoted 25 lines

Quoted 25 lines

Quoted 1 line

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version 4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

The "timeout" error is for situation when you issue write command (either single or multiple block) and you don't receive any response from the card.

In our case we use multiblock transfer (CMD25) with either set number of block to transfer (CMD23) or explicit end of transmission (CMD12).

Quoted 8 lines

Let's consider the second case.

We setup data and issue CMD25. Then we check the CMD25 status (if we don't receive reply in 250 ms we would get timeout error). Afterwards data from buffer is transmitted to the card and flashed. This operation may take long time. During this process you can issue CMD13 (SEND_STATUS) to receive information about card state ([*] 4.10. - page 85).

Maybe we should implement such polling through CMD13 then ? But, this should be a matter for next MW, this MW we should go with increasing the timeout to some 20 or 30 second. What do you think ?

Quoted 20 lines

Two notable fields of [*] to check are READY_FOR_DATA and CURRENT_STATE. Those two state what is the SD Card controller situation.

Then, you end the transfer with CMD12, which also provides some status information from the SDCard (like "prg"|"rcv", etc).

If you want to issue next command you must check READY_FOR_DATA and CURRENT_STATE. In the case of internal SD card controller operation you would not get READY_FOR_DATA until it ends.

Quoted 6 lines

Quoted 3 lines

I'll look into the spec and then comment :-).

For my boards the time was 1.2 seconds for storing 8 MiB file.

OK, but that's weird. The transfer should always be up to 512B long and not any longer, unless it's a multiblock transfer.

It is a multi block transfer.

[...]

Hi Marek,

Quoted 105 lines · Lukasz Majewski

Quoted 1 line

Hi Marek,

Hi!

Quoted 99 lines

Quoted 49 lines

On Saturday, August 29, 2015 at 06:38:48 PM, Lukasz Majewski

Quoted 1 line · wrote:

Hi Marek,

Hi Lukasz,

Quoted 34 lines

Quoted 26 lines

On Saturday, August 29, 2015 at 01:55:36 PM, Lukasz Majewski

Quoted 1 line · wrote:

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 19 lines

Quoted 17 lines · Marek Vasut

On Friday, August 28, 2015 at 03:50:20 PM, Lukasz Majewski

Quoted 11 lines · wrote:

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Quoted 7 lines

Quoted 1 line

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

Quoted 5 lines

I think that only the card manufacturer may reveal what can happen with the card (what policy have been implemented in their FW).

I guess that you may lost data in such case.

Uuuurgh, that's seriously shitty.

Quoted 33 lines

Quoted 5 lines

Quoted 2 lines

Is this behavior
specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata MicroSD HC (4GiB) cards.

I had bad previous experience with Kingston too.

Quoted 25 lines

Quoted 25 lines

Quoted 1 line

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version 4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

The "timeout" error is for situation when you issue write command (either single or multiple block) and you don't receive any response from the card.

In our case we use multiblock transfer (CMD25) with either set number of block to transfer (CMD23) or explicit end of transmission (CMD12).

Let's consider the second case.

We setup data and issue CMD25. Then we check the CMD25 status (if we don't receive reply in 250 ms we would get timeout error). Afterwards data from buffer is transmitted to the card and flashed. This operation may take long time. During this process you can issue CMD13 (SEND_STATUS) to receive information about card state ([*] 4.10. - page 85).

Quoted 3 lines

Maybe we should implement such polling through CMD13 then ? But, this should be a matter for next MW, this MW we should go with increasing the timeout to some 20 or 30 second. What do you think ?

For next release we can increase the timeout. Rewritting eMMC subsystem code is a challenging task and we will not finish until the next u-boot release.

Quoted 1 line · Lukasz Majewski

Hi Marek,

Hi Lukasz,

Quoted 29 lines

Quoted 28 lines · Lukasz Majewski

Quoted 1 line

On Fri, 28 Aug 2015 23:55:17 +0200

Hi!

Quoted 10 lines

Quoted 9 lines · Marek Vasut

Quoted 8 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90 "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

The new value - 20 seconds - takes into account the situation when SD card triggers internal clean up. Such process may take more than 10 seconds on some cards.

What happens if you pull the SD card out of the slot during such a process?

Then we would wait 20 seconds :-) to proceed.

Oops, I think I was not clear here. I was wondering what happens to the card if you yank it out of the slot whole it's performing it's internal cleanup or whatever. Is it possible that the card suffers data corruption, effectively trashing user data ?

Quoted 4 lines

I think that only the card manufacturer may reveal what can happen with the card (what policy have been implemented in their FW).

I guess that you may lost data in such case.

Uuuurgh, that's seriously shitty.

Quoted 5 lines

Quoted 2 lines

Is this behavior
specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata MicroSD HC (4GiB) cards.

I had bad previous experience with Kingston too.

Quoted 25 lines

Quoted 19 lines

Quoted 1 line

To be clear - the mentioned patch introduced regression.

That's a bug, not a regression, but anyway,
that's not the point. I do
agree with you that we do have a problem and I'm inclined to Ack this patch, I'd like to understand what the real implications of such a behavior of these cards are.

Quoted 10 lines

It works for
small files on a commonly available SD cards (like 4 GiB Kingston/Adata).

When I ran DFU tests I've discovered that there is a problem with storing 8MiB file (dat_8M.img).

Even worse - when one wants to store Image.itb file (which might be 4-6 MiB) it sometimes works and sometimes not. Nightmare for debugging.

Quoted 5 lines

Ew, that's one crappy card you have there. I'm reading the SD card "Physical Layer Simplified Specification Version 4.10" (part1_410.pdf) section 4.6.2.2 and it states that for SDHC cards, the write operation should take at most 250mS, for SDXC it's 500mS. Could it be that your card is violating the spec ?

Quoted 3 lines

I'll look into the spec and then comment :-).

For my boards the time was 1.2 seconds for storing 8 MiB file.

OK, but that's weird. The transfer should always be up to 512B long and not any longer, unless it's a multiblock transfer.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Please correct me if I'm wrong - but is seems like we are now using 1 second timeout for detection if SD card has been removed?

Shouldn't we use polling on the card detect IO pin instead? We could add such polling in several places in the MMC subsystem (like we do it with watchdog).

Marek, Pantelis, what do you think about this?

Quoted 6 lines

Quoted 3 lines

If you implement board_mmc_getcd(), you can check if the card is present this way instead of waiting for command to time out. The infrastructure for that is already in place. Right ?

So you suggest adding board_mmc_getcd() in several places in the mmc subsystem driver to detect removal of the SD card?

Hmmmm, I'm not sure about this one. Panto ?

Quoted 5 lines

Quoted 2 lines

It'd be cool if the MMC subsystem could pull the wp-gpios and cd-gpios from DT though :)

+1

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 2 lines

Also, where did you find out there is such "cleanup" mechanism please ?

Internally we did some tests with several SD cards. We were stunned when it turned out that for some workloads it took up to 15 seconds to end write operation for small data.

The culprit is the SD Card embedded controller responsible for FTL - flash translation layer.
It allows NAND memory on the card to be visible as the block device. More importantly it also takes care of wear leveling and bad block management.

Hence, we don't know when it would start housekeeping operations. We can only poll/wait until this controller finishes it work. The code as it was (with the indefinite loop) was taking this situation into account.

The 1 second timeout is apparently too short and makes using SD card non-deterministic and error prone in u-boot.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Even worse, many devices use SD card as the only storage device.

Yes, horrible.

Good that we have agreed.

Heh :)

Best regards,
Marek Vasut

Hi Marek,

Quoted 4 lines

Quoted 2 lines

So you suggest adding board_mmc_getcd() in several places in the mmc subsystem driver to detect removal of the SD card?

Hmmmm, I'm not sure about this one. Panto ?

I?m fine with this. Perhaps we can avoid spamming this all over the place, and that would be better.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

It'd be cool if the MMC subsystem could pull the wp-gpios and cd-gpios from DT though :)

+1

Indeed. I would expect this to be a per-board thing. I would not expect all platforms to provide that capability.

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 2 lines

Also, where did you find out there is such "cleanup" mechanism please ?

Internally we did some tests with several SD cards. We were stunned when it turned out that for some workloads it took up to 15 seconds to end write operation for small data.

The culprit is the SD Card embedded controller responsible for FTL - flash translation layer.
It allows NAND memory on the card to be visible as the block device. More importantly it also takes care of wear leveling and bad block management.

Hence, we don't know when it would start housekeeping operations. We can only poll/wait until this controller finishes it work. The code as it was (with the indefinite loop) was taking this situation into account.

The 1 second timeout is apparently too short and makes using SD card non-deterministic and error prone in u-boot.

Even worse, many devices use SD card as the only storage device.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Yes, horrible.

Good that we have agreed.

Heh :)

Horrible closed source flash management algorithm is horrible, film at 11 :)

The timeout error for DW MMC transfer should be visible on the u-boot console to speed up the process of debugging.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Tom Rini <[email protected]>

Cc Simon Glass <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Lukasz,

Quoted 4 lines · Lukasz Majewski

The timeout error for DW MMC transfer should be visible on the u-boot console to speed up the process of debugging.

Quoted 6 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Tom Rini <[email protected]>

Cc Simon Glass <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Can this be reported at the command line level instead?

It feels wrong
to be printing errors in a driver.

Also, why does it return TIMEOUT
instead of -ETIMEOUT?

Regards,
Simon

Quoted 8 lines · On Fri, 28 Aug 2015 17:21:51 -0600 Simon Glass <[email protected]> wrote:

Hi Lukasz,

Quoted 4 lines · Lukasz Majewski

The timeout error for DW MMC transfer should be visible on the u-boot console to speed up the process of debugging.

Quoted 8 lines

Quoted 6 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Tom Rini <[email protected]>

Cc Simon Glass <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Can this be reported at the command line level instead?

With my setup - dfu tests - I didn't receive any error/information about the MMC (SD Card to be precise) timeout.

(I would expect MMC subsystem to return -ETIMEOUT and then this error would be propagated to dfu and stop execution of the dfu command).

This didn't work and hence should be scrutinized.

Quoted 3 lines

It feels wrong
to be printing errors in a driver.

If this information would be printed on the console I might have noticed it immediately and save some time for debugging.

Quoted 2 lines

Also, why does it return TIMEOUT
instead of -ETIMEOUT?

Good question.

Hi Lukasz,

Quoted 20 lines · Lukasz Majewski

Quoted 8 lines · On Fri, 28 Aug 2015 17:21:51 -0600 Simon Glass <[email protected]> wrote:

Hi Lukasz,

Quoted 4 lines · Lukasz Majewski

The timeout error for DW MMC transfer should be visible on the u-boot console to speed up the process of debugging.

Quoted 8 lines

Quoted 6 lines

Signed-off-by Lukasz Majewski <[email protected]>

Cc Tom Rini <[email protected]>

Cc Simon Glass <[email protected]>

---
 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Can this be reported at the command line level instead?

Quoted 8 lines

With my setup - dfu tests - I didn't receive any error/information about the MMC (SD Card to be precise) timeout.

(I would expect MMC subsystem to return -ETIMEOUT and then this error would be propagated to dfu and stop execution of the dfu command).

This didn't work and hence should be scrutinized.

Agreed.

Quoted 10 lines

Quoted 3 lines

It feels wrong
to be printing errors in a driver.

If this information would be printed on the console I might have noticed it immediately and save some time for debugging.

Quoted 2 lines

Also, why does it return TIMEOUT
instead of -ETIMEOUT?

Quoted 1 line

Good question.

It looks likes mmc.h has its own errors (NO_CARD_ERR, etc.). I suspect these could be converted to use standard errors.

Anyway your question remains. A driver error should be reported by the caller.

Regards,
Simon

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

Quoted 19 lines

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Quoted 3 lines

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

Can you pick up Stephen's FAT replacement series and see if it also fixes this problem? Thanks!

Hi Tom,

Quoted 23 lines · Lukasz Majewski

Quoted 22 lines

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Quoted 5 lines

Quoted 1 line

Test HW: Odroid XU3 - Exynos 5433

Can you pick up Stephen's FAT replacement series and see if it also fixes this problem? Thanks!

Ok, I will test this fat implementation.

However, since for v2015.10 it won't be included, I would also opt for adding this fix to the current u-boot.

Hi Lukasz,

Quoted 31 lines

Hi Tom,

Quoted 28 lines · Lukasz Majewski

Quoted 15 lines

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

Quoted 8 lines

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

Can you pick up Stephen's FAT replacement series and see if it also fixes this problem? Thanks!

Quoted 1 line

Ok, I will test this fat implementation.

I've applied v2 of this patchset
on top of SHA1: 79c884d7e449a63fa8f07b7495f8f9873355c48f

Unfortunately, DFU tests fail with first attempt to pass the test.

Tegra124 (Jetson TK1) # load mmc 1:1 $loadaddr dfu1.bin reading dfu1.bin
1048576 bytes read in 95 ms (10.5 MiB/s)

Tegra124 (Jetson TK1) # load mmc 1:1 $loadaddr dfu1.bin 1048576 bytes read in 5038 ms (203.1 KiB/s)

Quoted 33 lines

Hi Lukasz,

Quoted 31 lines

Hi Tom,

Quoted 28 lines · Lukasz Majewski

Quoted 15 lines

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

Quoted 8 lines

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

Can you pick up Stephen's FAT replacement series and see if it also fixes this problem? Thanks!

Quoted 6 lines

Quoted 1 line

Ok, I will test this fat implementation.

I've applied v2 of this patchset
on top of SHA1: 79c884d7e449a63fa8f07b7495f8f9873355c48f

Unfortunately, DFU tests fail with first attempt to pass the test.

I've found a couple of problems.

First up, file_fat_write() wasn't truncating the file when writing, so the file size wasn't changing when over-writing a large file with a small file. With this fixed, I can run the DFU tests just fine for all the small files (<1M). I've fixed this locally and in the ff branch on my github.

Second, ff is slow:

Some random old build I had in flash on my system:

Quoted 3 lines

Tegra124 (Jetson TK1) # load mmc 1:1 $loadaddr dfu1.bin reading dfu1.bin
1048576 bytes read in 95 ms (10.5 MiB/s)

With my ff branch:

Quoted 2 lines

Tegra124 (Jetson TK1) # load mmc 1:1 $loadaddr dfu1.bin 1048576 bytes read in 5038 ms (203.1 KiB/s)

That's quite the slow-down! I believe this is causing dfu-util to time out on the larger files (1M+). Just for functional testing, I'll try and find a way to hack dfu-util to have a much larger timeout for the final flush operation. I wonder if the old FAT implementation had a disk cache (e.g. that 32K buffer in BSS?) and we need the same for ff?

I'll try and track down why it's so slow.

Perhaps there are other issues as yet unfound.

Hi Stephen,

Quoted 43 lines · Lukasz Majewski

Quoted 33 lines

Hi Lukasz,

Quoted 31 lines

Hi Tom,

Quoted 28 lines · Lukasz Majewski

Quoted 15 lines

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

Quoted 8 lines

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

Can you pick up Stephen's FAT replacement series and see if it also fixes this problem? Thanks!

Quoted 6 lines

Quoted 1 line

Ok, I will test this fat implementation.

I've applied v2 of this patchset
on top of SHA1: 79c884d7e449a63fa8f07b7495f8f9873355c48f

Unfortunately, DFU tests fail with first attempt to pass the test.

I've found a couple of problems.

Quoted 5 lines

First up, file_fat_write() wasn't truncating the file when writing, so the file size wasn't changing when over-writing a large file with a small file. With this fixed, I can run the DFU tests just fine for all the small files (<1M). I've fixed this locally and in the ff branch on my github.

Nice to hear that you have found the error.

Quoted 17 lines

Second, ff is slow:

Some random old build I had in flash on my system:

Quoted 3 lines

Tegra124 (Jetson TK1) # load mmc 1:1 $loadaddr dfu1.bin reading dfu1.bin
1048576 bytes read in 95 ms (10.5 MiB/s)

With my ff branch:

Quoted 2 lines

Tegra124 (Jetson TK1) # load mmc 1:1 $loadaddr dfu1.bin 1048576 bytes read in 5038 ms (203.1 KiB/s)

That's quite the slow-down! I believe this is causing dfu-util to time out on the larger files (1M+). Just for functional testing, I'll try and find a way to hack dfu-util to have a much larger timeout for the final flush operation. I wonder if the old FAT implementation had a disk cache (e.g. that 32K buffer in BSS?) and we need the same for ff?

I think that our current Fat implementation is optimized for tiny embedded system (and probably no cache).

Quoted 3 lines

I'll try and track down why it's so slow.

Perhaps there are other issues as yet unfound.

We might also check with sandbox FS set of tests.

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

---
 fs/fat/fat.c       | 11 +++++++++--
 fs/fat/fat_write.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

fs/fat/fat.c +4 -2

index bccc3e3..d743014 100644
@@ -45,11 +45,18 @@ static disk_partition_t cur_part_info;
 
 static int disk_read(__u32 block, __u32 nr_blocks, void *buf)
 {
+	ulong ret;
+
 	if (!cur_dev || !cur_dev->block_read)
 		return -1;
 
-	return cur_dev->block_read(cur_dev->dev,
-			cur_part_info.start + block, nr_blocks, buf);
+	ret = cur_dev->block_read(cur_dev->dev,
+				  cur_part_info.start + block,

nr_blocks, buf); +

fs/fat/fat.c +4 -0

+	if (nr_blocks && ret == 0)
+		return -1;
+
+	return ret;
 }
 
 int fat_set_blk_dev(block_dev_desc_t *dev_desc, disk_partition_t

*info) diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 98b88ad..adb6940 100644

fs/fat/fat_write.c +9 -2

@@ -30,6 +30,8 @@ static void uppercase(char *str, int len)
 static int total_sector;
 static int disk_write(__u32 block, __u32 nr_blocks, void *buf)
 {
+	ulong ret;
+
 	if (!cur_dev || !cur_dev->block_write)
 		return -1;
 

@@ -39,8 +41,13 @@ static int disk_write(__u32 block, __u32
 nr_blocks, void *buf) return -1;
 	}
 
-	return cur_dev->block_write(cur_dev->dev,
-			cur_part_info.start + block,
 nr_blocks,	buf);
+	ret = cur_dev->block_write(cur_dev->dev,
+				   cur_part_info.start + block,
+				   nr_blocks, buf);
+	if (nr_blocks && ret == 0)
+		return -1;
+
+	return ret;
 }
 

/*

Hi,

Quoted 14 lines

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

Quoted 5 lines

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Are there any more questions regarding this patch? I'd be more than happy if it would be added to v2015.10 :-).

Quoted 64 lines

Test HW: Odroid XU3 - Exynos 5433

---
 fs/fat/fat.c       | 11 +++++++++--
 fs/fat/fat_write.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

fs/fat/fat.c +4 -2

index bccc3e3..d743014 100644
@@ -45,11 +45,18 @@ static disk_partition_t cur_part_info;
 
 static int disk_read(__u32 block, __u32 nr_blocks, void *buf)
 {
+	ulong ret;
+
 	if (!cur_dev || !cur_dev->block_read)
 		return -1;
 
-	return cur_dev->block_read(cur_dev->dev,
-			cur_part_info.start + block, nr_blocks,
 buf);
+	ret = cur_dev->block_read(cur_dev->dev,
+				  cur_part_info.start + block,

nr_blocks, buf); +

fs/fat/fat.c +4 -0

+	if (nr_blocks && ret == 0)
+		return -1;
+
+	return ret;
 }
 
 int fat_set_blk_dev(block_dev_desc_t *dev_desc, disk_partition_t

*info) diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 98b88ad..adb6940 100644

fs/fat/fat_write.c +9 -2

@@ -30,6 +30,8 @@ static void uppercase(char *str, int len)
 static int total_sector;
 static int disk_write(__u32 block, __u32 nr_blocks, void *buf)
 {
+	ulong ret;
+
 	if (!cur_dev || !cur_dev->block_write)
 		return -1;
 

@@ -39,8 +41,13 @@ static int disk_write(__u32 block, __u32
 nr_blocks, void *buf) return -1;
 	}
 
-	return cur_dev->block_write(cur_dev->dev,
-			cur_part_info.start + block,
 nr_blocks,	buf);
+	ret = cur_dev->block_write(cur_dev->dev,
+				   cur_part_info.start + block,
+				   nr_blocks, buf);
+	if (nr_blocks && ret == 0)
+		return -1;
+
+	return ret;
 }
 

/*

It is very common that FAT code is using following pattern:
if (disk_{read|write}() < 0)

        return -1;

Up till now the above code was dead, since disk_{read|write) could only return value >= 0.
As a result some errors from medium layer (i.e. eMMC/SD) were not caught.

The above behavior was caused by block_{read|write|erase} declared at struct block_dev_desc (@part.h). It returns unsigned long, where 0 indicates error and > 0 indicates that medium operation was correct.

This patch as error regards 0 returned from block_{read|write|erase} when nr_blocks is grater than zero. Read/Write operation with nr_blocks=0 should return 0 and hence is not considered as an error.

Signed-off-by Lukasz Majewski <[email protected]>

Test HW: Odroid XU3 - Exynos 5433

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90
"mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 4 minutes (240 seconds) - is the same as the one used in Linux kernel driver. Such fix should be good enough until we come up with better fix for this issue.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

---
  drivers/mmc/dw_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +0 -0

index a84c1e1..26d34ae 100644

Hi Lukasz,

Quoted 4 lines · Lukasz Majewski

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90
"mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

Quoted 8 lines

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 4 minutes (240 seconds) - is the same as the one used in Linux kernel driver. Such fix should be good enough until we come up with better fix for this issue.

I think, that you should adjust the commit message, because it describes the result of the real problem which is in the background.

It doesn't fix the DFU, because it wasn't broken. What is the real problem and how does this commit fix it?

The commit: d9dbb97be0e4a550457aec5f11afefb446169c90
"mmc: dw_mmc: Zap endless timeout" removed endless loop waiting for end of dw mmc transfer.

For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) - and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB) the default timeout is to short.

The new value - 4 minutes (240 seconds) - is the same as the one used in Linux kernel driver. Such fix should be good enough until we come up with better fix for this issue.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

Cc Tom Rini <[email protected]>

Tested-by Przemyslaw Marczak <[email protected]>