linux-kernel - RE: [PATCH] mmc: block: Dont report successful writes with errors

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: <a66fe29560f545e18b91af57f4d0dddb@hyperstone.com>
Date:   Sat, 23 Jul 2022 15:08:19 +0000
From:   Christian Loehle <CLoehle@...erstone.com>
To:     Adrian Hunter <adrian.hunter@...el.com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        Avri Altman <Avri.Altman@....com>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        Linux MMC List <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] mmc: block: Dont report successful writes with errors

>On 19/07/22 18:34, Christian Loehle wrote:
>> Be as conservative about successful write reporting to the block layer 
>> for SPI as with normal SD and MMC.
>> That means on any errors bytes_xfered is ignored and the whole write 
>> must be repeated.
>> 
>> Signed-off-by: Christian Loehle <cloehle@...erstone.com>
>> ---
>>  drivers/mmc/core/block.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 
>> f4a1281658db..63d1c05582a9 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -1765,8 +1765,12 @@ static bool mmc_blk_status_error(struct request *req, u32 status)
>>  	struct mmc_queue *mq = req->q->queuedata;
>>  	u32 stop_err_bits;
>>  
>> +	/*
>> +	 * Either write timed out during busy and data->error is set
>> +	 * or we actually received a valid R2 and check for error bits.
>> +	 */
>>  	if (mmc_host_is_spi(mq->card->host))
>> -		return false;
>> +		return brq->data.error || !!status;
>
>This function is for checking status, so brq->data.error does not belong here.  Also it would be more readable to use a define e.g.
>
>		return status & SPI_R2_ERRORS;
>
>I think clearing bytes_xfered for SPI brq->data.error should be a separate patch, and you would need to explain a bit more for that case too.

I understand that, but there is no way of checking status in SPI mode.
The behavior of mmc block is only changed in a minor way here anyway, that is, checking for status is done one more time than before.
If brq->data.error is set directly after a write e.g. then bytes_xfered is already 0.
My intention was mostly to improve on the flow of the recovery and get the mmc_is_host_spi out of there for the most part with future patches.
After all it feels weird to do a single step read retry before ensuring a fix_state, and I ran into that quite often.
Unfortunately, I now realized that fix_state cannot properly be implemented within the spec or even real-world card's behavior and I won't be taking this further.
The best attempt I came up with is doing a loop of CMD12 and CMD13 in SPI and if CMD12 was ILLEGAL and CMD12 has no bits set, state is fixed.
But CMD12 is only defined for multiple block transfers in SPI and cards treat it differently on e.g. CMD17 transfers.
Instead I would just do a soft reset for SPI and retry and maybe increase the read timeout of 100ms which larger SD cards can fail sometimes.
Anyway since SPI initialization is quite fast, especially for soft resets there is likely no recovery to beat that performance-wise.
I will send an RFC for the soft reset in the coming days.
If not I would at least add the !mmc_is_host_spi condition for calling mmc_blk_status_error to make it a bit more clear that this function does do what is intended for SPI cards.

Regards,
Christian

Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782