Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] mmc: block: fix read single on recovery logic
To:     Ulf Hansson <ulf.hansson@linaro.org>,
        =?UTF-8?Q?Christian_L=c3=b6hle?= <CLoehle@hyperstone.com>
Cc:     "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Avri Altman <avri.altman@wdc.com>
References: <5e5f2e45d0a14a55a8b7a9357846114b@hyperstone.com>
 <7c4757cc707740e580c61c39f963a04d@hyperstone.com>
 <CAPDyKFr0YXCwL-8F9M7mkpNzSQpzw6gNUq2zaiJEXj1jNxUbrg@mail.gmail.com>
From:   Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
Message-ID: <5c66833d-4b35-2c76-db54-0306e08843e5@intel.com>
Date:   Fri, 4 Feb 2022 13:26:09 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <CAPDyKFr0YXCwL-8F9M7mkpNzSQpzw6gNUq2zaiJEXj1jNxUbrg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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