[PATCH] mmc: block: Check for errors after write on SPI

[PATCH] mmc: block: Check for errors after write on SPI

[Christian Löhle]

Introduce a SEND_STATUS check for writes through SPI to not mark
an unsuccessful write as successful.

Since SPI SD/MMC does not have states, after a write, the card will
just hold the line LOW until it is ready again. The driver marks the
write therefore as completed as soon as it reads something other than
all zeroes.
The driver does not distinguish from a card no longer signalling busy
and it being disconnected (and the line being pulled-up by the host).
This lead to writes being marked as successful when disconnecting
a busy card.
Now the card is ensured to be still connected by an additional CMD13,
just like non-SPI is ensured to go back to TRAN state.

While at it and since we already poll for the post-write status anyway,
we might as well check for SPIs error bits (any of them).

The disconnecting card problem is reproducable for me after continuous
write activity and randomly disconnecting, around every 20-50 tries
on SPI DS for some card.

Fixes: 7213d175e3b6f ("MMC/SD card driver learns SPI")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/block.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 4e67c1403cc9..fa34c4bbeebd 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1903,9 +1903,32 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req)
 	struct mmc_blk_busy_data cb_data;
 	int err;
 
-	if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ)
+	if (rq_data_dir(req) == READ)
 		return 0;
 
+	/*
+	 * SPI does not have a TRAN state we have to wait on, instead the
+	 * card is ready again when it no longer holds the line LOW.
+	 * We still have to ensure two things here before we know the write
+	 * was successful:
+	 * 1. The card has not disconnected during busy and we actually read our
+	 * own pull-up, thinking it was still connected, so ensure it
+	 * still responds.
+	 * 2. Check for any error bits, in particular R1_SPI_IDLE to catch a
+	 * just reconnected card after being disconnected during busy.
+	 */
+	if (mmc_host_is_spi(card->host)) {
+		u32 status = 0;
+
+		err = __mmc_send_status(card, &status, 0);
+		/* All R1 and R2 bits of SPI are errors in our case */
+		if (status)
+			err = err ? err : -EIO;
+		if (err)
+			mqrq->brq.data.bytes_xfered = 0;
+		return err;
+	}
+
 	cb_data.card = card;
 	cb_data.status = 0;
 	err = __mmc_poll_for_busy(card->host, 0, MMC_BLK_TIMEOUT_MS,
-- 
2.34.1
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCH] mmc: block: Check for errors after write on SPI

[andriy.shevchenko]

On Tue, Mar 22, 2022 at 04:21:34PM +0000, Christian Löhle wrote:
> Introduce a SEND_STATUS check for writes through SPI to not mark
> an unsuccessful write as successful.
> 
> Since SPI SD/MMC does not have states, after a write, the card will
> just hold the line LOW until it is ready again. The driver marks the
> write therefore as completed as soon as it reads something other than
> all zeroes.
> The driver does not distinguish from a card no longer signalling busy
> and it being disconnected (and the line being pulled-up by the host).
> This lead to writes being marked as successful when disconnecting
> a busy card.
> Now the card is ensured to be still connected by an additional CMD13,
> just like non-SPI is ensured to go back to TRAN state.
> 
> While at it and since we already poll for the post-write status anyway,
> we might as well check for SPIs error bits (any of them).
> 
> The disconnecting card problem is reproducable for me after continuous
> write activity and randomly disconnecting, around every 20-50 tries
> on SPI DS for some card.

...

> +	if (mmc_host_is_spi(card->host)) {
> +		u32 status = 0;

> +		err = __mmc_send_status(card, &status, 0);

> +		/* All R1 and R2 bits of SPI are errors in our case */
> +		if (status)
> +			err = err ? err : -EIO;

I would use either this:

		if (err || status) {
			mqrq->brq.data.bytes_xfered = 0;

			if (err)
				return err;
			return -EIO;
		}

		return 0;

or at least this:

			err = err ?: -EIO;

or even this:

		if (!err && status)
			err = -EIO;

(Personally I would choose the first option)


> +		if (err)
> +			mqrq->brq.data.bytes_xfered = 0;
> +		return err;
> +	}

-- 
With Best Regards,
Andy Shevchenko

[PATCHv2] mmc: block: Check for errors after write on SPI

[Christian Löhle]

Introduce a SEND_STATUS check for writes through SPI to not mark
an unsuccessful write as successful.

Since SPI SD/MMC does not have states, after a write, the card will
just hold the line LOW until it is ready again. The driver marks the
write therefore as completed as soon as it reads something other than
all zeroes.
The driver does not distinguish from a card no longer signalling busy
and it being disconnected (and the line being pulled-up by the host).
This lead to writes being marked as successful when disconnecting
a busy card.
Now the card is ensured to be still connected by an additional CMD13,
just like non-SPI is ensured to go back to TRAN state.

While at it and since we already poll for the post-write status anyway,
we might as well check for SPIs error bits (any of them).

The disconnecting card problem is reproducable for me after continuous
write activity and randomly disconnecting, around every 20-50 tries
on SPI DS for some card.

Fixes: 7213d175e3b6f ("MMC/SD card driver learns SPI")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
v2:
  - Reorder err and status check for err to take precedence and look cleaner
  
 drivers/mmc/core/block.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 4e67c1403cc9..54c2009f398f 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1903,9 +1903,34 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req)
 	struct mmc_blk_busy_data cb_data;
 	int err;
 
-	if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ)
+	if (rq_data_dir(req) == READ)
 		return 0;
 
+	/*
+	 * SPI does not have a TRAN state we have to wait on, instead the
+	 * card is ready again when it no longer holds the line LOW.
+	 * We still have to ensure two things here before we know the write
+	 * was successful:
+	 * 1. The card has not disconnected during busy and we actually read our
+	 * own pull-up, thinking it was still connected, so ensure it
+	 * still responds.
+	 * 2. Check for any error bits, in particular R1_SPI_IDLE to catch a
+	 * just reconnected card after being disconnected during busy.
+	 */
+	if (mmc_host_is_spi(card->host)) {
+		u32 status = 0;
+
+		err = __mmc_send_status(card, &status, 0);
+		/* All R1 and R2 bits of SPI are errors in our case */
+		if (err || status) {
+			mqrq->brq.data.bytes_xfered = 0;
+			if (err)
+				return err;
+			return -EIO;
+		}
+		return 0;
+	}
+
 	cb_data.card = card;
 	cb_data.status = 0;
 	err = __mmc_poll_for_busy(card->host, 0, MMC_BLK_TIMEOUT_MS,
-- 
2.34.1
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCHv2] mmc: block: Check for errors after write on SPI

[Andy Shevchenko]

On Wed, Mar 23, 2022 at 02:12:43PM +0000, Christian Löhle wrote:
> Introduce a SEND_STATUS check for writes through SPI to not mark
> an unsuccessful write as successful.
> 
> Since SPI SD/MMC does not have states, after a write, the card will
> just hold the line LOW until it is ready again. The driver marks the
> write therefore as completed as soon as it reads something other than
> all zeroes.
> The driver does not distinguish from a card no longer signalling busy
> and it being disconnected (and the line being pulled-up by the host).
> This lead to writes being marked as successful when disconnecting
> a busy card.
> Now the card is ensured to be still connected by an additional CMD13,
> just like non-SPI is ensured to go back to TRAN state.
> 
> While at it and since we already poll for the post-write status anyway,
> we might as well check for SPIs error bits (any of them).
> 
> The disconnecting card problem is reproducable for me after continuous
> write activity and randomly disconnecting, around every 20-50 tries
> on SPI DS for some card.

LGTM, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(Next time send a new version of the patch as a separate email thread,
 so tools won't mess with the different versions in one thread. I hope
 no need for resend this time and Ulf handles this)

> Fixes: 7213d175e3b6f ("MMC/SD card driver learns SPI")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
> v2:
>   - Reorder err and status check for err to take precedence and look cleaner
> 
>  drivers/mmc/core/block.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 4e67c1403cc9..54c2009f398f 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1903,9 +1903,34 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req)
>  	struct mmc_blk_busy_data cb_data;
>  	int err;
> 
> -	if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ)
> +	if (rq_data_dir(req) == READ)
>  		return 0;
> 
> +	/*
> +	 * SPI does not have a TRAN state we have to wait on, instead the
> +	 * card is ready again when it no longer holds the line LOW.
> +	 * We still have to ensure two things here before we know the write
> +	 * was successful:
> +	 * 1. The card has not disconnected during busy and we actually read our
> +	 * own pull-up, thinking it was still connected, so ensure it
> +	 * still responds.
> +	 * 2. Check for any error bits, in particular R1_SPI_IDLE to catch a
> +	 * just reconnected card after being disconnected during busy.
> +	 */
> +	if (mmc_host_is_spi(card->host)) {
> +		u32 status = 0;
> +
> +		err = __mmc_send_status(card, &status, 0);
> +		/* All R1 and R2 bits of SPI are errors in our case */
> +		if (err || status) {
> +			mqrq->brq.data.bytes_xfered = 0;
> +			if (err)
> +				return err;
> +			return -EIO;
> +		}
> +		return 0;
> +	}
> +
>  	cb_data.card = card;
>  	cb_data.status = 0;
>  	err = __mmc_poll_for_busy(card->host, 0, MMC_BLK_TIMEOUT_MS,
> --
> 2.34.1
> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
> Managing Director: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv2] mmc: block: Check for errors after write on SPI

[Ulf Hansson]

On Wed, 23 Mar 2022 at 15:12, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> Introduce a SEND_STATUS check for writes through SPI to not mark
> an unsuccessful write as successful.
>
> Since SPI SD/MMC does not have states, after a write, the card will
> just hold the line LOW until it is ready again. The driver marks the
> write therefore as completed as soon as it reads something other than
> all zeroes.
> The driver does not distinguish from a card no longer signalling busy
> and it being disconnected (and the line being pulled-up by the host).
> This lead to writes being marked as successful when disconnecting
> a busy card.
> Now the card is ensured to be still connected by an additional CMD13,
> just like non-SPI is ensured to go back to TRAN state.
>
> While at it and since we already poll for the post-write status anyway,
> we might as well check for SPIs error bits (any of them).
>
> The disconnecting card problem is reproducable for me after continuous
> write activity and randomly disconnecting, around every 20-50 tries
> on SPI DS for some card.
>
> Fixes: 7213d175e3b6f ("MMC/SD card driver learns SPI")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
> v2:
>   - Reorder err and status check for err to take precedence and look cleaner
>
>  drivers/mmc/core/block.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 4e67c1403cc9..54c2009f398f 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1903,9 +1903,34 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req)
>         struct mmc_blk_busy_data cb_data;
>         int err;
>
> -       if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ)
> +       if (rq_data_dir(req) == READ)
>                 return 0;
>
> +       /*
> +        * SPI does not have a TRAN state we have to wait on, instead the
> +        * card is ready again when it no longer holds the line LOW.
> +        * We still have to ensure two things here before we know the write
> +        * was successful:
> +        * 1. The card has not disconnected during busy and we actually read our
> +        * own pull-up, thinking it was still connected, so ensure it
> +        * still responds.
> +        * 2. Check for any error bits, in particular R1_SPI_IDLE to catch a
> +        * just reconnected card after being disconnected during busy.
> +        */
> +       if (mmc_host_is_spi(card->host)) {
> +               u32 status = 0;
> +
> +               err = __mmc_send_status(card, &status, 0);
> +               /* All R1 and R2 bits of SPI are errors in our case */
> +               if (err || status) {
> +                       mqrq->brq.data.bytes_xfered = 0;
> +                       if (err)
> +                               return err;
> +                       return -EIO;
> +               }
> +               return 0;
> +       }

Nitpick: Would you mind moving the above spi specific code into a
separate function instead?

> +
>         cb_data.card = card;
>         cb_data.status = 0;
>         err = __mmc_poll_for_busy(card->host, 0, MMC_BLK_TIMEOUT_MS,

Other than the above, this looks good to me.

Kind regards
Uffe