Re: [PATCH] mmc: block: Add single read for 4k sector cards

[Christian Löhle]

>Hi,
>
>> Cards with 4k native sector size may only be read 4k-aligned,
>> accommodate for this in the single read recovery and use it.
>
>Fixes: 2bf22b39823c (mmc: core: add workaround for controllers with broken multiblock reads)
If a Fixes tag is appropriate it should be mmc: block: Add blk-mq support, right?
(The commit introducing the FIXME)

>> 
>> Signed-off-by: Christian Loehle <cloehle@xxxxxxxxxxxxxx>
>> ---
>>  drivers/mmc/core/block.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index f4a1281658db..6f0b24cb0747 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -176,7 +176,7 @@ static inline int mmc_blk_part_switch(struct
>> mmc_card *card,
>>                                       unsigned int part_type);
>>  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>>                                struct mmc_card *card,
>> -                              int disable_multi,
>> +                              int recovery_mode,
>>                                struct mmc_queue *mq);
>>  static void mmc_blk_hsq_req_done(struct mmc_request *mrq);
>> 
>> @@ -1302,7 +1302,7 @@ static void mmc_blk_eval_resp_error(struct
>> mmc_blk_request *brq)
>>  }
>> 
>>  static void mmc_blk_data_prep(struct mmc_queue *mq, struct
>> mmc_queue_req *mqrq,
>> -                             int disable_multi, bool *do_rel_wr_p,
>> +                             int recovery_mode, bool *do_rel_wr_p,
>>                               bool *do_data_tag_p)
>>  {
>>         struct mmc_blk_data *md = mq->blkdata;
>> @@ -1372,8 +1372,11 @@ static void mmc_blk_data_prep(struct
>> mmc_queue *mq, struct mmc_queue_req *mqrq,
>>                  * at a time in order to accurately determine which
>>                  * sectors can be read successfully.
>>                  */
>> -               if (disable_multi)
>> +               if (recovery_mode) {
>>                         brq->data.blocks = 1;
>> +                       if (mmc_large_sector(card))
>> +                               brq->data.blocks = 8;
>Maybe a ternary operator here?
> 
Sure!
>> +               }
>> 
>>                 /*
>>                  * Some controllers have HW issues while operating
>> @@ -1590,7 +1593,7 @@ static int mmc_blk_cqe_issue_rw_rq(struct
>> mmc_queue *mq, struct request *req)
>> 
>>  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>>                                struct mmc_card *card,
>> -                              int disable_multi,
>> +                              int recovery_mode,
>>                                struct mmc_queue *mq)
>>  {
>>         u32 readcmd, writecmd;
>> @@ -1599,7 +1602,7 @@ static void mmc_blk_rw_rq_prep(struct
>> mmc_queue_req *mqrq,
>>         struct mmc_blk_data *md = mq->blkdata;
>>         bool do_rel_wr, do_data_tag;
>> 
>> -       mmc_blk_data_prep(mq, mqrq, disable_multi, &do_rel_wr,
>> &do_data_tag);
>> +       mmc_blk_data_prep(mq, mqrq, recovery_mode, &do_rel_wr,
>> &do_data_tag);
>> 
>>         brq->mrq.cmd = &brq->cmd;
>> 
>> @@ -1690,7 +1693,7 @@ static int mmc_blk_fix_state(struct mmc_card
>> *card, struct request *req)
>> 
>>  #define MMC_READ_SINGLE_RETRIES        2
>> 
>> -/* Single sector read during recovery */
>> +/* Single (native) sector read during recovery */
>>  static void mmc_blk_read_single(struct mmc_queue *mq, struct request
>> *req)
>>  {
>>         struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
>> @@ -1698,6 +1701,7 @@ 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;
>> +       size_t bytes_per_read = mmc_large_sector(card) ? 4069 : 512;
>Typo: 4069 -> 4096
Oh wow, thanks, not sure how I missed that, in that case it was not just sloppy testing,
but a manual rewrite from the kernel source for the board I currently have an eMMC with 4k native sector size on.
(So also sloppy testing)

>
>Thanks,
>Avri
>> 
>>         do {
>>                 u32 status;
>> @@ -1732,13 +1736,13 @@ static void mmc_blk_read_single(struct
>> mmc_queue *mq, struct request *req)
>>                 else
>>                         error = BLK_STS_OK;
>> 
>> -       } while (blk_update_request(req, error, 512));
>> +       } while (blk_update_request(req, error, bytes_per_read));
>> 
>>         return;
>> 
>>  error_exit:
>>         mrq->data->bytes_xfered = 0;
>> -       blk_update_request(req, BLK_STS_IOERR, 512);
>> +       blk_update_request(req, BLK_STS_IOERR, bytes_per_read);
>>         /* Let it try the remaining request again */
>>         if (mqrq->retries > MMC_MAX_RETRIES - 1)
>>                 mqrq->retries = MMC_MAX_RETRIES - 1;
>> @@ -1879,10 +1883,8 @@ static void mmc_blk_mq_rw_recovery(struct
>> mmc_queue *mq, struct request *req)
>>                 return;
>>         }
>> 
>> -       /* FIXME: Missing single sector read for large sector size */
>> -       if (!mmc_large_sector(card) && rq_data_dir(req) == READ &&
>> -           brq->data.blocks > 1) {
>> -               /* Read one sector at a time */
>> +       if (rq_data_dir(req) == READ && brq->data.blocks > 1) {
>> +               /* Read one (native) sector at a time */
>>                 mmc_blk_read_single(mq, req);
>>                 return;
>>         }
>> --
>> 2.36.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