diff mbox series

mmc: block: Use .card_busy() to detect busy state in card_busy_detect

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

Commit Message

Shawn Lin June 7, 2021, 9:18 a.m. UTC
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(+)

Comments

Christian Löhle June 8, 2021, 6:28 a.m. UTC | #1
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
Shawn Lin June 8, 2021, 6:58 a.m. UTC | #2
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;

>
Christian Löhle June 8, 2021, 7:51 a.m. UTC | #3
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 mbox series

Patch

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;