On 23/07/22 18:08, Christian Loehle wrote: >> 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@xxxxxxxxxxxxxx> >>> --- >>> 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. The expectation is that the driver sets bytes_xfered correctly, based controller errors. The driver is not expected to check for status errors, hence in that case the bytes_xfered is set to 0 by error recovery. > 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. That sounds like it would be a good improvement to have. > 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. I am not sure what you mean. Isn't it OK to check CMD13 response for SPI?
