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;
Message-ID: <e7ebb779-9938-d5f7-d900-dbea55b4ac72@intel.com>
Date:   Tue, 28 Jun 2022 14:13:36 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.9.1
Subject: Re: [PATCHv2] mmc: block: Add single read for 4k sector cards
Content-Language: en-US
To:     =?UTF-8?Q?Christian_L=c3=b6hle?= <CLoehle@hyperstone.com>,
        Avri Altman <Avri.Altman@wdc.com>,
        "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <e0cce326393645d3b4a163ce65c89fb9@hyperstone.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
In-Reply-To: <e0cce326393645d3b4a163ce65c89fb9@hyperstone.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@hyperstone.com>
>>
>> FYI checkpatch says:
>>
>> WARNING: From:/Signed-off-by: email name mismatch: 'From: "Christian Löhle" <CLoehle@hyperstone.com>' != 'Signed-off-by: Christian Loehle <cloehle@hyperstone.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