| Message ID | CWXP265MB26807AC3C130772D789D0AABC41B9@CWXP265MB2680.GBRP265.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: block: Differentiate busy and non-TRAN state | expand |
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
>> 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
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; + } +} + +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; +} + +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 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(-)