| Message ID | CWXP265MB268049D9AB181062DA7F6DDBC4009@CWXP265MB2680.GBRP265.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: block: Differentiate busy and non-TRAN state | expand |
> + /* > + * Cards will never return to TRAN after completing > + * identification commands or MMC_SEND_STATUS if they are not selected. > + */ > + return !(cmd->opcode == MMC_GO_IDLE_STATE > + || cmd->opcode == MMC_SEND_OP_COND > + || cmd->opcode == MMC_ALL_SEND_CID > + || cmd->opcode == MMC_SET_RELATIVE_ADDR > + || cmd->opcode == MMC_SET_DSR > + || cmd->opcode == MMC_SLEEP_AWAKE > + || cmd->opcode == MMC_SELECT_CARD > + || cmd->opcode == MMC_SEND_CSD > + || cmd->opcode == MMC_SEND_CID > + || cmd->opcode == MMC_SEND_STATUS > + || cmd->opcode == MMC_GO_INACTIVE_STATE > + || cmd->opcode == MMC_APP_CMD); This is not the normal kernel style, which puts operators at the end of the line. And while a little more verbose I think a switch statement would be a lot more readable here: switch (cmd->opcode) { case MMC_GO_IDLE_STATE: case MMC_SEND_OP_COND: case MMC_ALL_SEND_CID: case MMC_SET_RELATIVE_ADDR: case MMC_SET_DSR: case MMC_SLEEP_AWAKE: case MMC_SELECT_CARD: case MMC_SEND_CSD: case MMC_SEND_CID: case MMC_SEND_STATUS: case MMC_GO_INACTIVE_STATE: case MMC_APP_CMD: return false; default: return true; }
On Tue, 6 Jul 2021 at 10:20, Christian Löhle <CLoehle@hyperstone.com> wrote: > > Prevent race condition with ioctl commands > > Wait for both, a card no longer signalling busy > and it being returned back to TRAN state. > A card not signaling busy does not mean it is > ready to accept new regular commands. > Instead for a command to be done it should not only > no longer signal busy but also return back to TRAN state, > at least for commands that eventually transition back > to TRAN. Otherwise the next ioctl command may be rejected > as the card is still in PROG state after the previous command. > > Signed-off-by: Christian Loehle <cloehle@hyperstone.com> > --- > drivers/mmc/core/block.c | 84 +++++++++++++++++++++++++++++++++++----- > include/linux/mmc/mmc.h | 9 +++-- > 2 files changed, 80 insertions(+), 13 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 88f4c215caa6..dda10ccee37f 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -411,7 +411,32 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, > return 0; > } > > -static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > +static int is_return_to_tran_cmd(struct mmc_command *cmd) > +{ > + /* > + * Cards will never return to TRAN after completing > + * identification commands or MMC_SEND_STATUS if they are not selected. > + */ > + switch (cmd->opcode) { > + case MMC_GO_IDLE_STATE: > + case MMC_SEND_OP_COND: > + case MMC_ALL_SEND_CID: > + case MMC_SET_RELATIVE_ADDR: > + case MMC_SET_DSR: > + case MMC_SLEEP_AWAKE: > + case MMC_SELECT_CARD: > + case MMC_SEND_CSD: > + case MMC_SEND_CID: > + case MMC_SEND_STATUS: > + case MMC_GO_INACTIVE_STATE: > + case MMC_APP_CMD: > + return false; > + default: > + return true; > + } > +} What exactly are you trying to do with the user space program through the mmc ioctl with all these commands? The mmc ioctl interface is not designed to be used like that. In principle, it looks like we should support a complete re-initialization of the card. I am sorry, but no thanks! This doesn't work, but more importantly, this should be managed solely by the kernel, in my opinion. > + > +static int card_poll_until_tran(struct mmc_card *card, unsigned int timeout_ms, > u32 *resp_errs) > { > unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); > @@ -433,8 +458,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > *resp_errs |= status; > > /* > - * Timeout if the device never becomes ready for data and never > - * leaves the program state. > + * Timeout if the device never returns to TRAN state. > */ > if (done) { > dev_err(mmc_dev(card->host), > @@ -442,6 +466,41 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > __func__, status); > return -ETIMEDOUT; > } > + } while (R1_CURRENT_STATE(status) != R1_STATE_TRAN); > + > + return err; > +} > + > +static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > + u32 *resp_errs) > +{ > + unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); > + int err = 0; > + u32 status; > + > + do { > + bool done = time_after(jiffies, timeout); > + > + err = __mmc_send_status(card, &status, 5); > + if (err) { > + dev_err(mmc_dev(card->host), > + "error %d requesting status\n", err); > + return err; > + } > + > + /* Accumulate any response error bits seen */ > + if (resp_errs) > + *resp_errs |= status; > + > + /* > + * Timeout if the device never becomes ready for data. > + */ > + if (done) { > + dev_err(mmc_dev(card->host), > + "Card remained busy! %s status: %#x\n", > + __func__, status); > + return -ETIMEDOUT; > + } > } while (!mmc_ready_for_data(status)); > > return err; > @@ -596,12 +655,19 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) { > /* > - * Ensure RPMB/R1B command has completed by polling CMD13 > - * "Send Status". > + * Ensure card is no longer signalling busy by polling CMD13. > */ > err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL); > } > > + if (is_return_to_tran_cmd(&cmd)) { > + /* > + * Ensure card has returned back to TRAN state (e.g. from PROG) > + * and is ready to accept a new command. > + */ > + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, NULL); > + } > + > return err; > } > > @@ -1630,7 +1696,7 @@ static int mmc_blk_fix_state(struct mmc_card *card, struct request *req) > > mmc_blk_send_stop(card, timeout); > > - err = card_busy_detect(card, timeout, NULL); > + err = card_poll_until_tran(card, timeout, NULL); > > mmc_retune_release(card->host); > > @@ -1662,7 +1728,7 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) > goto error_exit; > > if (!mmc_host_is_spi(host) && > - !mmc_ready_for_data(status)) { > + !mmc_tran_and_ready_for_data(status)) { > err = mmc_blk_fix_state(card, req); > if (err) > goto error_exit; > @@ -1784,7 +1850,7 @@ static void mmc_blk_mq_rw_recovery(struct mmc_queue *mq, struct request *req) > > /* Try to get back to "tran" state */ > if (!mmc_host_is_spi(mq->card->host) && > - (err || !mmc_ready_for_data(status))) > + (err || !mmc_tran_and_ready_for_data(status))) > err = mmc_blk_fix_state(mq->card, req); > > /* > @@ -1854,7 +1920,7 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req) > if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ) > return 0; > > - err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, &status); > + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, &status); > > /* > * Do not assume data transferred correctly if there are any error bits > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h > index d9a65c6a8816..9ae27504cbc9 100644 > --- a/include/linux/mmc/mmc.h > +++ b/include/linux/mmc/mmc.h > @@ -163,10 +163,11 @@ static inline bool mmc_op_multi(u32 opcode) > > static inline bool mmc_ready_for_data(u32 status) > { > - /* > - * Some cards mishandle the status bits, so make sure to check both the > - * busy indication and the card state. > - */ > + return status & R1_READY_FOR_DATA; mmc_ready_for_data() is also being called from mmc_busy_cb(). The check for R1_STATE_TRAN is needed there. > +} > + > +static inline bool mmc_tran_and_ready_for_data(u32 status) > +{ > return status & R1_READY_FOR_DATA && > R1_CURRENT_STATE(status) == R1_STATE_TRAN; > } > -- Kind regards Uffe
Hey Uffe, >> +static int is_return_to_tran_cmd(struct mmc_command *cmd) >> +{ >> + /* >> + * Cards will never return to TRAN after completing >> + * identification commands or MMC_SEND_STATUS if they are not selected. >> + */ >> + switch (cmd->opcode) { >> + case MMC_GO_IDLE_STATE: >> + case MMC_SEND_OP_COND: >> + case MMC_ALL_SEND_CID: >> + case MMC_SET_RELATIVE_ADDR: >> + case MMC_SET_DSR: >> + case MMC_SLEEP_AWAKE: >> + case MMC_SELECT_CARD: >> + case MMC_SEND_CSD: >> + case MMC_SEND_CID: >> + case MMC_SEND_STATUS: >> + case MMC_GO_INACTIVE_STATE: >> + case MMC_APP_CMD: >> + return false; >> + default: >> + return true; >> + } >> +} >> >What exactly are you trying to do with the user space program through >the mmc ioctl with all these commands? The mmc ioctl interface is not >designed to be used like that. > >In principle, it looks like we should support a complete >re-initialization of the card. I am sorry, but no thanks! This doesn't >work, but more importantly, this should be managed solely by the >kernel, in my opinion. Doing initialization itself through ioctl is silly, I agree, and does not work on other ends. This patch is not about that. it just explicitly disables any CMD13 polling for TRAN for some of those commands. the behavior for such commands thus is the same as without the patch. The reason for this patch is to not run into the race condition that a following (ioctl) command will be rejected because the card is in e.g. PROG state of a previous ioctl command. As stated earlier, I encountered this a lot when doing a unlock force erase -> lock/set, in both scenarios, issued as two single ioctl commands and bundled together. But this race condition exists on any (non-R1b/ RPBM) currently. As there is no CMD13 polling happening after the response (or whenever the driver marks the request as done), the card's status is therefore generally unknown. So in short I don;t want to do anything too crazy from userspace, but the alternative now is to do like 100ms sleeps in the hope that the card is actually finished with the issued command (not just the host driver so to say). Kind Regards, Christian Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz Managing Directors: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
On Tue, 6 Jul 2021 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote: > > Hey Uffe, > > >> +static int is_return_to_tran_cmd(struct mmc_command *cmd) > >> +{ > >> + /* > >> + * Cards will never return to TRAN after completing > >> + * identification commands or MMC_SEND_STATUS if they are not selected. > >> + */ > >> + switch (cmd->opcode) { > >> + case MMC_GO_IDLE_STATE: > >> + case MMC_SEND_OP_COND: > >> + case MMC_ALL_SEND_CID: > >> + case MMC_SET_RELATIVE_ADDR: > >> + case MMC_SET_DSR: > >> + case MMC_SLEEP_AWAKE: > >> + case MMC_SELECT_CARD: > >> + case MMC_SEND_CSD: > >> + case MMC_SEND_CID: > >> + case MMC_SEND_STATUS: > >> + case MMC_GO_INACTIVE_STATE: > >> + case MMC_APP_CMD: > >> + return false; > >> + default: > >> + return true; > >> + } > >> +} > >> > >What exactly are you trying to do with the user space program through > >the mmc ioctl with all these commands? The mmc ioctl interface is not > >designed to be used like that. > > > >In principle, it looks like we should support a complete > >re-initialization of the card. I am sorry, but no thanks! This doesn't > >work, but more importantly, this should be managed solely by the > >kernel, in my opinion. > > Doing initialization itself through ioctl is silly, I agree, and does > not work on other ends. This patch is not about that. it just explicitly disables > any CMD13 polling for TRAN for some of those commands. the behavior > for such commands thus is the same as without the patch. You are right. But, what I think is bothering me with the approach, is that it looks like we are starting to add special treatment of a whole bunch of different commands. > The reason for this patch is to not run into the race condition that a > following (ioctl) command will be rejected because the card is in e.g. PROG state > of a previous ioctl command. As stated earlier, I encountered this a lot when > doing a unlock force erase -> lock/set, in both scenarios, issued as two single > ioctl commands and bundled together. I understand. I would rather see a patch that adds support, explicitly for this case. > But this race condition exists on any (non-R1b/ RPBM) currently. As there is > no CMD13 polling happening after the response (or whenever the driver marks > the request as done), the card's status is therefore generally unknown. So the commands to unlock/lock, etc, don't have R1B, but can still cause the card to become busy after the response has been delivered, right? As I said, then please add this as an explicit check to do polling, then I would be happy. :-) > > So in short I don;t want to do anything too crazy from userspace, but the > alternative now is to do like 100ms sleeps in the hope that the card is > actually finished with the issued command (not just the host driver so to say). Yeah, that sounds suboptimal, we can do better than that. Kind regards Uffe
Hi, > >What exactly are you trying to do with the user space program through > >the mmc ioctl with all these commands? The mmc ioctl interface is not > >designed to be used like that. > > > >In principle, it looks like we should support a complete > >re-initialization of the card. I am sorry, but no thanks! This doesn't > >work, but more importantly, this should be managed solely by the > >kernel, in my opinion. > > Doing initialization itself through ioctl is silly, I agree, and does > not work on other ends. This patch is not about that. it just explicitly disables > any CMD13 polling for TRAN for some of those commands. the behavior > for such commands thus is the same as without the patch. > The reason for this patch is to not run into the race condition that a > following (ioctl) command will be rejected because the card is in e.g. PROG > state > of a previous ioctl command. As stated earlier, I encountered this a lot when > doing a unlock force erase -> lock/set, in both scenarios, issued as two single > ioctl commands and bundled together. Are you using mmc-utils? Can you share exactly the sequence of commands you are sending? > But this race condition exists on any (non-R1b/ RPBM) currently. As there is > no CMD13 polling happening after the response (or whenever the driver > marks > the request as done), the card's status is therefore generally unknown. Again, can you share the sequence of the commands you are using? Thanks, Avri > > So in short I don;t want to do anything too crazy from userspace, but the > alternative now is to do like 100ms sleeps in the hope that the card is > actually finished with the issued command (not just the host driver so to > say). > > Kind Regards, > Christian > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz > Managing Directors: Dr. Jan Peter Berns. > Commercial register of local courts: Freiburg HRB381782
Hey Avri, >Are you using mmc-utils? No, Im accessing the ioctl interface with my own application. >Can you share exactly the sequence of commands you are sending? The one I initially encountered was, as stated earlier, a Unlock-Force Erase into a new Lock with set password. Basically any R1 (no b) command that transitions to PROG, so behaves like a write command, could trigger this. But obviously Unlock force erase is the best example, as a full erase will take quite some time and many (all?) cards will not accept new commands (i.e. stay in PROG) until the erase has actually completed. The current code will not check anything for CMD42 after the response. I have not hit the race condition with anything but CMD42. So to be verbose: CMD16 - CMD42 Set PW - (CMD16)* - CMD42 Unlock Force Erase - (CMD42 Set PW)+ * May be omitted if you craft the CMD42 carefully (i.e. equal data size) + is pretty much irrelevant, can be replaced with anything that is illegal in PROG. >Again, can you share the sequence of the commands you are using? > >Thanks, >Avri Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz Managing Directors: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
> Hey Avri, > > >Are you using mmc-utils? > No, Im accessing the ioctl interface with my own application. > > >Can you share exactly the sequence of commands you are sending? > > The one I initially encountered was, as stated earlier, a Unlock-Force Erase > into a new Lock with set password. Basically any R1 (no b) command that > transitions to PROG, so behaves like a write command, could trigger this. > But obviously Unlock force erase is the best example, as a full erase will > take quite some time and many (all?) cards will not accept new commands > (i.e. stay in PROG) until the erase has actually completed. The current > code will not check anything for CMD42 after the response. > I have not hit the race condition with anything but CMD42. > > So to be verbose: > CMD16 - CMD42 Set PW - (CMD16)* - CMD42 Unlock Force Erase - (CMD42 > Set PW)+ > * May be omitted if you craft the CMD42 carefully (i.e. equal data size) > + is pretty much irrelevant, can be replaced with anything that is illegal in > PROG. Oh, OK. Interesting. This functionality is missing in mmc-utils. While at it, I encourage you to consider adding it. Thanks, Avri > > >Again, can you share the sequence of the commands you are using? > > > >Thanks, > >Avri > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz > Managing Directors: Dr. Jan Peter Berns. > Commercial register of local courts: Freiburg HRB381782
On Wed, 7 Jul 2021 at 10:27, Christian Löhle <CLoehle@hyperstone.com> wrote: > > Prevent race condition with ioctl commands > > To fully prevent a race condition where the next > issued command will be rejected as the card is no > longer signalling busy but not yet back in TRAN state. > The card may be in PROG state without signalling busy, > for some of the commands that are only R1, but also > for R1b commands, the card will signal non-busy as soon > as receive buffers are free again, but the card has > not finished handling the command and may therefore be > in PROG. Can you please point me to the corresponding information in the spec that states that the above behavior is correct? In principle what you are saying is that busy signalling on DAT0 is *entirely* broken, at least for some cards and some commands. > Since the next command is not known at the time of > completion we must assume that it may be one that can > only be accepted in TRAN state. > Therefore we only consider a PROG command completed > when we have polled for TRAN. Right. See more comments about this further below. > > Signed-off-by: Christian Loehle <cloehle@hyperstone.com> > --- > drivers/mmc/core/block.c | 86 ++++++++++++++++++++++++++++++++++---- > drivers/mmc/core/mmc_ops.c | 2 +- > include/linux/mmc/mmc.h | 10 +++-- > include/linux/mmc/sd.h | 3 ++ > 4 files changed, 87 insertions(+), 14 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 88f4c215caa6..cb78690647bf 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -411,7 +411,34 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, > return 0; > } > > -static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > +static int is_prog_cmd(struct mmc_command *cmd) > +{ > + /* > + * Cards will move to programming state (PROG) after these commands. > + * So we must not consider the command as completed until the card > + * has actually returned back to TRAN state. > + */ > + switch (cmd->opcode) { > + case MMC_STOP_TRANSMISSION: This has an R1B response, hence we already do the proper polling that is needed. In other words, we don't need to explicitly check for this command here, as we are already checking the response type (R1B) in __mmc_blk_ioctl_cmd(). > + case MMC_WRITE_DAT_UNTIL_STOP: What's this used for? It's obsolete, at least in the eMMC spec. Please drop it. > + case MMC_WRITE_BLOCK: > + case MMC_WRITE_MULTIPLE_BLOCK: These are already supported via the generic block interface, please drop the checks. > + case MMC_PROGRAM_CID: > + case MMC_PROGRAM_CSD: Let's discuss these, since they have R1 responses. Although, according to the eMMC spec, the card moves to rcv state, not the prg state as you refer to in the commit message. Normally, we don't need to poll for busy/tran completion of these commands. Have you observed through proper tests that this is actually needed? > + case MMC_SET_WRITE_PROT: > + case MMC_CLR_WRITE_PROT: > + case MMC_ERASE: The three above have R1B, please drop them from here as they are already supported correctly. > + case MMC_LOCK_UNLOCK: Again, this has an R1 response and the card moves to rcv state. Normally we shouldn't need to poll, but I have to admit that the eMMC spec isn't really clear on what will happen when using the "forced erase" argument. The spec mentions a 3 minute timeout.... > + case MMC_SET_TIME: /* Also covers SD_WRITE_EXTR_SINGLE */ > + case MMC_GEN_CMD: > + case SD_WRITE_EXTR_MULTI: Are these actually being used? If not, please drop them from being supported. I don't want to encourage crazy operations being issued from userspace. > + return true; > + default: > + return false; > + } > +} Overall, it looks like we need to add a check for MMC_LOCK_UNLOCK to poll for busy, but that's it, I think. > + > +static int card_poll_until_tran(struct mmc_card *card, unsigned int timeout_ms, > u32 *resp_errs) > { > unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); > @@ -433,8 +460,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > *resp_errs |= status; > > /* > - * Timeout if the device never becomes ready for data and never > - * leaves the program state. > + * Timeout if the device never returns to TRAN state. > */ > if (done) { > dev_err(mmc_dev(card->host), > @@ -442,6 +468,41 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > __func__, status); > return -ETIMEDOUT; > } > + } while (R1_CURRENT_STATE(status) != R1_STATE_TRAN); > + > + return err; > +} > + > +static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > + u32 *resp_errs) > +{ > + unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); > + int err = 0; > + u32 status; > + > + do { > + bool done = time_after(jiffies, timeout); > + > + err = __mmc_send_status(card, &status, 5); > + if (err) { > + dev_err(mmc_dev(card->host), > + "error %d requesting status\n", err); > + return err; > + } > + > + /* Accumulate any response error bits seen */ > + if (resp_errs) > + *resp_errs |= status; > + > + /* > + * Timeout if the device never becomes ready for data. > + */ > + if (done) { > + dev_err(mmc_dev(card->host), > + "Card remained busy! %s status: %#x\n", > + __func__, status); > + return -ETIMEDOUT; > + } > } while (!mmc_ready_for_data(status)); I don't quite understand what we accomplish with polling for TRAN state in one case and in the other case, both TRAN and READY_FOR_DATA. Why can't we always poll for TRAN and READY_FOR_DATA? It should work for all cases, no? > > return err; > @@ -596,12 +657,19 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) { > /* > - * Ensure RPMB/R1B command has completed by polling CMD13 > - * "Send Status". > + * Ensure card is no longer signalling busy by polling CMD13. > */ > err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL); > } > > + if (is_prog_cmd(&cmd)) { > + /* > + * Ensure card has returned back to TRAN state > + * and is ready to accept a new command. > + */ > + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, NULL); > + } > + > return err; > } > > @@ -1630,7 +1698,7 @@ static int mmc_blk_fix_state(struct mmc_card *card, struct request *req) > > mmc_blk_send_stop(card, timeout); > > - err = card_busy_detect(card, timeout, NULL); > + err = card_poll_until_tran(card, timeout, NULL); > > mmc_retune_release(card->host); > > @@ -1662,7 +1730,7 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) > goto error_exit; > > if (!mmc_host_is_spi(host) && > - !mmc_ready_for_data(status)) { > + !mmc_tran_and_ready_for_data(status)) { > err = mmc_blk_fix_state(card, req); > if (err) > goto error_exit; > @@ -1784,7 +1852,7 @@ static void mmc_blk_mq_rw_recovery(struct mmc_queue *mq, struct request *req) > > /* Try to get back to "tran" state */ > if (!mmc_host_is_spi(mq->card->host) && > - (err || !mmc_ready_for_data(status))) > + (err || !mmc_tran_and_ready_for_data(status))) > err = mmc_blk_fix_state(mq->card, req); > > /* > @@ -1854,7 +1922,7 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req) > if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ) > return 0; > > - err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, &status); > + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, &status); > > /* > * Do not assume data transferred correctly if there are any error bits > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index 973756ed4016..a0be45118a93 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -465,7 +465,7 @@ static int mmc_busy_cb(void *cb_data, bool *busy) > if (err) > return err; > > - *busy = !mmc_ready_for_data(status); > + *busy = !mmc_tran_and_ready_for_data(status); > return 0; > } Kind regards Uffe
>> >> Prevent race condition with ioctl commands >> >> To fully prevent a race condition where the next >> issued command will be rejected as the card is no >> longer signalling busy but not yet back in TRAN state. >> The card may be in PROG state without signalling busy, >> for some of the commands that are only R1, but also >> for R1b commands, the card will signal non-busy as soon >> as receive buffers are free again, but the card has >> not finished handling the command and may therefore be >> in PROG. > >Can you please point me to the corresponding information in the spec >that states that the above behavior is correct? Sure, unfortunately it is blanked in the simplified spec so I (think I) cannot simply cite it now. If access to the full spec is a problem for many contributors of the list, I would reconsider the legal aspects. The part I'm referring to is the last (two) sentence(s) of the section "Single Block Write" in 4.12.3 Data Write. (Single Block Write is the correct section as all R1 (no b) commands with data behave like Single Block Write. This info itself is scattered throughout the (simplified) spec, but for CMD42 it is: "The card lock/unlock command has the structure and bus transaction type of a regular single block write command." For some other commands it is in the command description column of the 4.7.4) > >In principle what you are saying is that busy signalling on DAT0 is >*entirely* broken, at least for some cards and some commands. I wouldn't say broken, it seems to do what is described. But right now busy polling is essentially CMD13 polling, right? at least for card_busy_detect. So it doesn't hurt polling for TRAN. But not for all commands busy signalling <=> PROG state is true: "The card may provide buffering for block write. This means that the next block can be sent to the card while the previous is being programmed." (4.3) "There is no buffering option for write CSD, write protection and erase. This means that while the card is busy servicing any one of these commands, no other data transfer commands will be accepted. DAT0 line will be kept low as long as the card is busy and in the Programming State." (4.3) Definitely leaves us with MMC_PROGRAM_CID, CMD20, CMD42, MMC_SET_TIME, MMC_GEN_CMD and both SD_WRITE_EXTR. Furthermore I think the cleaner solution is to poll for TRAN anyway. Just in theory there does not seem to be a timing constraint for when the card starts signalling busy for R1 (non-b) commands. Just in theory the ioctl handling could be done BEFORE the card ever moves to PROG and starts signalling busy. (Im not sure about this though) >> >> -static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, >> +static int is_prog_cmd(struct mmc_command *cmd) >> +{ >> + /* >> + * Cards will move to programming state (PROG) after these commands. >> + * So we must not consider the command as completed until the card >> + * has actually returned back to TRAN state. >> + */ >> + switch (cmd->opcode) { >> + case MMC_STOP_TRANSMISSION: > >This has an R1B response, hence we already do the proper polling that is needed. > >In other words, we don't need to explicitly check for this command >here, as we are already checking the response type (R1B) in >__mmc_blk_ioctl_cmd(). Fair. I will happily remove any multi block write commands. Just seemed like the safer way to do so, but I have not specifically checked if any cards violate this here. > >> + case MMC_WRITE_DAT_UNTIL_STOP: > >What's this used for? It's obsolete, at least in the eMMC spec. Please drop it. Okay. >> + case MMC_WRITE_BLOCK: >> + case MMC_WRITE_MULTIPLE_BLOCK: > >These are already supported via the generic block interface, please >drop the checks. Might make sense to let userspace issue read/write, too. But I understand if this is not desired. >> + case MMC_PROGRAM_CID: >> + case MMC_PROGRAM_CSD: > >Let's discuss these, since they have R1 responses. > >Although, according to the eMMC spec, the card moves to rcv state, not >the prg state as you refer to in the commit message. Normally, we >don't need to poll for busy/tran completion of these commands. Why not? Sure they move to rcv first, but if data stops they move to PROG. > >Have you observed through proper tests that this is actually needed? No, seems unlikely to hit this, as PROG will likely be shorter than getting a second command through. > >> + case MMC_SET_WRITE_PROT: >> + case MMC_CLR_WRITE_PROT: >> + case MMC_ERASE: > >The three above have R1B, please drop them from here as they are >already supported correctly. > >> + case MMC_LOCK_UNLOCK: > >Again, this has an R1 response and the card moves to rcv state. >Normally we shouldn't need to poll, but I have to admit that the eMMC >spec isn't really clear on what will happen when using the "forced >erase" argument. The spec mentions a 3 minute timeout.... Again I don't know why you would not need to poll. The force erase has a good reason to remain in PROG for long, but whatever, a card may decide to just take 5 seconds in unlock PROG. (to prevent bruteforcing passwords lets say)(Not anything I have seen or expect to see) > >> + case MMC_SET_TIME: /* Also covers SD_WRITE_EXTR_SINGLE */ >> + case MMC_GEN_CMD: >> + case SD_WRITE_EXTR_MULTI: > >Are these actually being used? If not, please drop them from being >supported. I don't want to encourage crazy operations being issued >from userspace. GEN_CMD is extremly interesting for issuing vendor commands from user-space. Not sure if anyone uses it (yet), but if so it's unlikely to be seen in the wild. SD_WRITE_EXTR_MULTI is simply too new to really say. MMC_SET_TIME probably not used. > >Overall, it looks like we need to add a check for MMC_LOCK_UNLOCK to >poll for busy, but that's it, I think. See above. >> } while (!mmc_ready_for_data(status)); > >I don't quite understand what we accomplish with polling for TRAN >state in one case and in the other case, both TRAN and READY_FOR_DATA. >Why can't we always poll for TRAN and READY_FOR_DATA? It should work >for all cases, no? > Well in theory you're then dropping the buffered writing feature of the SD spec if waiting for TRAN, too. I'm fine with that, especially since it is not desired to be used through ioctl anyway? Kind Regards, Christian Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz Managing Directors: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
>> CMD16 - CMD42 Set PW - (CMD16)* - CMD42 Unlock Force Erase - (CMD42 >> Set PW)+ >> * May be omitted if you craft the CMD42 carefully (i.e. equal data size) >> + is pretty much irrelevant, can be replaced with anything that is illegal in >> PROG. >Oh, OK. Interesting. >This functionality is missing in mmc-utils. >While at it, I encourage you to consider adding it. > >Thanks, >Avri That is true and I have thought about it. Unfortunately a locked card currently will not initalize, so not be open to the ioctl interface. So in fear of doing more harm than good I would first change that. (One thing is reading of SCR is required by linux-mmc but locked cards must not respond to it). Something I'm working on on the side but don't expect anything very soon. Kind Regards, Christian > > >Again, can you share the sequence of the commands you are using? > > > >Thanks, > >Avri > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz > Managing Directors: Dr. Jan Peter Berns. > Commercial register of local courts: Freiburg HRB381782 Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz Managing Directors: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
[...] > >> + case MMC_PROGRAM_CID: > >> + case MMC_PROGRAM_CSD: > > > >Let's discuss these, since they have R1 responses. > > > >Although, according to the eMMC spec, the card moves to rcv state, not > >the prg state as you refer to in the commit message. Normally, we > >don't need to poll for busy/tran completion of these commands. > > Why not? Sure they move to rcv first, but if data stops they move to PROG. Yes, I guess they could. > > > >Have you observed through proper tests that this is actually needed? > > No, seems unlikely to hit this, as PROG will likely be shorter than getting a second command through. Right. In any case, I wouldn't mind if we start to poll for these, it sure sounds a bit more robust. > > > >> + case MMC_SET_WRITE_PROT: > >> + case MMC_CLR_WRITE_PROT: > >> + case MMC_ERASE: > > > >The three above have R1B, please drop them from here as they are > >already supported correctly. > > > >> + case MMC_LOCK_UNLOCK: > > > >Again, this has an R1 response and the card moves to rcv state. > >Normally we shouldn't need to poll, but I have to admit that the eMMC > >spec isn't really clear on what will happen when using the "forced > >erase" argument. The spec mentions a 3 minute timeout.... > > Again I don't know why you would not need to poll. > The force erase has a good reason to remain in PROG for long, but whatever, a card may decide to just take 5 seconds in unlock PROG. (to prevent bruteforcing passwords lets say)(Not anything I have seen or expect to see) As I said, the spec is not really clear here. So yes, polling sounds more safe/robust. > > > > >> + case MMC_SET_TIME: /* Also covers SD_WRITE_EXTR_SINGLE */ > >> + case MMC_GEN_CMD: > >> + case SD_WRITE_EXTR_MULTI: > > > >Are these actually being used? If not, please drop them from being > >supported. I don't want to encourage crazy operations being issued > >from userspace. > > GEN_CMD is extremly interesting for issuing vendor commands from user-space. > Not sure if anyone uses it (yet), but if so it's unlikely to be seen in the wild. > SD_WRITE_EXTR_MULTI is simply too new to really say. > MMC_SET_TIME probably not used. > I am worried that it looks like we encourage doing "crazy" things from userspace, when we add explicit checks for these commands. In any case, if you strongly think we need these, please add polling for them (or a subset of them). > > > > >Overall, it looks like we need to add a check for MMC_LOCK_UNLOCK to > >poll for busy, but that's it, I think. > > See above. > > > >> } while (!mmc_ready_for_data(status)); > > > >I don't quite understand what we accomplish with polling for TRAN > >state in one case and in the other case, both TRAN and READY_FOR_DATA. > >Why can't we always poll for TRAN and READY_FOR_DATA? It should work > >for all cases, no? > > > > Well in theory you're then dropping the buffered writing feature of the SD spec if waiting for TRAN, too. > I'm fine with that, especially since it is not desired to be used through ioctl anyway? Well, I think you are misinterpreting how the buffered writing feature is working in practice. The mmc core doesn't write one block at the time and polls for READY_FOR_DATA/TRAN in between each written block. Instead, it's the responsibility of the mmc controller HW, to monitor DAT0 during a data transmission, to understand whether it can continue to fill and write more buffers. Kind regards Uffe
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 88f4c215caa6..ddb395afe6e0 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -411,7 +411,27 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, return 0; } -static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, +static int is_return_to_tran_cmd(struct mmc_command *cmd) +{ + /* + * Cards will never return to TRAN after completing + * identification commands or MMC_SEND_STATUS if they are not selected. + */ + return !(cmd->opcode == MMC_GO_IDLE_STATE + || cmd->opcode == MMC_SEND_OP_COND + || cmd->opcode == MMC_ALL_SEND_CID + || cmd->opcode == MMC_SET_RELATIVE_ADDR + || cmd->opcode == MMC_SET_DSR + || cmd->opcode == MMC_SLEEP_AWAKE + || cmd->opcode == MMC_SELECT_CARD + || cmd->opcode == MMC_SEND_CSD + || cmd->opcode == MMC_SEND_CID + || cmd->opcode == MMC_SEND_STATUS + || cmd->opcode == MMC_GO_INACTIVE_STATE + || cmd->opcode == MMC_APP_CMD); +} + +static int card_poll_until_tran(struct mmc_card *card, unsigned int timeout_ms, u32 *resp_errs) { unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); @@ -433,8 +453,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, *resp_errs |= status; /* - * Timeout if the device never becomes ready for data and never - * leaves the program state. + * Timeout if the device never returns to TRAN state. */ if (done) { dev_err(mmc_dev(card->host), @@ -442,6 +461,41 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, __func__, status); return -ETIMEDOUT; } + } while (R1_CURRENT_STATE(status) != R1_STATE_TRAN); + + return err; +} + +static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, + u32 *resp_errs) +{ + unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); + int err = 0; + u32 status; + + do { + bool done = time_after(jiffies, timeout); + + err = __mmc_send_status(card, &status, 5); + if (err) { + dev_err(mmc_dev(card->host), + "error %d requesting status\n", err); + return err; + } + + /* Accumulate any response error bits seen */ + if (resp_errs) + *resp_errs |= status; + + /* + * Timeout if the device never becomes ready for data. + */ + if (done) { + dev_err(mmc_dev(card->host), + "Card remained busy! %s status: %#x\n", + __func__, status); + return -ETIMEDOUT; + } } while (!mmc_ready_for_data(status)); return err; @@ -602,6 +656,10 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL); } + if (is_return_to_tran_cmd(&cmd)) + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, NULL); + return err; } @@ -1630,7 +1688,7 @@ static int mmc_blk_fix_state(struct mmc_card *card, struct request *req) mmc_blk_send_stop(card, timeout); - err = card_busy_detect(card, timeout, NULL); + err = card_poll_until_tran(card, timeout, NULL); mmc_retune_release(card->host); @@ -1662,7 +1720,7 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) goto error_exit; if (!mmc_host_is_spi(host) && - !mmc_ready_for_data(status)) { + !mmc_tran_and_ready_for_data(status)) { err = mmc_blk_fix_state(card, req); if (err) goto error_exit; @@ -1784,7 +1842,7 @@ static void mmc_blk_mq_rw_recovery(struct mmc_queue *mq, struct request *req) /* Try to get back to "tran" state */ if (!mmc_host_is_spi(mq->card->host) && - (err || !mmc_ready_for_data(status))) + (err || !mmc_tran_and_ready_for_data(status))) err = mmc_blk_fix_state(mq->card, req); /* @@ -1854,7 +1912,7 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req) if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ) return 0; - err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, &status); + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, &status); /* * Do not assume data transferred correctly if there are any error bits diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index d9a65c6a8816..cf1a4a330e02 100644 --- a/include/linux/mmc/mmc.h +++ b/include/linux/mmc/mmc.h @@ -163,10 +167,11 @@ static inline bool mmc_op_multi(u32 opcode) static inline bool mmc_ready_for_data(u32 status) { - /* - * Some cards mishandle the status bits, so make sure to check both the - * busy indication and the card state. - */ + return status & R1_READY_FOR_DATA; +} + +static inline bool mmc_tran_and_ready_for_data(u32 status) +{ return status & R1_READY_FOR_DATA && R1_CURRENT_STATE(status) == R1_STATE_TRAN; }
Prevent race condition with ioctl commands A card not signaling busy does not mean it is ready to accept new regular commands. Instead for a command to be done it should not only no longer busy but also return back to TRAN state, at least for commands that eventually transition back to TRAN. Otherwise the next ioctl command may be rejected as the card is still in PROG state after the previous command. Signed-off-by: Christian Loehle <cloehle@hyperstone.com> --- drivers/mmc/core/block.c | 72 ++++++++++++++++++++++++++++++++++++---- include/linux/mmc/mmc.h | 13 +++++--- 2 files changed, 74 insertions(+), 11 deletions(-)