On 3/01/23 22:59, Shyam Saini wrote:
>
> Hi Adrian,
>
> Thank you for response.
>
>> On 20/12/22 01:41, Shyam Saini wrote:
>>> As per the JEDEC spec, tuning(command CMD21) is not allowed in
>>> RPMB partition.
>>>
>>> To avoid retuning while switching to RPMB, hold_retune variable was
>>> introduced
>>
>> No, mmc_retune_pause() is used:
>>
>> /*
>> * Pause re-tuning for a small set of operations. The pause begins after the
>> * next command and after first doing re-tuning.
>> */
>> void mmc_retune_pause(struct mmc_host *host)
>> {
>> if (!host->retune_paused) {
>> host->retune_paused = 1;
>> mmc_retune_needed(host);
>> mmc_retune_hold(host);
>> }
>> }
>>
>>> but it is not taken into account while making the tuning
>>> decision. As a consequence of this, mmc driver aborts while switching to
>>> RPMB partition:
>>> mmc0: mmc_hs400_to_hs200 failed, error -84
>>
>> Do you normally re-tune at all? It could just be that re-tuning
>> doesn't work.
>>
>
> Yes, we are able to retune.
>
> In fact this bug occurs when we iteratively switch to RPMB partition.
>
> May be related to this, we also observed that in __mmc_blk_ioctl_cmd
> function in line 487[1] part_index is assigned target_part variable and
> on the next line EXT_CSD_PART_CONFIG_ACC_RPMB OR'ed to the target_part
> variable.
>
> In mmc_blk_part_switch_pre function line 831 [2], part_type variable is
> compared to EXT_CSD_PART_CONFIG_ACC_RPMB without taking into account that
> the part_index variable is also OR'ed and its not separated before
> the comparision.
>
> Same thing happens in mmc_blk_part_switch_post function.
>
> Is it expected to be this way?, please let me know.
According to the spec, there can only be 1 RPMB partition,
so the comment and code are not very meaningful.
AFAICT rpmb->part_index is set to EXT_CSD_PART_CONFIG_ACC_RPMB
anyway, originally by mmc_decode_ext_csd():
/*
* RPMB regions are defined in multiples of 128K.
*/
card->ext_csd.raw_rpmb_size_mult = ext_csd[EXT_CSD_RPMB_MULT];
if (ext_csd[EXT_CSD_RPMB_MULT] && mmc_host_cmd23(card->host)) {
mmc_part_add(card, ext_csd[EXT_CSD_RPMB_MULT] << 17,
EXT_CSD_PART_CONFIG_ACC_RPMB,
"rpmb", 0, false,
MMC_BLK_DATA_AREA_RPMB);
}
So the OR has no effect.
As for "mmc_hs400_to_hs200 failed, error -84", I suggest enabling
some dynamic debug messages to determine what command is getting
an error and what the I/O state (as per mmc_set_ios()) is.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git/tree/drivers/mmc/core/block.c?h=next#n487
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git/tree/drivers/mmc/core/block.c?h=next#n831
>
>>>
>>> To fix this, take hold_retune variable into account while making retune
>>> decision in mmc_retune() function.
>>>
>>> Fixes: 57da0c042f4a ("mmc: block: Pause re-tuning while switched to the RPMB partition")
>>> Reported-by: Judy Wang <wangjudy@xxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Shyam Saini <shyamsaini@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/mmc/core/host.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>> index b89dca1f15e9..342c1f5c256b 100644
>>> --- a/drivers/mmc/core/host.c
>>> +++ b/drivers/mmc/core/host.c
>>> @@ -181,6 +181,9 @@ int mmc_retune(struct mmc_host *host)
>>> bool return_to_hs400 = false;
>>> int err;
>>>
>>> + if (host->hold_retune >= 1)
>>> + return 0;
>>
>> No, hold_retune is set *before* the command *after* which
>> tuning is not permitted.
>>
>>> +
>>> if (host->retune_now)
>>> host->retune_now = 0;
>>> else
>>> --
>>> 2.34.1
>>>
>> On 20/12/22 01:41, Shyam Saini wrote:
>>> Hi All,
>>>
>>> Cc'ing Tyler
>>>
>>> Please note that I am not 100 % sure about the fix.
>>> This issue is reproducible without this patch and the patch at least fix the issue without
>>> any regressions on our side.
>>>
>>> We observed that hold_retune is always greater or equal to 1 in mmc_retune() function,
>>>
>>> so it may always avoid re-tuning when it is actually needed and there may
>>>
>>> be a better fix for the issue.
>>>
>>> Please let me know your any feedback or comments.
>>>
>>> Best Regards,
>>> Shyam
>>>
>>>
>>
>>
