| Message ID | 1623057495-63363-1-git-send-email-shawn.lin@rock-chips.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: block: Use .card_busy() to detect busy state in card_busy_detect | expand |
Hey Shawn, You're assuming a card not signalling busy indicates TRAN state, and set the state manually, but a card might not be pulling DAT lines in PROG state. I will send a patch later that reworks card_busy_detect, as there are some issues with some command sequences in practice. I'd ask to wait with the patch until then. Regards, Christian From: Shawn Lin <shawn.lin@rock-chips.com> Sent: Monday, June 7, 2021 11:18 AM To: Ulf Hansson <ulf.hansson@linaro.org> Cc: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; Shawn Lin <shawn.lin@rock-chips.com> Subject: [PATCH] mmc: block: Use .card_busy() to detect busy state in card_busy_detect No need to send CMD13 if host driver supports .card_busy(). Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/core/block.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 88f4c215..23623a9 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -417,10 +417,17 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); int err = 0; u32 status; + bool busy; do { bool done = time_after(jiffies, timeout); + if (card->host->ops->card_busy) { + busy = card->host->ops->card_busy(card->host); + status = busy ? 0 : R1_READY_FOR_DATA | R1_STATE_TRAN << 9; + goto cb; + } + err = __mmc_send_status(card, &status, 5); if (err) { dev_err(mmc_dev(card->host), @@ -442,6 +449,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, __func__, status); return -ETIMEDOUT; } +cb: } while (!mmc_ready_for_data(status)); return err; -- 2.7.4 Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz Managing Directors: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
Hi Christian On 2021/6/8 14:28, Christian Löhle wrote: > Hey Shawn, > You're assuming a card not signalling busy indicates TRAN state, and set the state manually, > but a card might not be pulling DAT lines in PROG state. Refer to JESD84-B51 for emmc spec, section 6.5.13 clearly says that. And SD spec V4 also has a similar statement in section 4.3.4. So I guess if that was the case you point out, most of all operations in mmc_ops.c would suffer from this. > I will send a patch later that reworks card_busy_detect, as there are some issues > with some command sequences in practice. I'd ask to wait with the patch until then. > > Regards, > Christian > > > From: Shawn Lin <shawn.lin@rock-chips.com> > Sent: Monday, June 7, 2021 11:18 AM > To: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; Shawn Lin <shawn.lin@rock-chips.com> > Subject: [PATCH] mmc: block: Use .card_busy() to detect busy state in card_busy_detect > > No need to send CMD13 if host driver supports .card_busy(). > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/core/block.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 88f4c215..23623a9 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -417,10 +417,17 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); > int err = 0; > u32 status; > + bool busy; > > do { > bool done = time_after(jiffies, timeout); > > + if (card->host->ops->card_busy) { > + busy = card->host->ops->card_busy(card->host); > + status = busy ? 0 : R1_READY_FOR_DATA | R1_STATE_TRAN << 9; > + goto cb; > + } > + > err = __mmc_send_status(card, &status, 5); > if (err) { > dev_err(mmc_dev(card->host), > @@ -442,6 +449,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, > __func__, status); > return -ETIMEDOUT; > } > +cb: > } while (!mmc_ready_for_data(status)); > > return err; >
From: Shawn Lin <shawn.lin@rock-chips.com> Sent: Tuesday, June 8, 2021 8:58 AM >>You're assuming a card not signalling busy indicates TRAN state, and >>set the state manually, but a card might not be pulling DAT lines in PROG state >> >Refer to JESD84-B51 for emmc spec, section 6.5.13 clearly says that. And >SD spec V4 also has a similar statement in section 4.3.4. > >So I guess if that was the case you point out, most of all operations in >mmc_ops.c would suffer from this. Im not sure what youre referring to as 6.5.13 is inside of 6.5 (no further subsections) in JESD84-B51 and 4.3.4 in the SD spec is "4.3.4. Data Write" The patch itself is fine, i just think that card_busy_detect should not actually be a busy detect but rather a TRAN detect. I will send a patch now, but if anyone wants to check this out, a full SDSC card that receives CMD16->CMD42 Lock -> CMD42 Force Erase -> CMD16 (Reset to 512) -> Read like CMD17 will hit this race condition pretty consistently as the CMD42 Force Erase will be in PROG for a while on most cards. If the commands are sent through ioctl in a batch that is. If card_busy_detect is changed to card_tran_detect then using .card_busy no longer makes sense. Regards, Christian 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 88f4c215..23623a9 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -417,10 +417,17 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); int err = 0; u32 status; + bool busy; do { bool done = time_after(jiffies, timeout); + if (card->host->ops->card_busy) { + busy = card->host->ops->card_busy(card->host); + status = busy ? 0 : R1_READY_FOR_DATA | R1_STATE_TRAN << 9; + goto cb; + } + err = __mmc_send_status(card, &status, 5); if (err) { dev_err(mmc_dev(card->host), @@ -442,6 +449,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms, __func__, status); return -ETIMEDOUT; } +cb: } while (!mmc_ready_for_data(status)); return err;
No need to send CMD13 if host driver supports .card_busy(). Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/core/block.c | 8 ++++++++ 1 file changed, 8 insertions(+)