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

[Wolfram Sang <wsa@the-dreams.de>]

[-- 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 --]