linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] mmc: core: add a new property
       [not found] <CGME20210217053519epcas1p4204253d3af8e1c6d2a5c3acb73eb877f@epcas1p4.samsung.com>
@ 2021-02-17  5:22 ` DooHyun Hwang
       [not found]   ` <CGME20210217053520epcas1p1af462836bd9fc690d99baed0b122d6fd@epcas1p1.samsung.com>
       [not found]   ` <CGME20210217053521epcas1p2aa80cae5d52f30c8c8882f44abe8045c@epcas1p2.samsung.com>
  0 siblings, 2 replies; 8+ messages in thread
From: DooHyun Hwang @ 2021-02-17  5:22 UTC (permalink / raw)
  To: linux-mmc, linux-kernel, devicetree, ulf.hansson, robh+dt, axboe,
	adrian.hunter, satyat, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim, DooHyun Hwang

Add an optional property to not retry multiple block read error
with several single block reads.

DooHyun Hwang (2):
  dt-bindings: mmc: Add no-single-read-retry property
  mmc: core: Add no single read retries

 Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 6 ++++++
 drivers/mmc/core/block.c                                  | 3 ++-
 drivers/mmc/core/host.c                                   | 6 ++++++
 include/linux/mmc/host.h                                  | 3 +++
 4 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.29.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] dt-bindings: mmc: Add no-single-read-retry property
       [not found]   ` <CGME20210217053520epcas1p1af462836bd9fc690d99baed0b122d6fd@epcas1p1.samsung.com>
@ 2021-02-17  5:22     ` DooHyun Hwang
  0 siblings, 0 replies; 8+ messages in thread
From: DooHyun Hwang @ 2021-02-17  5:22 UTC (permalink / raw)
  To: linux-mmc, linux-kernel, devicetree, ulf.hansson, robh+dt, axboe,
	adrian.hunter, satyat, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim, DooHyun Hwang

Add an optional property to not retry multiple block read error
with several single block reads.

This property makes to handle read errors faster by not retrying
multiple block read errors with single block reads.

Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
---
 Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index e141330c1114..1eb3b73a164a 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -245,6 +245,12 @@ properties:
       Controller is limited to send MMC commands during
       initialization.
 
+  no-single-read-retry:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Single block read retrying is not supported
+      when multiple block read fails.
+
   fixed-emmc-driver-type:
     description:
       For non-removable eMMC, enforce this driver type. The value is
-- 
2.29.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] mmc: core: Add no single read retries
       [not found]   ` <CGME20210217053521epcas1p2aa80cae5d52f30c8c8882f44abe8045c@epcas1p2.samsung.com>
@ 2021-02-17  5:22     ` DooHyun Hwang
  2021-02-17  5:46       ` Adrian Hunter
  0 siblings, 1 reply; 8+ messages in thread
From: DooHyun Hwang @ 2021-02-17  5:22 UTC (permalink / raw)
  To: linux-mmc, linux-kernel, devicetree, ulf.hansson, robh+dt, axboe,
	adrian.hunter, satyat, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim, DooHyun Hwang

This makes to handle read errors faster by not retrying
multiple block read(CMD18) errors with single block reads(CMD17).

On some bad SD Cards that have problem with read operations,
it is not helpful to retry multiple block read errors with
several single block reads, and it is delayed to treat read
operations as I/O error as much as retrying single block reads.

Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
---
 drivers/mmc/core/block.c | 3 ++-
 drivers/mmc/core/host.c  | 6 ++++++
 include/linux/mmc/host.h | 3 +++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index d666e24fbe0e..e25aaf8fca34 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1809,7 +1809,8 @@ static void mmc_blk_mq_rw_recovery(struct mmc_queue *mq, struct request *req)
 
 	/* FIXME: Missing single sector read for large sector size */
 	if (!mmc_large_sector(card) && rq_data_dir(req) == READ &&
-	    brq->data.blocks > 1) {
+	    brq->data.blocks > 1 &&
+	    !card->host->no_single_read_retry) {
 		/* Read one sector at a time */
 		mmc_blk_read_single(mq, req);
 		return;
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 9b89a91b6b47..3bf5b2fc111b 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -352,6 +352,12 @@ int mmc_of_parse(struct mmc_host *host)
 	if (device_property_read_bool(dev, "no-mmc"))
 		host->caps2 |= MMC_CAP2_NO_MMC;
 
+	if (device_property_read_bool(dev, "no-single-read-retry")) {
+		dev_info(host->parent,
+			"Single block read retrying is not supported\n");
+		host->no_single_read_retry = true;
+	}
+
 	/* Must be after "non-removable" check */
 	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
 		if (host->caps & MMC_CAP_NONREMOVABLE)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 26a3c7bc29ae..faec55035a63 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -502,6 +502,9 @@ struct mmc_host {
 	/* Host Software Queue support */
 	bool			hsq_enabled;
 
+	/* Do not retry multi block read as single block read */
+	bool			no_single_read_retry;
+
 	unsigned long		private[] ____cacheline_aligned;
 };
 
-- 
2.29.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add no single read retries
  2021-02-17  5:22     ` [PATCH 2/2] mmc: core: Add no single read retries DooHyun Hwang
@ 2021-02-17  5:46       ` Adrian Hunter
  2021-02-17  6:00         ` Adrian Hunter
  0 siblings, 1 reply; 8+ messages in thread
From: Adrian Hunter @ 2021-02-17  5:46 UTC (permalink / raw)
  To: DooHyun Hwang, linux-mmc, linux-kernel, devicetree, ulf.hansson,
	robh+dt, axboe, satyat, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim

On 17/02/21 7:22 am, DooHyun Hwang wrote:
> This makes to handle read errors faster by not retrying
> multiple block read(CMD18) errors with single block reads(CMD17).
> 
> On some bad SD Cards that have problem with read operations,
> it is not helpful to retry multiple block read errors with
> several single block reads, and it is delayed to treat read
> operations as I/O error as much as retrying single block reads.

If the issue is that it takes too long, then maybe it would be better to get
mmc_blk_read_single() to give up after a certain amount of time.

> 
> Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> ---
>  drivers/mmc/core/block.c | 3 ++-
>  drivers/mmc/core/host.c  | 6 ++++++
>  include/linux/mmc/host.h | 3 +++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index d666e24fbe0e..e25aaf8fca34 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1809,7 +1809,8 @@ static void mmc_blk_mq_rw_recovery(struct mmc_queue *mq, struct request *req)
>  
>  	/* FIXME: Missing single sector read for large sector size */
>  	if (!mmc_large_sector(card) && rq_data_dir(req) == READ &&
> -	    brq->data.blocks > 1) {
> +	    brq->data.blocks > 1 &&
> +	    !card->host->no_single_read_retry) {
>  		/* Read one sector at a time */
>  		mmc_blk_read_single(mq, req);
>  		return;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 9b89a91b6b47..3bf5b2fc111b 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -352,6 +352,12 @@ int mmc_of_parse(struct mmc_host *host)
>  	if (device_property_read_bool(dev, "no-mmc"))
>  		host->caps2 |= MMC_CAP2_NO_MMC;
>  
> +	if (device_property_read_bool(dev, "no-single-read-retry")) {
> +		dev_info(host->parent,
> +			"Single block read retrying is not supported\n");
> +		host->no_single_read_retry = true;
> +	}
> +
>  	/* Must be after "non-removable" check */
>  	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
>  		if (host->caps & MMC_CAP_NONREMOVABLE)
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 26a3c7bc29ae..faec55035a63 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -502,6 +502,9 @@ struct mmc_host {
>  	/* Host Software Queue support */
>  	bool			hsq_enabled;
>  
> +	/* Do not retry multi block read as single block read */
> +	bool			no_single_read_retry;
> +
>  	unsigned long		private[] ____cacheline_aligned;
>  };
>  
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add no single read retries
  2021-02-17  5:46       ` Adrian Hunter
@ 2021-02-17  6:00         ` Adrian Hunter
  2021-02-17  8:46           ` DooHyun Hwang
  0 siblings, 1 reply; 8+ messages in thread
From: Adrian Hunter @ 2021-02-17  6:00 UTC (permalink / raw)
  To: DooHyun Hwang, linux-mmc, linux-kernel, devicetree, ulf.hansson,
	robh+dt, axboe, satyat, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim

On 17/02/21 7:46 am, Adrian Hunter wrote:
> On 17/02/21 7:22 am, DooHyun Hwang wrote:
>> This makes to handle read errors faster by not retrying
>> multiple block read(CMD18) errors with single block reads(CMD17).
>>
>> On some bad SD Cards that have problem with read operations,
>> it is not helpful to retry multiple block read errors with
>> several single block reads, and it is delayed to treat read
>> operations as I/O error as much as retrying single block reads.
> 
> If the issue is that it takes too long, then maybe it would be better to get
> mmc_blk_read_single() to give up after a certain amount of time.
> 

So that a device property would not be needed I mean.  Then everyone would
benefit.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 2/2] mmc: core: Add no single read retries
  2021-02-17  6:00         ` Adrian Hunter
@ 2021-02-17  8:46           ` DooHyun Hwang
  2021-03-01  8:23             ` Ulf Hansson
  0 siblings, 1 reply; 8+ messages in thread
From: DooHyun Hwang @ 2021-02-17  8:46 UTC (permalink / raw)
  To: 'Adrian Hunter',
	linux-mmc, linux-kernel, devicetree, ulf.hansson, robh+dt, axboe,
	satyat, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim


On 17/02/21 8:00 am, Adrian Hunter wrote:
>On 17/02/21 7:46 am, Adrian Hunter wrote:
>> On 17/02/21 7:22 am, DooHyun Hwang wrote:
>>> This makes to handle read errors faster by not retrying multiple
>>> block read(CMD18) errors with single block reads(CMD17).
>>>
>>> On some bad SD Cards that have problem with read operations, it is
>>> not helpful to retry multiple block read errors with several single
>>> block reads, and it is delayed to treat read operations as I/O error
>>> as much as retrying single block reads.
>>
>> If the issue is that it takes too long, then maybe it would be better
>> to get
>> mmc_blk_read_single() to give up after a certain amount of time.
>>
>
>So that a device property would not be needed I mean.  Then everyone would
>benefit.

Thank you for reviewing this.

mmc_blk_read_single() takes a different time depending on the number of
sectors to read and the timeout value for each CMD.

I think it's difficult to set the criteria for "a certain amount of time"
you talked about.
And it's harder to proceed with any errors caused by giving up in
mmc_blk_read_single() than no retrying.

So, I would like to add a configurable property to skip the single block
read retrying because if multiple block read error occurs, single block
read retrying doesn't help for some bad SD cards.

This is the log to check for this patch.
#0. time difference is about 2.37s for 8 sectors between with(#1) and without(#2)
     single block read retrying
     This is a test for just one CMD18.
     When there are many I/O requests, it takes too long to handle the errors.

#1. retry multiple block read (8 sectors) error with single block reads
// It takes about 3.585671s for the I/O error.
// issue CMD23 (+ arg 0x8)
// issue CMD18 (+ arg 0x000320e0) and error occurs
<7>[  316.657115]  [5:   kworker/5:1H:  324] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  316.657124]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
<7>[  316.826302] I[0:      swapper/0:    0] mmc0: req done <CMD23>: 0: 00000000 00000000 00000000 00000000
<7>[  316.826327] I[0:      swapper/0:    0] mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
<7>[  316.826362] I[0:      swapper/0:    0] mmc0:     0 bytes transferred: -110
<7>[  316.826389] I[0:      swapper/0:    0] mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
<7>[  316.826516]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
<7>[  316.826621] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
// retry CMD18 (+ arg 0x000320e0) and error occurs again. Same as above.
<7>[  316.829224]  [5:   kworker/5:1H:  324] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  316.829237]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
<7>[  316.999588] I[0:      swapper/0:    0] mmc0: req done <CMD23>: 0: 00000000 00000000 00000000 00000000
<7>[  316.999653] I[0:      swapper/0:    0] mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
<7>[  316.999725] I[0:      swapper/0:    0] mmc0:     0 bytes transferred: -110
<7>[  316.999789] I[0:      swapper/0:    0] mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
<7>[  317.000034]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
<7>[  317.000370] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
// mmc_blk_reset() and it's completed
<7>[  317.000523]  [0:   kworker/0:1H:  338] mmc0: clock 0Hz busmode 2 powermode 0 cs 0 Vdd 0 width 1 timing 0
...
// mmc_blk_read_single() : CMD17, CMD13 and CMD12 repeats 8 times (for retrying multiple block read with 8 sectors)
// CMD17 (+ arg 0x000320e0 ~ 0x000320e7) and timeout errors occur
// It takes about 1.351s
<7>[  317.200351]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e0 flags 000000b5
<7>[  317.368748] I[0:      swapper/0:    0] mmc0: req done (CMD17): 0: 00000900 00000000 00000000 00000000
<7>[  317.368776] I[0:      swapper/0:    0] mmc0:     0 bytes transferred: -110
<7>[  317.368871]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
<7>[  317.368932] I[0:   kworker/0:1H:  338] mmc0: sdhci: IRQ status 0x00000001
<7>[  317.368970] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000b00 00000000 00000000 00000000
<7>[  317.369020]  [0:   kworker/0:1H:  338] mmc0: starting CMD12 arg 00000000 flags 00000095
<7>[  317.369070] I[0:   kworker/0:1H:  338] mmc0: sdhci: IRQ status 0x00000001
<7>[  317.369108] I[0:   kworker/0:1H:  338] mmc0: req done (CMD12): 0: 00000b00 00000000 00000000 00000000
<7>[  317.369155]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
<7>[  317.369204] I[0:   kworker/0:1H:  338] mmc0: sdhci: IRQ status 0x00000001
<7>[  317.369245] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
<3>[  317.369298]  [0:   kworker/0:1H:  338] print_req_error: I/O error, dev mmcblk0, sector 205024
<7>[  317.369342]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e1 flags 000000b5
...
<7>[  318.382668]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e7 flags 000000b5
<3>[  318.551568]  [0:   kworker/0:1H:  338] print_req_error: I/O error, dev mmcblk0, sector 205031
// retry CMD18 (+ arg 0x000320e0) and error occurs again.
<7>[  318.551850]  [5:   kworker/5:1H:  324] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  318.551867]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
...
// retry CMD18 (+ arg 0x000320e0) and error occurs again.
<7>[  318.721767]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
// CMD17 (+ arg 0x000320e0 ~ 0x000320e7)
<7>[  318.891054]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e0 flags 000000b5
...
<7>[  320.073861]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e7 flags 000000b5
// Return I/O error for read operation finally
<3>[  320.242786]  [0:   kworker/0:1H:  338] Buffer I/O error on dev mmcblk0, logical block 25628, async page read

#2. retry multiple block read (8 sectors) error without single block reads
// It takes about 1.205941s for the I/O error.
// issue CMD23 (+ arg 0x8)
// issue CMD18 (+ arg 0x000320e0) and error occurs
<7>[  126.467114]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  126.467125]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
<7>[  126.636188] I[0:Measurement Wor: 9074] mmc0: req done <CMD23>: 0: 00000000 00000000 00000000 00000000
<7>[  126.636213] I[0:Measurement Wor: 9074] mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
<7>[  126.636241] I[0:Measurement Wor: 9074] mmc0:     0 bytes transferred: -110
<7>[  126.636265] I[0:Measurement Wor: 9074] mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
<7>[  126.636379]  [0:   kworker/0:1H:  336] mmc0: starting CMD13 arg 00010000 flags 00000195
<7>[  126.636495] I[0:   kworker/0:1H:  336] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
// retry CMD18 (+ arg 0x000320e0) and error occurs again. Same as above.
<7>[  126.638284]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  126.638298]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
// mmc_blk_reset() and it's completed
<7>[  126.807645]  [0:   kworker/0:1H:  336] mmc0: clock 0Hz busmode 2 powermode 0 cs 0 Vdd 0 width 1 timing 0
...
// no mmc_blk_read_single() calling
// retry CMD18 (+ arg 0x000320e0) and error occurs again.
<7>[  126.993628]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  126.993643]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
// retry CMD18 (+ arg 0x000320e0) and error occurs again.
<7>[  127.164836]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
<7>[  127.164848]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
...
// Return I/O error for read operation finally
<3>[  127.673055] I[7:      swapper/7:    0] Buffer I/O error on dev mmcblk0, logical block 25628, async page read


Thank you.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add no single read retries
  2021-02-17  8:46           ` DooHyun Hwang
@ 2021-03-01  8:23             ` Ulf Hansson
  0 siblings, 0 replies; 8+ messages in thread
From: Ulf Hansson @ 2021-03-01  8:23 UTC (permalink / raw)
  To: DooHyun Hwang
  Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, DTML,
	Rob Herring, Jens Axboe, Satya Tangirala, ebiggers,
	Gustavo A. R. Silva, grant.jung, jt77.jang, junwoo80.lee,
	jangsub.yi, sh043.lee, Chanwoo Lee, sh8267.baek, wkon.kim

On Wed, 17 Feb 2021 at 09:46, DooHyun Hwang <dh0421.hwang@samsung.com> wrote:
>
>
> On 17/02/21 8:00 am, Adrian Hunter wrote:
> >On 17/02/21 7:46 am, Adrian Hunter wrote:
> >> On 17/02/21 7:22 am, DooHyun Hwang wrote:
> >>> This makes to handle read errors faster by not retrying multiple
> >>> block read(CMD18) errors with single block reads(CMD17).
> >>>
> >>> On some bad SD Cards that have problem with read operations, it is
> >>> not helpful to retry multiple block read errors with several single
> >>> block reads, and it is delayed to treat read operations as I/O error
> >>> as much as retrying single block reads.
> >>
> >> If the issue is that it takes too long, then maybe it would be better
> >> to get
> >> mmc_blk_read_single() to give up after a certain amount of time.
> >>
> >
> >So that a device property would not be needed I mean.  Then everyone would
> >benefit.

Just wanted to confirm with Adrian's points, that we don't want a
device property for this.

In fact, the DT maintainers would reject it because it would be
considered as a software configuration, which doesn't belong in DT.

>
> Thank you for reviewing this.
>
> mmc_blk_read_single() takes a different time depending on the number of
> sectors to read and the timeout value for each CMD.
>
> I think it's difficult to set the criteria for "a certain amount of time"
> you talked about.
> And it's harder to proceed with any errors caused by giving up in
> mmc_blk_read_single() than no retrying.
>
> So, I would like to add a configurable property to skip the single block
> read retrying because if multiple block read error occurs, single block
> read retrying doesn't help for some bad SD cards.

I certainly agree that falling back to single block reads is
questionable, at least for some cases. Moreover, I am pretty sure it's
not always the SD card that should be blamed, but a broken mmc host
driver or broken HW/controller.

That said, I assume that the main reason why we fall back to retry
with single block reads, is to try to recover as much data as possible
from a broken SD card. The intent is good, but to recover data from a
broken card, we should also consider to move to a lower/legacy speed
mode and to decrease the clock rate of the interface.

For the clock rate, we already have a debugfs node, allowing us to
change the rate per mmc host. I suggest we add a few more debugfs
nodes, allowing us to restrict the speed mode and to enable/disable
single/multi block read.

If we can get these things in place to help with recovery, I wouldn't
mind us changing the default behaviour to skip the single block read
in the recovery path.

>
> This is the log to check for this patch.
> #0. time difference is about 2.37s for 8 sectors between with(#1) and without(#2)
>      single block read retrying
>      This is a test for just one CMD18.
>      When there are many I/O requests, it takes too long to handle the errors.
>
> #1. retry multiple block read (8 sectors) error with single block reads
> // It takes about 3.585671s for the I/O error.
> // issue CMD23 (+ arg 0x8)
> // issue CMD18 (+ arg 0x000320e0) and error occurs
> <7>[  316.657115]  [5:   kworker/5:1H:  324] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  316.657124]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> <7>[  316.826302] I[0:      swapper/0:    0] mmc0: req done <CMD23>: 0: 00000000 00000000 00000000 00000000
> <7>[  316.826327] I[0:      swapper/0:    0] mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
> <7>[  316.826362] I[0:      swapper/0:    0] mmc0:     0 bytes transferred: -110
> <7>[  316.826389] I[0:      swapper/0:    0] mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
> <7>[  316.826516]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
> <7>[  316.826621] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
> // retry CMD18 (+ arg 0x000320e0) and error occurs again. Same as above.
> <7>[  316.829224]  [5:   kworker/5:1H:  324] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  316.829237]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> <7>[  316.999588] I[0:      swapper/0:    0] mmc0: req done <CMD23>: 0: 00000000 00000000 00000000 00000000
> <7>[  316.999653] I[0:      swapper/0:    0] mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
> <7>[  316.999725] I[0:      swapper/0:    0] mmc0:     0 bytes transferred: -110
> <7>[  316.999789] I[0:      swapper/0:    0] mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
> <7>[  317.000034]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
> <7>[  317.000370] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
> // mmc_blk_reset() and it's completed
> <7>[  317.000523]  [0:   kworker/0:1H:  338] mmc0: clock 0Hz busmode 2 powermode 0 cs 0 Vdd 0 width 1 timing 0
> ...
> // mmc_blk_read_single() : CMD17, CMD13 and CMD12 repeats 8 times (for retrying multiple block read with 8 sectors)
> // CMD17 (+ arg 0x000320e0 ~ 0x000320e7) and timeout errors occur
> // It takes about 1.351s
> <7>[  317.200351]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e0 flags 000000b5
> <7>[  317.368748] I[0:      swapper/0:    0] mmc0: req done (CMD17): 0: 00000900 00000000 00000000 00000000
> <7>[  317.368776] I[0:      swapper/0:    0] mmc0:     0 bytes transferred: -110
> <7>[  317.368871]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
> <7>[  317.368932] I[0:   kworker/0:1H:  338] mmc0: sdhci: IRQ status 0x00000001
> <7>[  317.368970] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000b00 00000000 00000000 00000000
> <7>[  317.369020]  [0:   kworker/0:1H:  338] mmc0: starting CMD12 arg 00000000 flags 00000095
> <7>[  317.369070] I[0:   kworker/0:1H:  338] mmc0: sdhci: IRQ status 0x00000001
> <7>[  317.369108] I[0:   kworker/0:1H:  338] mmc0: req done (CMD12): 0: 00000b00 00000000 00000000 00000000
> <7>[  317.369155]  [0:   kworker/0:1H:  338] mmc0: starting CMD13 arg 00010000 flags 00000195
> <7>[  317.369204] I[0:   kworker/0:1H:  338] mmc0: sdhci: IRQ status 0x00000001
> <7>[  317.369245] I[0:   kworker/0:1H:  338] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
> <3>[  317.369298]  [0:   kworker/0:1H:  338] print_req_error: I/O error, dev mmcblk0, sector 205024
> <7>[  317.369342]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e1 flags 000000b5
> ...
> <7>[  318.382668]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e7 flags 000000b5
> <3>[  318.551568]  [0:   kworker/0:1H:  338] print_req_error: I/O error, dev mmcblk0, sector 205031
> // retry CMD18 (+ arg 0x000320e0) and error occurs again.
> <7>[  318.551850]  [5:   kworker/5:1H:  324] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  318.551867]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> ...
> // retry CMD18 (+ arg 0x000320e0) and error occurs again.
> <7>[  318.721767]  [5:   kworker/5:1H:  324] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> // CMD17 (+ arg 0x000320e0 ~ 0x000320e7)
> <7>[  318.891054]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e0 flags 000000b5
> ...
> <7>[  320.073861]  [0:   kworker/0:1H:  338] mmc0: starting CMD17 arg 000320e7 flags 000000b5
> // Return I/O error for read operation finally
> <3>[  320.242786]  [0:   kworker/0:1H:  338] Buffer I/O error on dev mmcblk0, logical block 25628, async page read
>
> #2. retry multiple block read (8 sectors) error without single block reads
> // It takes about 1.205941s for the I/O error.
> // issue CMD23 (+ arg 0x8)
> // issue CMD18 (+ arg 0x000320e0) and error occurs
> <7>[  126.467114]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  126.467125]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> <7>[  126.636188] I[0:Measurement Wor: 9074] mmc0: req done <CMD23>: 0: 00000000 00000000 00000000 00000000
> <7>[  126.636213] I[0:Measurement Wor: 9074] mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
> <7>[  126.636241] I[0:Measurement Wor: 9074] mmc0:     0 bytes transferred: -110
> <7>[  126.636265] I[0:Measurement Wor: 9074] mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
> <7>[  126.636379]  [0:   kworker/0:1H:  336] mmc0: starting CMD13 arg 00010000 flags 00000195
> <7>[  126.636495] I[0:   kworker/0:1H:  336] mmc0: req done (CMD13): 0: 00000900 00000000 00000000 00000000
> // retry CMD18 (+ arg 0x000320e0) and error occurs again. Same as above.
> <7>[  126.638284]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  126.638298]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> // mmc_blk_reset() and it's completed
> <7>[  126.807645]  [0:   kworker/0:1H:  336] mmc0: clock 0Hz busmode 2 powermode 0 cs 0 Vdd 0 width 1 timing 0
> ...
> // no mmc_blk_read_single() calling
> // retry CMD18 (+ arg 0x000320e0) and error occurs again.
> <7>[  126.993628]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  126.993643]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> // retry CMD18 (+ arg 0x000320e0) and error occurs again.
> <7>[  127.164836]  [7:   kworker/7:2H: 8887] <mmc0: starting CMD23 arg 00000008 flags 00000015>
> <7>[  127.164848]  [7:   kworker/7:2H: 8887] mmc0: starting CMD18 arg 000320e0 flags 000000b5
> ...
> // Return I/O error for read operation finally
> <3>[  127.673055] I[7:      swapper/7:    0] Buffer I/O error on dev mmcblk0, logical block 25628, async page read
>

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 0/2] mmc: core: add a new property
       [not found] <CGME20210217053233epcas1p3f6b8177e5f2d27a4f7dc4f136bdcfa48@epcas1p3.samsung.com>
@ 2021-02-17  5:19 ` DooHyun Hwang
  0 siblings, 0 replies; 8+ messages in thread
From: DooHyun Hwang @ 2021-02-17  5:19 UTC (permalink / raw)
  To: linux-mmc, linux-kernel, devicetree, ulf.hansson, robh+dt, axboe,
	adrian.hunter, satyat, baolin.wang, ebiggers, gustavoars
  Cc: grant.jung, jt77.jang, junwoo80.lee, jangsub.yi, sh043.lee,
	cw9316.lee, sh8267.baek, wkon.kim, DooHyun Hwang

Add an optional property to not retry multiple block read error
with several single block reads.

DooHyun Hwang (2):
  dt-bindings: mmc: Add no-single-read-retry property
  mmc: core: Add no single read retries

 Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 6 ++++++
 drivers/mmc/core/block.c                                  | 3 ++-
 drivers/mmc/core/host.c                                   | 6 ++++++
 include/linux/mmc/host.h                                  | 3 +++
 4 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.29.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-03-01  8:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210217053519epcas1p4204253d3af8e1c6d2a5c3acb73eb877f@epcas1p4.samsung.com>
2021-02-17  5:22 ` [PATCH 0/2] mmc: core: add a new property DooHyun Hwang
     [not found]   ` <CGME20210217053520epcas1p1af462836bd9fc690d99baed0b122d6fd@epcas1p1.samsung.com>
2021-02-17  5:22     ` [PATCH 1/2] dt-bindings: mmc: Add no-single-read-retry property DooHyun Hwang
     [not found]   ` <CGME20210217053521epcas1p2aa80cae5d52f30c8c8882f44abe8045c@epcas1p2.samsung.com>
2021-02-17  5:22     ` [PATCH 2/2] mmc: core: Add no single read retries DooHyun Hwang
2021-02-17  5:46       ` Adrian Hunter
2021-02-17  6:00         ` Adrian Hunter
2021-02-17  8:46           ` DooHyun Hwang
2021-03-01  8:23             ` Ulf Hansson
     [not found] <CGME20210217053233epcas1p3f6b8177e5f2d27a4f7dc4f136bdcfa48@epcas1p3.samsung.com>
2021-02-17  5:19 ` [PATCH 0/2] mmc: core: add a new property DooHyun Hwang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).