* [PATCH] mmc: block: fix read single on recovery logic @ 2022-01-05 16:43 Christian Löhle 2022-02-03 10:09 ` Christian Löhle 0 siblings, 1 reply; 8+ messages in thread From: Christian Löhle @ 2022-01-05 16:43 UTC (permalink / raw) To: ulf.hansson, Christian Löhle, linux-mmc, linux-kernel; +Cc: Avri Altman On reads with MMC_READ_MULTIPLE_BLOCK that fail, the recovery handler will use MMC_READ_SINGLE_BLOCK for each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. The logic for this is fixed to never report unsuccessful reads as success to the block layer. On command error with retries remaining, blk_update_request was called with whatever value error was set last to. In case it was last set to BLK_STS_OK (default), the read will be reported as success, even though there was no data read from the device. This could happen on a CRC mismatch for the response, a card rejecting the command (e.g. again due to a CRC mismatch). In case it was last set to BLK_STS_IOERR, the error is reported correctly, but no retries will be attempted. The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining, or BLK_STS_OK if the single read was successful in the meantime. Signed-off-by: Christian Loehle <cloehle@hyperstone.com> --- drivers/mmc/core/block.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 90e1bcd03b46..d7d880ce0f8a 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) struct mmc_card *card = mq->card; struct mmc_host *host = card->host; blk_status_t error = BLK_STS_OK; - int retries = 0; do { u32 status; int err; + int retries = 0; - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); + while (retries++ < MMC_READ_SINGLE_RETRIES) { + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); - mmc_wait_for_req(host, mrq); + mmc_wait_for_req(host, mrq); - err = mmc_send_status(card, &status); - if (err) - goto error_exit; - - if (!mmc_host_is_spi(host) && - !mmc_ready_for_data(status)) { - err = mmc_blk_fix_state(card, req); + err = mmc_send_status(card, &status); if (err) goto error_exit; - } - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) - continue; + if (!mmc_host_is_spi(host) && + !mmc_ready_for_data(status)) { + err = mmc_blk_fix_state(card, req); + if (err) + goto error_exit; + } - retries = 0; + if (!mrq->cmd->error && !mrq->data->error) + break; + } if (mrq->cmd->error || mrq->data->error || -- 2.34.1 Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: block: fix read single on recovery logic 2022-01-05 16:43 [PATCH] mmc: block: fix read single on recovery logic Christian Löhle @ 2022-02-03 10:09 ` Christian Löhle 2022-02-04 9:47 ` Ulf Hansson 0 siblings, 1 reply; 8+ messages in thread From: Christian Löhle @ 2022-02-03 10:09 UTC (permalink / raw) To: ulf.hansson, linux-mmc, linux-kernel; +Cc: Avri Altman So could anyone take a long at this so far? From: Christian Löhle Sent: Wednesday, January 5, 2022 5:43 PM To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Avri Altman Subject: [PATCH] mmc: block: fix read single on recovery logic On reads with MMC_READ_MULTIPLE_BLOCK that fail, the recovery handler will use MMC_READ_SINGLE_BLOCK for each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. The logic for this is fixed to never report unsuccessful reads as success to the block layer. On command error with retries remaining, blk_update_request was called with whatever value error was set last to. In case it was last set to BLK_STS_OK (default), the read will be reported as success, even though there was no data read from the device. This could happen on a CRC mismatch for the response, a card rejecting the command (e.g. again due to a CRC mismatch). In case it was last set to BLK_STS_IOERR, the error is reported correctly, but no retries will be attempted. The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining, or BLK_STS_OK if the single read was successful in the meantime. Signed-off-by: Christian Loehle <cloehle@hyperstone.com> --- drivers/mmc/core/block.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 90e1bcd03b46..d7d880ce0f8a 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) struct mmc_card *card = mq->card; struct mmc_host *host = card->host; blk_status_t error = BLK_STS_OK; - int retries = 0; do { u32 status; int err; + int retries = 0; - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); + while (retries++ < MMC_READ_SINGLE_RETRIES) { + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); - mmc_wait_for_req(host, mrq); + mmc_wait_for_req(host, mrq); - err = mmc_send_status(card, &status); - if (err) - goto error_exit; - - if (!mmc_host_is_spi(host) && - !mmc_ready_for_data(status)) { - err = mmc_blk_fix_state(card, req); + err = mmc_send_status(card, &status); if (err) goto error_exit; - } - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) - continue; + if (!mmc_host_is_spi(host) && + !mmc_ready_for_data(status)) { + err = mmc_blk_fix_state(card, req); + if (err) + goto error_exit; + } - retries = 0; + if (!mrq->cmd->error && !mrq->data->error) + break; + } if (mrq->cmd->error || mrq->data->error || -- 2.34.1 = Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: block: fix read single on recovery logic 2022-02-03 10:09 ` Christian Löhle @ 2022-02-04 9:47 ` Ulf Hansson 2022-02-04 11:26 ` Adrian Hunter 0 siblings, 1 reply; 8+ messages in thread From: Ulf Hansson @ 2022-02-04 9:47 UTC (permalink / raw) To: Christian Löhle; +Cc: linux-mmc, linux-kernel, Avri Altman, Adrian Hunter + Adrian On Thu, 3 Feb 2022 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote: > > So could anyone take a long at this so far? > Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list. I have added Adrian too, who knows this code very well too. Kind regards Uffe > > > From: Christian Löhle > Sent: Wednesday, January 5, 2022 5:43 PM > To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Avri Altman > Subject: [PATCH] mmc: block: fix read single on recovery logic > > On reads with MMC_READ_MULTIPLE_BLOCK that fail, > the recovery handler will use MMC_READ_SINGLE_BLOCK for > each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. > The logic for this is fixed to never report unsuccessful reads > as success to the block layer. > > On command error with retries remaining, blk_update_request was > called with whatever value error was set last to. > In case it was last set to BLK_STS_OK (default), the read will be > reported as success, even though there was no data read from the device. > This could happen on a CRC mismatch for the response, > a card rejecting the command (e.g. again due to a CRC mismatch). > In case it was last set to BLK_STS_IOERR, the error is reported correctly, > but no retries will be attempted. > > The patch now will count both command and data errors as retries and > send BLK_STS_IOERR if there are no retries remaining, > or BLK_STS_OK if the single read was successful in the meantime. > > Signed-off-by: Christian Loehle <cloehle@hyperstone.com> > --- > drivers/mmc/core/block.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 90e1bcd03b46..d7d880ce0f8a 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) > struct mmc_card *card = mq->card; > struct mmc_host *host = card->host; > blk_status_t error = BLK_STS_OK; > - int retries = 0; > > do { > u32 status; > int err; > + int retries = 0; > > - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); > + while (retries++ < MMC_READ_SINGLE_RETRIES) { > + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); > > - mmc_wait_for_req(host, mrq); > + mmc_wait_for_req(host, mrq); > > - err = mmc_send_status(card, &status); > - if (err) > - goto error_exit; > - > - if (!mmc_host_is_spi(host) && > - !mmc_ready_for_data(status)) { > - err = mmc_blk_fix_state(card, req); > + err = mmc_send_status(card, &status); > if (err) > goto error_exit; > - } > > - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) > - continue; > + if (!mmc_host_is_spi(host) && > + !mmc_ready_for_data(status)) { > + err = mmc_blk_fix_state(card, req); > + if (err) > + goto error_exit; > + } > > - retries = 0; > + if (!mrq->cmd->error && !mrq->data->error) > + break; > + } > > if (mrq->cmd->error || > mrq->data->error || > -- > 2.34.1 > = > Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz > Managing Director: Dr. Jan Peter Berns. > Commercial register of local courts: Freiburg HRB381782 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: block: fix read single on recovery logic 2022-02-04 9:47 ` Ulf Hansson @ 2022-02-04 11:26 ` Adrian Hunter 2022-02-04 14:34 ` Christian Löhle 0 siblings, 1 reply; 8+ messages in thread From: Adrian Hunter @ 2022-02-04 11:26 UTC (permalink / raw) To: Ulf Hansson, Christian Löhle; +Cc: linux-mmc, linux-kernel, Avri Altman On 04/02/2022 11:47, Ulf Hansson wrote: > + Adrian > > On Thu, 3 Feb 2022 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote: >> >> So could anyone take a long at this so far? >> > > Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list. > > I have added Adrian too, who knows this code very well too. > > Kind regards > Uffe > >> >> >> From: Christian Löhle >> Sent: Wednesday, January 5, 2022 5:43 PM >> To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: Avri Altman >> Subject: [PATCH] mmc: block: fix read single on recovery logic >> >> On reads with MMC_READ_MULTIPLE_BLOCK that fail, >> the recovery handler will use MMC_READ_SINGLE_BLOCK for >> each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. >> The logic for this is fixed to never report unsuccessful reads >> as success to the block layer. >> >> On command error with retries remaining, blk_update_request was >> called with whatever value error was set last to. >> In case it was last set to BLK_STS_OK (default), the read will be >> reported as success, even though there was no data read from the device. >> This could happen on a CRC mismatch for the response, >> a card rejecting the command (e.g. again due to a CRC mismatch). >> In case it was last set to BLK_STS_IOERR, the error is reported correctly, >> but no retries will be attempted. >> >> The patch now will count both command and data errors as retries and >> send BLK_STS_IOERR if there are no retries remaining, >> or BLK_STS_OK if the single read was successful in the meantime. >> >> Signed-off-by: Christian Loehle <cloehle@hyperstone.com> Thanks for the patch. Looks OK, although a couple of comments below, plus it needs a Fixes tag, and Cc for stable. Fixes: 81196976ed946c ("mmc: block: Add blk-mq support") Cc: stable@vger.kernel.org >> --- >> drivers/mmc/core/block.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c >> index 90e1bcd03b46..d7d880ce0f8a 100644 >> --- a/drivers/mmc/core/block.c >> +++ b/drivers/mmc/core/block.c >> @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) >> struct mmc_card *card = mq->card; >> struct mmc_host *host = card->host; >> blk_status_t error = BLK_STS_OK; >> - int retries = 0; >> >> do { >> u32 status; >> int err; >> + int retries = 0; >> >> - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); >> + while (retries++ < MMC_READ_SINGLE_RETRIES) { Because this is now checked at the top of the loop, wouldn't that result in one fewer retries than before? So, maybe: while (retries++ <= MMC_READ_SINGLE_RETRIES) { >> + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); >> >> - mmc_wait_for_req(host, mrq); >> + mmc_wait_for_req(host, mrq); >> >> - err = mmc_send_status(card, &status); >> - if (err) >> - goto error_exit; >> - >> - if (!mmc_host_is_spi(host) && >> - !mmc_ready_for_data(status)) { >> - err = mmc_blk_fix_state(card, req); >> + err = mmc_send_status(card, &status); >> if (err) >> goto error_exit; >> - } >> >> - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) >> - continue; >> + if (!mmc_host_is_spi(host) && >> + !mmc_ready_for_data(status)) { >> + err = mmc_blk_fix_state(card, req); >> + if (err) >> + goto error_exit; >> + } >> >> - retries = 0; >> + if (!mrq->cmd->error && !mrq->data->error) We weren't retrying for data errors before, and I don't think we want to because single block read can be very slow. i.e. just if (!mrq->cmd->error) >> + break; >> + } >> >> if (mrq->cmd->error || >> mrq->data->error || >> -- >> 2.34.1 >> = >> Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz >> Managing Director: Dr. Jan Peter Berns. >> Commercial register of local courts: Freiburg HRB381782 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: block: fix read single on recovery logic 2022-02-04 11:26 ` Adrian Hunter @ 2022-02-04 14:34 ` Christian Löhle 2022-02-04 15:11 ` [PATCHv2] " Christian Löhle 0 siblings, 1 reply; 8+ messages in thread From: Christian Löhle @ 2022-02-04 14:34 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson; +Cc: linux-mmc, linux-kernel, Avri Altman Thanks for the comments Adrian! >> + while (retries++ < MMC_READ_SINGLE_RETRIES) { > >Because this is now checked at the top of the loop, wouldn't that >result in one fewer retries than before? So, maybe: > > while (retries++ <= MMC_READ_SINGLE_RETRIES) { Yes, you are correct. Will be fixed in v2. >> + if (!mrq->cmd->error && !mrq->data->error) > >We weren't retrying for data errors before, and I don't think we want to >because single block read can be very slow. i.e. just > > if (!mrq->cmd->error) That was intentional by me, it was very unintuitive to my you would not retry for data errors. (Considering a data error is likely how you got into the whole recovery in the first place.) But yes I see your point, a very large request might block this for quite a while. Will change in v2, too. From: Adrian Hunter <adrian.hunter@intel.com> Sent: Friday, February 4, 2022 12:26 PM To: Ulf Hansson; Christian Löhle Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Avri Altman Subject: Re: [PATCH] mmc: block: fix read single on recovery logic On 04/02/2022 11:47, Ulf Hansson wrote: > + Adrian > > On Thu, 3 Feb 2022 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote: >> >> So could anyone take a long at this so far? >> > > Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list. > > I have added Adrian too, who knows this code very well too. > > Kind regards > Uffe > >> >> >> From: Christian Löhle >> Sent: Wednesday, January 5, 2022 5:43 PM >> To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: Avri Altman >> Subject: [PATCH] mmc: block: fix read single on recovery logic >> >> On reads with MMC_READ_MULTIPLE_BLOCK that fail, >> the recovery handler will use MMC_READ_SINGLE_BLOCK for >> each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. >> The logic for this is fixed to never report unsuccessful reads >> as success to the block layer. >> >> On command error with retries remaining, blk_update_request was >> called with whatever value error was set last to. >> In case it was last set to BLK_STS_OK (default), the read will be >> reported as success, even though there was no data read from the device. >> This could happen on a CRC mismatch for the response, >> a card rejecting the command (e.g. again due to a CRC mismatch). >> In case it was last set to BLK_STS_IOERR, the error is reported correctly, >> but no retries will be attempted. >> >> The patch now will count both command and data errors as retries and >> send BLK_STS_IOERR if there are no retries remaining, >> or BLK_STS_OK if the single read was successful in the meantime. >> >> Signed-off-by: Christian Loehle <cloehle@hyperstone.com> Thanks for the patch. Looks OK, although a couple of comments below, plus it needs a Fixes tag, and Cc for stable. Fixes: 81196976ed946c ("mmc: block: Add blk-mq support") Cc: stable@vger.kernel.org >> --- >> drivers/mmc/core/block.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c >> index 90e1bcd03b46..d7d880ce0f8a 100644 >> --- a/drivers/mmc/core/block.c >> +++ b/drivers/mmc/core/block.c >> @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) >> struct mmc_card *card = mq->card; >> struct mmc_host *host = card->host; >> blk_status_t error = BLK_STS_OK; >> - int retries = 0; >> >> do { >> u32 status; >> int err; >> + int retries = 0; >> >> - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); >> + while (retries++ < MMC_READ_SINGLE_RETRIES) { Because this is now checked at the top of the loop, wouldn't that result in one fewer retries than before? So, maybe: while (retries++ <= MMC_READ_SINGLE_RETRIES) { >> + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); >> >> - mmc_wait_for_req(host, mrq); >> + mmc_wait_for_req(host, mrq); >> >> - err = mmc_send_status(card, &status); >> - if (err) >> - goto error_exit; >> - >> - if (!mmc_host_is_spi(host) && >> - !mmc_ready_for_data(status)) { >> - err = mmc_blk_fix_state(card, req); >> + err = mmc_send_status(card, &status); >> if (err) >> goto error_exit; >> - } >> >> - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) >> - continue; >> + if (!mmc_host_is_spi(host) && >> + !mmc_ready_for_data(status)) { >> + err = mmc_blk_fix_state(card, req); >> + if (err) >> + goto error_exit; >> + } >> >> - retries = 0; >> + if (!mrq->cmd->error && !mrq->data->error) We weren't retrying for data errors before, and I don't think we want to because single block read can be very slow. i.e. just if (!mrq->cmd->error) >> + break; >> + } >> >> if (mrq->cmd->error || >> mrq->data->error || >> -- >> 2.34.1 >> = >> Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz >> Managing Director: Dr. Jan Peter Berns. >> Commercial register of local courts: Freiburg HRB381782 >> = Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] mmc: block: fix read single on recovery logic 2022-02-04 14:34 ` Christian Löhle @ 2022-02-04 15:11 ` Christian Löhle 2022-02-08 6:37 ` Adrian Hunter 0 siblings, 1 reply; 8+ messages in thread From: Christian Löhle @ 2022-02-04 15:11 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Christian Löhle Cc: linux-mmc, linux-kernel, Avri Altman, stable On reads with MMC_READ_MULTIPLE_BLOCK that fail, the recovery handler will use MMC_READ_SINGLE_BLOCK for each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. The logic for this is fixed to never report unsuccessful reads as success to the block layer. On command error with retries remaining, blk_update_request was called with whatever value error was set last to. In case it was last set to BLK_STS_OK (default), the read will be reported as success, even though there was no data read from the device. This could happen on a CRC mismatch for the response, a card rejecting the command (e.g. again due to a CRC mismatch). In case it was last set to BLK_STS_IOERR, the error is reported correctly, but no retries will be attempted. Fixes: 81196976ed946c ("mmc: block: Add blk-mq support") Cc: stable@vger.kernel.org Signed-off-by: Christian Loehle <cloehle@hyperstone.com> --- v2: - Do not allow data error retries - Actually retry MMC_READ_SINGLE_RETRIES times instead of MMC_READ_SINGLE_RETRIES-1 drivers/mmc/core/block.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 4e61b28a002f..8d718aa56d33 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) struct mmc_card *card = mq->card; struct mmc_host *host = card->host; blk_status_t error = BLK_STS_OK; - int retries = 0; do { u32 status; int err; + int retries = 0; - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); + while (retries++ <= MMC_READ_SINGLE_RETRIES) { + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); - mmc_wait_for_req(host, mrq); + mmc_wait_for_req(host, mrq); - err = mmc_send_status(card, &status); - if (err) - goto error_exit; - - if (!mmc_host_is_spi(host) && - !mmc_ready_for_data(status)) { - err = mmc_blk_fix_state(card, req); + err = mmc_send_status(card, &status); if (err) goto error_exit; - } - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) - continue; + if (!mmc_host_is_spi(host) && + !mmc_ready_for_data(status)) { + err = mmc_blk_fix_state(card, req); + if (err) + goto error_exit; + } - retries = 0; + if (!mrq->cmd->error) + break; + } if (mrq->cmd->error || mrq->data->error || -- 2.34.1 Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv2] mmc: block: fix read single on recovery logic 2022-02-04 15:11 ` [PATCHv2] " Christian Löhle @ 2022-02-08 6:37 ` Adrian Hunter 2022-02-08 15:12 ` Ulf Hansson 0 siblings, 1 reply; 8+ messages in thread From: Adrian Hunter @ 2022-02-08 6:37 UTC (permalink / raw) To: Christian Löhle, Ulf Hansson Cc: linux-mmc, linux-kernel, Avri Altman, stable On 04/02/2022 17:11, Christian Löhle wrote: > On reads with MMC_READ_MULTIPLE_BLOCK that fail, > the recovery handler will use MMC_READ_SINGLE_BLOCK for > each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. > The logic for this is fixed to never report unsuccessful reads > as success to the block layer. > > On command error with retries remaining, blk_update_request was > called with whatever value error was set last to. > In case it was last set to BLK_STS_OK (default), the read will be > reported as success, even though there was no data read from the device. > This could happen on a CRC mismatch for the response, > a card rejecting the command (e.g. again due to a CRC mismatch). > In case it was last set to BLK_STS_IOERR, the error is reported correctly, > but no retries will be attempted. > > Fixes: 81196976ed946c ("mmc: block: Add blk-mq support") > Cc: stable@vger.kernel.org > Signed-off-by: Christian Loehle <cloehle@hyperstone.com> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com> > --- > v2: > - Do not allow data error retries > - Actually retry MMC_READ_SINGLE_RETRIES times instead of > MMC_READ_SINGLE_RETRIES-1 > > > drivers/mmc/core/block.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 4e61b28a002f..8d718aa56d33 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) > struct mmc_card *card = mq->card; > struct mmc_host *host = card->host; > blk_status_t error = BLK_STS_OK; > - int retries = 0; > > do { > u32 status; > int err; > + int retries = 0; > > - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); > + while (retries++ <= MMC_READ_SINGLE_RETRIES) { > + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); > > - mmc_wait_for_req(host, mrq); > + mmc_wait_for_req(host, mrq); > > - err = mmc_send_status(card, &status); > - if (err) > - goto error_exit; > - > - if (!mmc_host_is_spi(host) && > - !mmc_ready_for_data(status)) { > - err = mmc_blk_fix_state(card, req); > + err = mmc_send_status(card, &status); > if (err) > goto error_exit; > - } > > - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) > - continue; > + if (!mmc_host_is_spi(host) && > + !mmc_ready_for_data(status)) { > + err = mmc_blk_fix_state(card, req); > + if (err) > + goto error_exit; > + } > > - retries = 0; > + if (!mrq->cmd->error) > + break; > + } > > if (mrq->cmd->error || > mrq->data->error || > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] mmc: block: fix read single on recovery logic 2022-02-08 6:37 ` Adrian Hunter @ 2022-02-08 15:12 ` Ulf Hansson 0 siblings, 0 replies; 8+ messages in thread From: Ulf Hansson @ 2022-02-08 15:12 UTC (permalink / raw) To: Adrian Hunter, Christian Löhle Cc: linux-mmc, linux-kernel, Avri Altman, stable On Tue, 8 Feb 2022 at 07:37, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 04/02/2022 17:11, Christian Löhle wrote: > > On reads with MMC_READ_MULTIPLE_BLOCK that fail, > > the recovery handler will use MMC_READ_SINGLE_BLOCK for > > each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. > > The logic for this is fixed to never report unsuccessful reads > > as success to the block layer. > > > > On command error with retries remaining, blk_update_request was > > called with whatever value error was set last to. > > In case it was last set to BLK_STS_OK (default), the read will be > > reported as success, even though there was no data read from the device. > > This could happen on a CRC mismatch for the response, > > a card rejecting the command (e.g. again due to a CRC mismatch). > > In case it was last set to BLK_STS_IOERR, the error is reported correctly, > > but no retries will be attempted. > > > > Fixes: 81196976ed946c ("mmc: block: Add blk-mq support") > > Cc: stable@vger.kernel.org > > Signed-off-by: Christian Loehle <cloehle@hyperstone.com> > > Reviewed-by: Adrian Hunter <adrian.hunter@intel.com> Applied for fixes, thanks to both of you! Kind regards Uffe > > > --- > > v2: > > - Do not allow data error retries > > - Actually retry MMC_READ_SINGLE_RETRIES times instead of > > MMC_READ_SINGLE_RETRIES-1 > > > > > > drivers/mmc/core/block.c | 28 ++++++++++++++-------------- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > > index 4e61b28a002f..8d718aa56d33 100644 > > --- a/drivers/mmc/core/block.c > > +++ b/drivers/mmc/core/block.c > > @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) > > struct mmc_card *card = mq->card; > > struct mmc_host *host = card->host; > > blk_status_t error = BLK_STS_OK; > > - int retries = 0; > > > > do { > > u32 status; > > int err; > > + int retries = 0; > > > > - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); > > + while (retries++ <= MMC_READ_SINGLE_RETRIES) { > > + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); > > > > - mmc_wait_for_req(host, mrq); > > + mmc_wait_for_req(host, mrq); > > > > - err = mmc_send_status(card, &status); > > - if (err) > > - goto error_exit; > > - > > - if (!mmc_host_is_spi(host) && > > - !mmc_ready_for_data(status)) { > > - err = mmc_blk_fix_state(card, req); > > + err = mmc_send_status(card, &status); > > if (err) > > goto error_exit; > > - } > > > > - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) > > - continue; > > + if (!mmc_host_is_spi(host) && > > + !mmc_ready_for_data(status)) { > > + err = mmc_blk_fix_state(card, req); > > + if (err) > > + goto error_exit; > > + } > > > > - retries = 0; > > + if (!mrq->cmd->error) > > + break; > > + } > > > > if (mrq->cmd->error || > > mrq->data->error || > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-08 15:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-05 16:43 [PATCH] mmc: block: fix read single on recovery logic Christian Löhle 2022-02-03 10:09 ` Christian Löhle 2022-02-04 9:47 ` Ulf Hansson 2022-02-04 11:26 ` Adrian Hunter 2022-02-04 14:34 ` Christian Löhle 2022-02-04 15:11 ` [PATCHv2] " Christian Löhle 2022-02-08 6:37 ` Adrian Hunter 2022-02-08 15:12 ` Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.