Poll for TRAN state if the ioctl command will eventually return to TRAN
The ioctl submitted command should not be considered completed until
the card has returned back to TRAN state. Waiting just for the card
to no longer signal busy is not enough as they might remain in a
non-busy PROG state for a while after the command.
Further commands requiring TRAN will fail then.
It should not be the responsibility of the user to check if their command
has completed until sending the next via ioctl,
instead the check should be made here.
So now, in doubt, wait for TRAN except for the few commands that will
never return to TRAN state.
Signed-off-by: Christian Loehle <[email protected]>
---
drivers/mmc/core/block.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 689eb9afeeed..a02187e4fad7 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -410,7 +410,23 @@ 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_SEND_CID
+ || cmd->opcode == MMC_ALL_SEND_CID
+ || cmd->opcode == MMC_SEND_CSD
+ || cmd->opcode == MMC_SEND_STATUS
+ || cmd->opcode == MMC_SELECT_CARD
+ || cmd->opcode == MMC_APP_CMD
+ || cmd->opcode == MMC_GO_INACTIVE_STATE
+ || cmd->opcode == MMC_GO_IDLE_STATE);
+
+}
+
+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);
@@ -593,12 +609,13 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
- if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
+ if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B
+ || is_return_to_tran_cmd(&cmd)) {
/*
* Ensure RPMB/R1B command has completed by polling CMD13
* "Send Status".
*/
- err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL);
+ err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, NULL);
}
return err;
@@ -1621,7 +1638,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);
@@ -1845,7 +1862,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
--
2.31.1
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
Poll for TRAN state if the ioctl command will eventually return to TRAN
The ioctl submitted command should not be considered completed until
the card has returned back to TRAN state. Waiting just for the card
to no longer signal busy is not enough as they might remain in a
non-busy PROG state for a while after the command.
Further commands requiring TRAN will fail then.
It should not be the responsibility of the user to check if their command
has completed until sending the next via ioctl,
instead the check should be made here.
So now, in doubt, wait for TRAN except for the few commands that will
never return to TRAN state.
Signed-off-by: Christian Loehle <[email protected]>
---
drivers/mmc/core/block.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 689eb9afeeed..a02187e4fad7 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -410,7 +410,23 @@ 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_SEND_CID
+ || cmd->opcode == MMC_ALL_SEND_CID
+ || cmd->opcode == MMC_SEND_CSD
+ || cmd->opcode == MMC_SEND_STATUS
+ || cmd->opcode == MMC_SELECT_CARD
+ || cmd->opcode == MMC_APP_CMD
+ || cmd->opcode == MMC_GO_INACTIVE_STATE
+ || cmd->opcode == MMC_GO_IDLE_STATE);
+
+}
+
+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);
@@ -593,12 +609,13 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
- if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
+ if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B
+ || is_return_to_tran_cmd(&cmd)) {
/*
* Ensure RPMB/R1B command has completed by polling CMD13
* "Send Status".
*/
- err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL);
+ err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, NULL);
}
return err;
@@ -1621,7 +1638,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);
@@ -1845,7 +1862,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
--
2.31.1
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
> Poll for TRAN state if the ioctl command will eventually return to TRAN
>
> The ioctl submitted command should not be considered completed until
> the card has returned back to TRAN state. Waiting just for the card
> to no longer signal busy is not enough as they might remain in a
> non-busy PROG state for a while after the command.
> Further commands requiring TRAN will fail then.
> It should not be the responsibility of the user to check if their command
> has completed until sending the next via ioctl,
> instead the check should be made here.
> So now, in doubt, wait for TRAN except for the few commands that will
> never return to TRAN state.
Is this theoretical, or do you have an exact scenario in which the polling with cmd13 isn't enough?
Thanks,
Avri
> Poll for TRAN state if the ioctl command will eventually return to TRAN
>
> The ioctl submitted command should not be considered completed until
> the card has returned back to TRAN state. Waiting just for the card
> to no longer signal busy is not enough as they might remain in a
> non-busy PROG state for a while after the command.
> Further commands requiring TRAN will fail then.
> It should not be the responsibility of the user to check if their command
> has completed until sending the next via ioctl,
> instead the check should be made here.
> So now, in doubt, wait for TRAN except for the few commands that will
> never return to TRAN state.
>
> Signed-off-by: Christian Loehle <[email protected]>
> ---
> drivers/mmc/core/block.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 689eb9afeeed..a02187e4fad7 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -410,7 +410,23 @@ 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
Multi-line comment style
> + * identification commands or MMC_SEND_STATUS if they are not
> selected.
> + */
> + return !(cmd->opcode == MMC_SEND_CID
> + || cmd->opcode == MMC_ALL_SEND_CID
> + || cmd->opcode == MMC_SEND_CSD
> + || cmd->opcode == MMC_SEND_STATUS
> + || cmd->opcode == MMC_SELECT_CARD
> + || cmd->opcode == MMC_APP_CMD
> + || cmd->opcode == MMC_GO_INACTIVE_STATE
> + || cmd->opcode == MMC_GO_IDLE_STATE);
> +
Aren’t you only interested in cmds that move to tran state from other state?
According to the Device state transitions (table 61 in eMMC5.1) it only concern
cmd7 (stby->tran), cmd12 (data->tran), and cmd14 (btst->tran).
Thanks,
Avri
> +}
> +
> +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);
> @@ -593,12 +609,13 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> *card, struct mmc_blk_data *md,
>
> memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
>
> - if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
> + if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B
> + || is_return_to_tran_cmd(&cmd)) {
> /*
> * Ensure RPMB/R1B command has completed by polling CMD13
> * "Send Status".
> */
> - err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL);
> + err = card_poll_until_tran(card, MMC_BLK_TIMEOUT_MS, NULL);
> }
>
> return err;
> @@ -1621,7 +1638,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);
>
> @@ -1845,7 +1862,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
> --
> 2.31.1
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>> + */
>> + return !(cmd->opcode == MMC_SEND_CID
>> + || cmd->opcode == MMC_ALL_SEND_CID
>> + || cmd->opcode == MMC_SEND_CSD
>> + || cmd->opcode == MMC_SEND_STATUS
>> + || cmd->opcode == MMC_SELECT_CARD
>> + || cmd->opcode == MMC_APP_CMD
>> + || cmd->opcode == MMC_GO_INACTIVE_STATE
>> + || cmd->opcode == MMC_GO_IDLE_STATE);
>> +
>Aren’t you only interested in cmds that move to tran state from other state?
>According to the Device state transitions (table 61 in eMMC5.1) it only concern
>cmd7 (stby->tran), cmd12 (data->tran), and cmd14 (btst->tran).
>
>Thanks,
>Avri
No, I'd poll for any command where this is possible, so I only exclude the
ones not ending up in TRAN.
The TRAN->RCV->PRG->TRAN commands are the ones that need polling
to be race condition free.
Technically CMD16, 23 and so on (TRAN->TRAN directly) don't need the
CMD13 polling but it does not hurt either, especially because the cmd->opcode
itself overlaps for general and sd commands.
Tracking whether CMD55 was the last command would have been awkward and
CMD13 polling wherever possible seems the more sane option.
If adding a flag to disable any inter-CMD13 polling of none-R1b commands is
desired, I can add that, but I did not see a good reason for that right now.
Regards,
Christian
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
>Poll for TRAN state if the ioctl command will eventually return to TRAN
>
>The ioctl submitted command should not be considered completed until
>the card has returned back to TRAN state. Waiting just for the card
>to no longer signal busy is not enough as they might remain in a
>non-busy PROG state for a while after the command.
>Further commands requiring TRAN will fail then.
>It should not be the responsibility of the user to check if their command
>has completed until sending the next via ioctl,
>instead the check should be made here.
>So now, in doubt, wait for TRAN except for the few commands that will
>never return to TRAN state.
So apart from the fact that I missed a couple of non-TRAN returning MMC
commands, which I will add in v2, are there any other thoughts about this
patch? It would change the behavior of the ioctl interface, but I think it is
the only way to prevent race conditions here.
Best Regards,
Christian
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
On Fri, 18 Jun 2021 at 12:00, Christian Löhle <[email protected]> wrote:
>
> >Poll for TRAN state if the ioctl command will eventually return to TRAN
> >
> >The ioctl submitted command should not be considered completed until
> >the card has returned back to TRAN state. Waiting just for the card
> >to no longer signal busy is not enough as they might remain in a
> >non-busy PROG state for a while after the command.
> >Further commands requiring TRAN will fail then.
> >It should not be the responsibility of the user to check if their command
> >has completed until sending the next via ioctl,
> >instead the check should be made here.
> >So now, in doubt, wait for TRAN except for the few commands that will
> >never return to TRAN state.
>
> So apart from the fact that I missed a couple of non-TRAN returning MMC
> commands, which I will add in v2, are there any other thoughts about this
> patch? It would change the behavior of the ioctl interface, but I think it is
> the only way to prevent race conditions here.
I need some more time to review, but feel free to post a v2, I can
look at that instead.
Kind regards
Uffe
>
> Best Regards,
> Christian
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>
Sorry Uffe, Im not quite ready and need some more testing myself before v2.
But I can outline and reason the need for a (now bigger) patch already.
mmc_ready_for_data checks for R1_STATE_TRAN also, comment says something
about cards abusing status bits, please warn me if this is an actual issue and in
what direction (From the code I'm assuming R1_READY_FOR_DATA may be
set when the card is not actually ready, but R1_READY_FOR_DATA is not unset
'randomly')
But for v2 I would keep the ready_for_data check on R1b commands,
but actually only check for R1_READY_FOR_DATA without any state.
(This could include Shawn's hardware busy_detect patch).
Then afterwards, for return_to_tran commands, do additional CMD13 polling.
I'm still struggling to get this 'clean' and race-condition free, because of e.g.
CMD5 MMC_SLEEP_AWAKE always signals busy, but only transitions to TRAN if
sleep bit is not set. Busy detection should be done in both cases, though.
(I then should be able to send MMC_SLEEP_AWAKE sleep and
MMC_SLEEP_AWAKE awake and MMC_SLEEP_AWAKE sleep in direct
succession from user space through the ioctl interface without any further
considerations from there.)
But clearly MMC_SLEEP_AWAKE sleep will never respond to a CMD13 with
busy bit unset, as it is in sleep then. (Shawns's hardware patch could be used
as a best effort.)
Maybe I'm overthinking this for an edge case of a rarely used ioctl interface,
but that is my status so far.
If I leave out perfect MMC_SLEEP_AWAKE behavior (and maybe something else)
then:
if (rpmb or R1b response)
card_busy_detect (status bit only)
if (return_to_tran_cmd)
card_poll_until_tran (using cmd13)
would be ideal.
Feel free to comment on my thoughts so far, until I've tested it some more.
Best Regards,
Christian
From: Ulf Hansson <[email protected]>
>I need some more time to review, but feel free to post a v2, I can
>look at that instead.
>
>Kind regards
>Uffe
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782