1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check

Ben Dooks posted 1 patch 3 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250724105807.697915-1-ben.dooks@codethink.co.uk
Maintainers: "Philippe Mathieu-Daudé" <philmd@linaro.org>, Bin Meng <bmeng.cn@gmail.com>
hw/sd/sd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
Posted by Ben Dooks 3 months, 3 weeks ago 
The addition of specific handlers for mmc-spi for SEND_CSD and
SEND_CID has broken at least Linux and possibly also u-boot's
mmc-spi code.

It looks like when adding the code, it is checking for these
commands to not be in sd_standby_state but the check looks to
have been accidentally reversed (see below)

     if (sd->state != sd_standby_state) {
         return sd_invalid_state_for_cmd(sd, req);
     }

Linux shows the following:

[    0.293983] Waiting for root device /dev/mmcblk0...
[    1.363071] mmc0: error -38 whilst initialising SD card
[    2.418566] mmc0: error -38 whilst initialising SD card

Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)")
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 hw/sd/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 49fc79cf8a..e6c1ba7c5d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd, SDRequest req)
 /* CMD9 */
 static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
 {
-    if (sd->state != sd_standby_state) {
+    if (sd->state == sd_standby_state) {
         return sd_invalid_state_for_cmd(sd, req);
     }
     return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
@@ -1336,7 +1336,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
 /* CMD10 */
 static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
 {
-    if (sd->state != sd_standby_state) {
+    if (sd->state == sd_standby_state) {
         return sd_invalid_state_for_cmd(sd, req);
     }
     return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
@@ -1345,7 +1345,7 @@ static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
 
 static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req)
 {
-    if (sd->state != sd_standby_state) {
+    if (sd->state == sd_standby_state) {
         return sd_invalid_state_for_cmd(sd, req);
     }
 
-- 
2.37.2.352.g3c44437643
Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago 
Hi Ben,

On 24/7/25 12:58, Ben Dooks wrote:
> The addition of specific handlers for mmc-spi for SEND_CSD and
> SEND_CID has broken at least Linux and possibly also u-boot's
> mmc-spi code.
> 
> It looks like when adding the code, it is checking for these
> commands to not be in sd_standby_state but the check looks to
> have been accidentally reversed (see below)
> 
>       if (sd->state != sd_standby_state) {
>           return sd_invalid_state_for_cmd(sd, req);
>       }
> 
> Linux shows the following:
> 
> [    0.293983] Waiting for root device /dev/mmcblk0...
> [    1.363071] mmc0: error -38 whilst initialising SD card
> [    2.418566] mmc0: error -38 whilst initialising SD card
> 
> Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)")
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>   hw/sd/sd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 49fc79cf8a..e6c1ba7c5d 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd, SDRequest req)
>   /* CMD9 */
>   static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
>   {
> -    if (sd->state != sd_standby_state) {
> +    if (sd->state == sd_standby_state) {

This happens to work in your case by luck.

Since we switch to sd_sendingdata_state in sd_cmd_to_sendingdata(),
we need to check for sd_transfer_state -- the spec is wrong here! --.

Btw sd_cmd_to_sendingdata() already checks for that.


>           return sd_invalid_state_for_cmd(sd, req);
>       }
>       return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
> @@ -1336,7 +1336,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
>   /* CMD10 */
>   static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
>   {
> -    if (sd->state != sd_standby_state) {
> +    if (sd->state == sd_standby_state) {

Ditto.

>           return sd_invalid_state_for_cmd(sd, req);
>       }
>       return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
> @@ -1345,7 +1345,7 @@ static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
>   
>   static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req)
>   {
> -    if (sd->state != sd_standby_state) {
> +    if (sd->state == sd_standby_state) {

Nack, you just broke SD card implementation. Have a look at
the "4.8 Card State Transition Table" in the spec.

>           return sd_invalid_state_for_cmd(sd, req);
>       }
>   
Regards,
Phil.
Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
Posted by Guenter Roeck 3 months, 2 weeks ago 
On Tue, Jul 29, 2025 at 03:51:33PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Ben,
> 
> On 24/7/25 12:58, Ben Dooks wrote:
> > The addition of specific handlers for mmc-spi for SEND_CSD and
> > SEND_CID has broken at least Linux and possibly also u-boot's
> > mmc-spi code.
> > 
> > It looks like when adding the code, it is checking for these
> > commands to not be in sd_standby_state but the check looks to
> > have been accidentally reversed (see below)
> > 
> >       if (sd->state != sd_standby_state) {
> >           return sd_invalid_state_for_cmd(sd, req);
> >       }
> > 
> > Linux shows the following:
> > 
> > [    0.293983] Waiting for root device /dev/mmcblk0...
> > [    1.363071] mmc0: error -38 whilst initialising SD card
> > [    2.418566] mmc0: error -38 whilst initialising SD card
> > 
> > Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)")
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> >   hw/sd/sd.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 49fc79cf8a..e6c1ba7c5d 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd, SDRequest req)
> >   /* CMD9 */
> >   static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
> >   {
> > -    if (sd->state != sd_standby_state) {
> > +    if (sd->state == sd_standby_state) {
> 
> This happens to work in your case by luck.
> 
> Since we switch to sd_sendingdata_state in sd_cmd_to_sendingdata(),
> we need to check for sd_transfer_state -- the spec is wrong here! --.
> 
> Btw sd_cmd_to_sendingdata() already checks for that.
> 

In my fix (the one I am carrying downstream) I have

-    if (sd->state != sd_standby_state) {
+    if (sd->state != sd_transfer_state) {

in spi_cmd_SEND_CSD() and spi_cmd_SEND_CID(), together with

-        return sd_r2_s;
+        return sd_r1;

in sd_cmd_SEND_STATUS().

Guenter

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.