linux-kernel - Re: [PATCHv2] mmc: block: Add single read for 4k sector cards

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e7ebb779-9938-d5f7-d900-dbea55b4ac72@intel.com>
Date:   Tue, 28 Jun 2022 14:13:36 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Christian Löhle <CLoehle@...erstone.com>,
        Avri Altman <Avri.Altman@....com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2] mmc: block: Add single read for 4k sector cards

On 28/06/22 12:08, Christian Löhle wrote:
>> Cards with 4k native sector size may only be read 4k-aligned,
>>> accommodate for this in the single read recovery and use it.
>>
>> Thanks for the patch.
>>
>>>
>>> Fixes: 81196976ed946 (mmc: block: Add blk-mq support)
>>> Signed-off-by: Christian Loehle <cloehle@...erstone.com>
>>
>> FYI checkpatch says:
>>
>> WARNING: From:/Signed-off-by: email name mismatch: 'From: "Christian Löhle" <CLoehle@...erstone.com>' != 'Signed-off-by: Christian Loehle <cloehle@...erstone.com>'
> 
> Will be fixed in my future patches, thanks for the hint.
> 
>>
>>> ---
>>>  drivers/mmc/core/block.c | 25 ++++++++++++-------------
>>>  1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>>> index f4a1281658db..a75a208ce203 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,8 @@ 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)
>>> -			brq->data.blocks = 1;
>>> +		if (recovery_mode)
>>> +			brq->data.blocks = mmc_large_sector(card) ? 8 : 1;
>>
>> I suggest changing to use queue_physical_block_size() here and further below
> 
> This part I'm impartial about, not sure if it makes it more readable, hopefully we never have to support another "native sector size" apart from the two.
> Anyway I will send the next patch with queue_physical_block_size()
> 
>>
>> 			brq->data.blocks = queue_physical_block_size(req->q) >> SECTOR_SHIFT;
> 
> Do we want to switch to SECTOR_SHIFT instead of 9? So far SECTOR_SHIFT is not used at all in mmc core.

I guess '9' is more consistent

> If so I would go ahead and change all the others in another patch:
> queue.c:187:	q->limits.discard_granularity = card->pref_erase << 9;
> core.c:103:	data->bytes_xfered = (prandom_u32() % (data->bytes_xfered >> 9)) << 9;
> mmc.c:792:MMC_DEV_ATTR(erase_size, "%u\n", card->erase_size << 9);
> mmc.c:793:MMC_DEV_ATTR(preferred_erase_size, "%u\n", card->pref_erase << 9);
> mmc_test.c:1557:	sz = (unsigned long)test->card->pref_erase << 9;
> mmc_test.c:1570:		t->max_tfr = test->card->host->max_blk_count << 9;
> mmc_test.c:2461:	if (repeat_cmd && (t->blocks + 1) << 9 > t->max_tfr)
> sd.c:707:MMC_DEV_ATTR(erase_size, "%u\n", card->erase_size << 9);
> sd.c:708:MMC_DEV_ATTR(preferred_erase_size, "%u\n", card->pref_erase << 9);
> block.c:1417:		int i, data_size = brq->data.blocks << 9;
> block.c:1851:			brq->data.bytes_xfered = blocks << 9;
> 
> 
> 
> 
> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
> Managing Director: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782