[PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Shawn Lin]

We to some extent should tolerate R1_OUT_OF_RANGE for open-ending
mode as it is expected behaviour and most of the backup partition
tables should be located near some of the last blocks which will
always make open-ending read exceed the capacity of cards.

Fixes: 9820a5b11101 ("mmc: core: for data errors, take response of stop cmd into account")
Fixes: a04e6bae9e6f ("mmc: core: check also R1 response for stop commands")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Tested-by: Shawn Guo <shawnguo@kernel.org>
Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---

Changes in v2:
- fix a typo and introduce STOP_ERROR
- reword the comment and always include a description from the spec if possible

 drivers/mmc/core/block.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index a11bead..38267a0 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1360,18 +1360,34 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
 	}
 }
 
-#define CMD_ERRORS							\
-	(R1_OUT_OF_RANGE |	/* Command argument out of range */	\
-	 R1_ADDRESS_ERROR |	/* Misaligned address */		\
+#define STOP_ERRORS							\
+	(R1_ADDRESS_ERROR |	/* Misaligned address */		\
 	 R1_BLOCK_LEN_ERROR |	/* Transferred block length incorrect */\
 	 R1_WP_VIOLATION |	/* Tried to write to protected block */	\
 	 R1_CARD_ECC_FAILED |	/* Card ECC failed */			\
 	 R1_CC_ERROR |		/* Card controller error */		\
 	 R1_ERROR)		/* General/unknown error */
 
-static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
+#define CMD_ERRORS (STOP_ERRORS | \
+		    R1_OUT_OF_RANGE) /* Command argument out of range */
+
+static bool mmc_blk_has_cmd_err(struct mmc_card *card, struct mmc_command *cmd)
 {
-	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
+	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
+	u32 error = (md->flags & MMC_BLK_CMD23) ? CMD_ERRORS : STOP_ERRORS;
+
+	/*
+	 * Per the SD specification(physical layer version 4.10),
+	 * section 4.3.3, it explicitly states that "When the last
+	 * block of user area is read using CMD18, the host should
+	 * ignore OUT_OF_RANGE error that may occur even the sequence
+	 * is correct". And JESD84-B51 for eMMC also has a similar
+	 * statement on section 6.8.3. As CMD23 is optional for either
+	 * cards or hosts, so we need to check the MMC_BLK_CMD23 flag
+	 * to prevent the OUT_OF_RANGE error for open-ending multiple
+	 * block operations as it's normal behaviour.
+	 */
+	if (!cmd->error && cmd->resp[0] & error)
 		cmd->error = -EIO;
 
 	return cmd->error;
@@ -1398,8 +1414,8 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 	 * stop.error indicates a problem with the stop command.  Data
 	 * may have been transferred, or may still be transferring.
 	 */
-	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
-	    brq->data.error) {
+	if (brq->sbc.error || brq->cmd.error ||
+	    mmc_blk_has_cmd_err(card, &brq->stop) || brq->data.error) {
 		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
 		case ERR_RETRY:
 			return MMC_BLK_RETRY;
-- 
1.9.1

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Ulf Hansson]

On 8 August 2017 at 02:27, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> We to some extent should tolerate R1_OUT_OF_RANGE for open-ending
> mode as it is expected behaviour and most of the backup partition
> tables should be located near some of the last blocks which will
> always make open-ending read exceed the capacity of cards.
>
> Fixes: 9820a5b11101 ("mmc: core: for data errors, take response of stop cmd into account")
> Fixes: a04e6bae9e6f ("mmc: core: check also R1 response for stop commands")
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Tested-by: Shawn Guo <shawnguo@kernel.org>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks, applied for fixes!

Kind regards
Uffe

> ---
>
> Changes in v2:
> - fix a typo and introduce STOP_ERROR
> - reword the comment and always include a description from the spec if possible
>
>  drivers/mmc/core/block.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index a11bead..38267a0 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1360,18 +1360,34 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
>         }
>  }
>
> -#define CMD_ERRORS                                                     \
> -       (R1_OUT_OF_RANGE |      /* Command argument out of range */     \
> -        R1_ADDRESS_ERROR |     /* Misaligned address */                \
> +#define STOP_ERRORS                                                    \
> +       (R1_ADDRESS_ERROR |     /* Misaligned address */                \
>          R1_BLOCK_LEN_ERROR |   /* Transferred block length incorrect */\
>          R1_WP_VIOLATION |      /* Tried to write to protected block */ \
>          R1_CARD_ECC_FAILED |   /* Card ECC failed */                   \
>          R1_CC_ERROR |          /* Card controller error */             \
>          R1_ERROR)              /* General/unknown error */
>
> -static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
> +#define CMD_ERRORS (STOP_ERRORS | \
> +                   R1_OUT_OF_RANGE) /* Command argument out of range */
> +
> +static bool mmc_blk_has_cmd_err(struct mmc_card *card, struct mmc_command *cmd)
>  {
> -       if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
> +       struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
> +       u32 error = (md->flags & MMC_BLK_CMD23) ? CMD_ERRORS : STOP_ERRORS;
> +
> +       /*
> +        * Per the SD specification(physical layer version 4.10),
> +        * section 4.3.3, it explicitly states that "When the last
> +        * block of user area is read using CMD18, the host should
> +        * ignore OUT_OF_RANGE error that may occur even the sequence
> +        * is correct". And JESD84-B51 for eMMC also has a similar
> +        * statement on section 6.8.3. As CMD23 is optional for either
> +        * cards or hosts, so we need to check the MMC_BLK_CMD23 flag
> +        * to prevent the OUT_OF_RANGE error for open-ending multiple
> +        * block operations as it's normal behaviour.
> +        */
> +       if (!cmd->error && cmd->resp[0] & error)
>                 cmd->error = -EIO;
>
>         return cmd->error;
> @@ -1398,8 +1414,8 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
>          * stop.error indicates a problem with the stop command.  Data
>          * may have been transferred, or may still be transferring.
>          */
> -       if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
> -           brq->data.error) {
> +       if (brq->sbc.error || brq->cmd.error ||
> +           mmc_blk_has_cmd_err(card, &brq->stop) || brq->data.error) {
>                 switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
>                 case ERR_RETRY:
>                         return MMC_BLK_RETRY;
> --
> 1.9.1
>
>

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Tue, Aug 08, 2017 at 12:37:31PM +0200, Ulf Hansson wrote:
> On 8 August 2017 at 02:27, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> > We to some extent should tolerate R1_OUT_OF_RANGE for open-ending
> > mode as it is expected behaviour and most of the backup partition
> > tables should be located near some of the last blocks which will
> > always make open-ending read exceed the capacity of cards.
> >
> > Fixes: 9820a5b11101 ("mmc: core: for data errors, take response of stop cmd into account")
> > Fixes: a04e6bae9e6f ("mmc: core: check also R1 response for stop commands")
> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > Tested-by: Shawn Guo <shawnguo@kernel.org>
> > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks, applied for fixes!

Ulf, I would like some more discussion about this patch but need time to
think about it some more. I'll try to have my answer ready by this
evening, but I'd appreciate to have at least one day to respond to a
patch. Otherwise it is difficult for me to focus on the other work I
have. Could that meet your workflow?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Ulf Hansson]

On 8 August 2017 at 13:47, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Tue, Aug 08, 2017 at 12:37:31PM +0200, Ulf Hansson wrote:
>> On 8 August 2017 at 02:27, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> > We to some extent should tolerate R1_OUT_OF_RANGE for open-ending
>> > mode as it is expected behaviour and most of the backup partition
>> > tables should be located near some of the last blocks which will
>> > always make open-ending read exceed the capacity of cards.
>> >
>> > Fixes: 9820a5b11101 ("mmc: core: for data errors, take response of stop cmd into account")
>> > Fixes: a04e6bae9e6f ("mmc: core: check also R1 response for stop commands")
>> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> > Tested-by: Shawn Guo <shawnguo@kernel.org>
>> > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>
>> Thanks, applied for fixes!
>
> Ulf, I would like some more discussion about this patch but need time to
> think about it some more. I'll try to have my answer ready by this
> evening, but I'd appreciate to have at least one day to respond to a
> patch. Otherwise it is difficult for me to focus on the other work I
> have. Could that meet your workflow?

My apologies, trying to catch up with everything and I was too fast. :-)

I have dropped the change from my fixes and next branch. Awaiting an
ack/reviewed by from you first before I apply again.

Kind regards
Uffe

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Shawn Lin]

On 2017/8/8 8:27, Shawn Lin wrote:
> We to some extent should tolerate R1_OUT_OF_RANGE for open-ending
> mode as it is expected behaviour and most of the backup partition
> tables should be located near some of the last blocks which will
> always make open-ending read exceed the capacity of cards.
> 
> Fixes: 9820a5b11101 ("mmc: core: for data errors, take response of stop cmd into account")
> Fixes: a04e6bae9e6f ("mmc: core: check also R1 response for stop commands")
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Tested-by: Shawn Guo <shawnguo@kernel.org>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Just ping to make sure this fix won't be missing from the TODO list.


> ---
> 
> Changes in v2:
> - fix a typo and introduce STOP_ERROR
> - reword the comment and always include a description from the spec if possible
> 
>   drivers/mmc/core/block.c | 30 +++++++++++++++++++++++-------
>   1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index a11bead..38267a0 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1360,18 +1360,34 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
>   	}
>   }
>   
> -#define CMD_ERRORS							\
> -	(R1_OUT_OF_RANGE |	/* Command argument out of range */	\
> -	 R1_ADDRESS_ERROR |	/* Misaligned address */		\
> +#define STOP_ERRORS							\
> +	(R1_ADDRESS_ERROR |	/* Misaligned address */		\
>   	 R1_BLOCK_LEN_ERROR |	/* Transferred block length incorrect */\
>   	 R1_WP_VIOLATION |	/* Tried to write to protected block */	\
>   	 R1_CARD_ECC_FAILED |	/* Card ECC failed */			\
>   	 R1_CC_ERROR |		/* Card controller error */		\
>   	 R1_ERROR)		/* General/unknown error */
>   
> -static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
> +#define CMD_ERRORS (STOP_ERRORS | \
> +		    R1_OUT_OF_RANGE) /* Command argument out of range */
> +
> +static bool mmc_blk_has_cmd_err(struct mmc_card *card, struct mmc_command *cmd)
>   {
> -	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
> +	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
> +	u32 error = (md->flags & MMC_BLK_CMD23) ? CMD_ERRORS : STOP_ERRORS;
> +
> +	/*
> +	 * Per the SD specification(physical layer version 4.10),
> +	 * section 4.3.3, it explicitly states that "When the last
> +	 * block of user area is read using CMD18, the host should
> +	 * ignore OUT_OF_RANGE error that may occur even the sequence
> +	 * is correct". And JESD84-B51 for eMMC also has a similar
> +	 * statement on section 6.8.3. As CMD23 is optional for either
> +	 * cards or hosts, so we need to check the MMC_BLK_CMD23 flag
> +	 * to prevent the OUT_OF_RANGE error for open-ending multiple
> +	 * block operations as it's normal behaviour.
> +	 */
> +	if (!cmd->error && cmd->resp[0] & error)
>   		cmd->error = -EIO;
>   
>   	return cmd->error;
> @@ -1398,8 +1414,8 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
>   	 * stop.error indicates a problem with the stop command.  Data
>   	 * may have been transferred, or may still be transferring.
>   	 */
> -	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
> -	    brq->data.error) {
> +	if (brq->sbc.error || brq->cmd.error ||
> +	    mmc_blk_has_cmd_err(card, &brq->stop) || brq->data.error) {
>   		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
>   		case ERR_RETRY:
>   			return MMC_BLK_RETRY;
> 

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3073 bytes --]

Hi,

really sorry for the delay! But now, here are my thoughts:

> +	/*
> +	 * Per the SD specification(physical layer version 4.10),
> +	 * section 4.3.3, it explicitly states that "When the last
> +	 * block of user area is read using CMD18, the host should
> +	 * ignore OUT_OF_RANGE error that may occur even the sequence

missing "if"? ..."even if the sequence"... Yeah, it is missing in the
specs, too, but still.

> +	 * is correct". And JESD84-B51 for eMMC also has a similar
> +	 * statement on section 6.8.3. As CMD23 is optional for either
> +	 * cards or hosts, so we need to check the MMC_BLK_CMD23 flag
> +	 * to prevent the OUT_OF_RANGE error for open-ending multiple
> +	 * block operations as it's normal behaviour.
> +	 */

I really like adding this comment. Yet, I don't really get why you check
for CMD23, though, since the SD specs say CMD18? As I understand it,
this is true for all multiblock accesses, so we could also do something
like this (only build tested)?

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index f1bbfd389367ff..e83d8291ad4f99 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1362,6 +1362,14 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
 	}
 }
 
+#define STOP_ERRORS							\
+	 (R1_ADDRESS_ERROR |	/* Misaligned address */		\
+	 R1_BLOCK_LEN_ERROR |	/* Transferred block length incorrect */\
+	 R1_WP_VIOLATION |	/* Tried to write to protected block */	\
+	 R1_CARD_ECC_FAILED |	/* Card ECC failed */			\
+	 R1_CC_ERROR |		/* Card controller error */		\
+	 R1_ERROR)		/* General/unknown error */
+
 #define CMD_ERRORS							\
 	(R1_OUT_OF_RANGE |	/* Command argument out of range */	\
 	 R1_ADDRESS_ERROR |	/* Misaligned address */		\
@@ -1371,9 +1379,9 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
 	 R1_CC_ERROR |		/* Card controller error */		\
 	 R1_ERROR)		/* General/unknown error */
 
-static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
+static bool mmc_blk_has_cmd_err(struct mmc_command *cmd, u32 err_mask)
 {
-	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
+	if (!cmd->error && cmd->resp[0] & err_mask)
 		cmd->error = -EIO;
 
 	return cmd->error;
@@ -1400,7 +1408,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 	 * stop.error indicates a problem with the stop command.  Data
 	 * may have been transferred, or may still be transferring.
 	 */
-	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
+	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop, STOP_ERRORS) ||
 	    brq->data.error) {
 		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
 		case ERR_RETRY:

Note that I decided to not couple STOP_ERRORS and CMD_ERRORS in case we
need more adjustments in the future. And your comment needs to be added
somewhere, too. It is just to show what I mean.

Do you think this could work, too? Or am I missing something?

Thanks and kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Shawn Lin]

Hi,

On 2017/8/15 1:58, Wolfram Sang wrote:
> Hi,
> 
> really sorry for the delay! But now, here are my thoughts:
> 
>> +	/*
>> +	 * Per the SD specification(physical layer version 4.10),
>> +	 * section 4.3.3, it explicitly states that "When the last
>> +	 * block of user area is read using CMD18, the host should
>> +	 * ignore OUT_OF_RANGE error that may occur even the sequence
> 
> missing "if"? ..."even if the sequence"... Yeah, it is missing in the
> specs, too, but still.
> 

Aha, yes.

>> +	 * is correct". And JESD84-B51 for eMMC also has a similar
>> +	 * statement on section 6.8.3. As CMD23 is optional for either
>> +	 * cards or hosts, so we need to check the MMC_BLK_CMD23 flag
>> +	 * to prevent the OUT_OF_RANGE error for open-ending multiple
>> +	 * block operations as it's normal behaviour.
>> +	 */
> 
> I really like adding this comment. Yet, I don't really get why you check
> for CMD23, though, since the SD specs say CMD18? As I understand it,
> this is true for all multiblock accesses, so we could also do something
> like this (only build tested)?

Nope, that is true only for open-ending mode.

Per the SD specification(physical layer version 4.10), section 4.15 Set
Block Count Command, it says"If illegal block count is set, out of range
error will be indicated during read/write operation (For example, data
transfer is stopped at user area boundary)." In another word, you could
expect a out of range error in the response for the following CMD18/25
if your argument of CMD23 + the argument of CMD18/25 exceed the max
number of blocks. So we have check the R1 of CMD18/25 for sure that
won't that happen.

And even if you ignore that out of range bit from CMD18/25, but you
could still get a -ETIMEDOUT or any error number from the host drivers
due to missing data response(for write)/data(for read), as the cards
will stop the data transfer by itself.

So that implies we only need to care about open-ending mode.

> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index f1bbfd389367ff..e83d8291ad4f99 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1362,6 +1362,14 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
>   	}
>   }
>   
> +#define STOP_ERRORS							\
> +	 (R1_ADDRESS_ERROR |	/* Misaligned address */		\
> +	 R1_BLOCK_LEN_ERROR |	/* Transferred block length incorrect */\
> +	 R1_WP_VIOLATION |	/* Tried to write to protected block */	\
> +	 R1_CARD_ECC_FAILED |	/* Card ECC failed */			\
> +	 R1_CC_ERROR |		/* Card controller error */		\
> +	 R1_ERROR)		/* General/unknown error */
> +
>   #define CMD_ERRORS							\
>   	(R1_OUT_OF_RANGE |	/* Command argument out of range */	\
>   	 R1_ADDRESS_ERROR |	/* Misaligned address */		\
> @@ -1371,9 +1379,9 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
>   	 R1_CC_ERROR |		/* Card controller error */		\
>   	 R1_ERROR)		/* General/unknown error */
>   
> -static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
> +static bool mmc_blk_has_cmd_err(struct mmc_command *cmd, u32 err_mask)
>   {
> -	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
> +	if (!cmd->error && cmd->resp[0] & err_mask)
>   		cmd->error = -EIO;
>   
>   	return cmd->error;
> @@ -1400,7 +1408,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
>   	 * stop.error indicates a problem with the stop command.  Data
>   	 * may have been transferred, or may still be transferring.
>   	 */
> -	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
> +	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop, STOP_ERRORS) ||
>   	    brq->data.error) {
>   		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
>   		case ERR_RETRY:
> 
> Note that I decided to not couple STOP_ERRORS and CMD_ERRORS in case we
> need more adjustments in the future. And your comment needs to be added
> somewhere, too. It is just to show what I mean.
> 

I don't have a hard opinion here if you insist on that. :)
I could fold in the description from the spec see explain why
we don't need to check this for the CMD23 cases.

Does all the above sound goot to you?


> Do you think this could work, too? Or am I missing something?
> 
> Thanks and kind regards,
> 
>     Wolfram
> 

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Hi Shawn,

> So that implies we only need to care about open-ending mode.

I see. Thanks for the explanation.

> I could fold in the description from the spec see explain why
> we don't need to check this for the CMD23 cases.

That would be great.

> Does all the above sound goot to you?

Basically, yes. If we need to check for CMD23 then, I wonder if why we
really need to do (md->flags & MMC_BLK_CMD23) or if we can't simply
check the presence of brq->mrq.sbc? That could then lead to the
following refactorization which is a lot easier to read IMO (but only
compile tested, just to give you an idea what I had in mind):

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index f1bbfd389367ff..1cf905d0e88e77 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1371,12 +1371,17 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
 	 R1_CC_ERROR |		/* Card controller error */		\
 	 R1_ERROR)		/* General/unknown error */
 
-static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
+/* Map R1 errors to error codes */
+static void mmc_blk_eval_resp_error(struct mmc_blk_request *brq)
 {
-	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
-		cmd->error = -EIO;
+	u32 val;
 
-	return cmd->error;
+	/* If there is no error yet, check R1 response */
+	if (!brq->stop.error) {
+		val = brq->stop.resp[0] & CMD_ERRORS;
+		if (val && !(val & R1_OUT_OF_RANGE && brq->mrq.sbc))
+			brq->stop.error = -EIO;
+	}
 }
 
 static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
@@ -1400,8 +1405,10 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 	 * stop.error indicates a problem with the stop command.  Data
 	 * may have been transferred, or may still be transferring.
 	 */
-	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
-	    brq->data.error) {
+
+	mmc_blk_eval_resp_error(brq);
+
+	if (brq->sbc.error || brq->cmd.error || brq->stop.error || brq->data.error) {
 		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
 		case ERR_RETRY:
 			return MMC_BLK_RETRY;

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mmc: block: prevent propagating R1_OUT_OF_RANGE for open-ending mode

[Shawn Lin]

Hi Wolfram,

On 2017/8/15 17:16, Wolfram Sang wrote:
> Hi Shawn,
> 
>> So that implies we only need to care about open-ending mode.
> 
> I see. Thanks for the explanation.
> 
>> I could fold in the description from the spec see explain why
>> we don't need to check this for the CMD23 cases.
> 
> That would be great.
> 
>> Does all the above sound goot to you?
> 
> Basically, yes. If we need to check for CMD23 then, I wonder if why we
> really need to do (md->flags & MMC_BLK_CMD23) or if we can't simply
> check the presence of brq->mrq.sbc? That could then lead to the
> following refactorization which is a lot easier to read IMO (but only
> compile tested, just to give you an idea what I had in mind):
> 

Ok, I get your point and it looks good to me as I also plan to introduce
ACMD23 for SD card and your suggestion also inspire me there to simplify
the patch.

Great, I will respin v3 then. :)

Thanks for sharing your thought!

> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index f1bbfd389367ff..1cf905d0e88e77 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1371,12 +1371,17 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
>   	 R1_CC_ERROR |		/* Card controller error */		\
>   	 R1_ERROR)		/* General/unknown error */
>   
> -static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
> +/* Map R1 errors to error codes */
> +static void mmc_blk_eval_resp_error(struct mmc_blk_request *brq)
>   {
> -	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
> -		cmd->error = -EIO;
> +	u32 val;
>   
> -	return cmd->error;
> +	/* If there is no error yet, check R1 response */
> +	if (!brq->stop.error) {
> +		val = brq->stop.resp[0] & CMD_ERRORS;
> +		if (val && !(val & R1_OUT_OF_RANGE && brq->mrq.sbc))
> +			brq->stop.error = -EIO;
> +	}
>   }
>   
>   static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
> @@ -1400,8 +1405,10 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
>   	 * stop.error indicates a problem with the stop command.  Data
>   	 * may have been transferred, or may still be transferring.
>   	 */
> -	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
> -	    brq->data.error) {
> +
> +	mmc_blk_eval_resp_error(brq);
> +
> +	if (brq->sbc.error || brq->cmd.error || brq->stop.error || brq->data.error) {
>   		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
>   		case ERR_RETRY:
>   			return MMC_BLK_RETRY;
> 
> Regards,
> 
>     Wolfram
>