| Message ID | 20170814175831.bgwr6hwc4tvbkmoc@ninjato (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: