[PATCH v3] mmc: core: Return correct emmc response in case of ioctl error

[PATCH v3] mmc: core: Return correct emmc response in case of ioctl error

[Nishad Kamdar]

When a read/write command is sent via ioctl to the kernel,
and the command fails, the actual error response of the emmc
is not sent to the user.

IOCTL read/write tests are carried out using commands
17 (Single BLock Read), 24 (Single Block Write),
18 (Multi Block Read), 25 (Multi Block Write)

The tests are carried out on a 64Gb emmc device. All of these
tests try to access an "out of range" sector address (0x09B2FFFF).

It is seen that without the patch the response received by the user
is not OUT_OF_RANGE error (R1 response 31st bit is not set) as per
JEDEC specification. After applying the patch proper response is seen.
This is because the function returns without copying the response to
the user in case of failure. This patch fixes the issue.

The test code and the output of only the CMD17 is included in the
commit to limit the message length.

CMD17 (Test Code Snippet):
==========================
        printf("Forming CMD%d\n", opt_idx);
        /*  single block read */
        cmd.blksz = 512;
        cmd.blocks = 1;
        cmd.write_flag = 0;
        cmd.opcode = 17;
        //cmd.arg = atoi(argv[3]);
        cmd.arg = 0x09B2FFFF;
        /* Expecting response R1B */
        cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;

        memset(data, 0, sizeof(__u8) * 512);
        mmc_ioc_cmd_set_data(cmd, data);

        printf("Sending CMD%d: ARG[0x%08x]\n", opt_idx, cmd.arg);
        if(ioctl(fd, MMC_IOC_CMD, &cmd))
                perror("Error");

        printf("\nResponse: %08x\n", cmd.response[0]);

CMD17 (Output without patch):
=============================
test@test-LIVA-Z:~$ sudo ./mmc cmd_test /dev/mmcblk0 17
Entering the do_mmc_commands:Device: /dev/mmcblk0 nargs:4
Entering the do_mmc_commands:Device: /dev/mmcblk0 options[17, 0x09B2FFF]
Forming CMD17
Sending CMD17: ARG[0x09b2ffff]
Error: Connection timed out

Response: 00000000
(Incorrect response)

CMD17 (Output with patch):
==========================
test@test-LIVA-Z:~$ sudo ./mmc cmd_test /dev/mmcblk0 17
[sudo] password for test:
Entering the do_mmc_commands:Device: /dev/mmcblk0 nargs:4
Entering the do_mmc_commands:Device: /dev/mmcblk0 options[17, 09B2FFFF]
Forming CMD17
Sending CMD17: ARG[0x09b2ffff]
Error: Connection timed out

Response: 80000900
(Correct OUT_OF_ERROR response as per JEDEC specification)

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
---
Changes in v2:
  - Make commit message clearer by adding test cases as outputs.
Changes in v3:
  - Shorten the commit message to include only CMD17 related
    code and test.

 drivers/mmc/core/block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index a9ad9f5fa9491..efa92aa7e2368 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -522,11 +522,13 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	if (cmd.error) {
 		dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
 						__func__, cmd.error);
+		memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));
 		return cmd.error;
 	}
 	if (data.error) {
 		dev_err(mmc_dev(card->host), "%s: data error %d\n",
 						__func__, data.error);
+		memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));
 		return data.error;
 	}
 
-- 
2.17.1

Re: [PATCH v3] mmc: core: Return correct emmc response in case of ioctl error

[Ulf Hansson]

On Thu, 12 Aug 2021 at 08:57, Nishad Kamdar <nishadkamdar@gmail.com> wrote:
>
> When a read/write command is sent via ioctl to the kernel,
> and the command fails, the actual error response of the emmc
> is not sent to the user.
>
> IOCTL read/write tests are carried out using commands
> 17 (Single BLock Read), 24 (Single Block Write),
> 18 (Multi Block Read), 25 (Multi Block Write)
>
> The tests are carried out on a 64Gb emmc device. All of these
> tests try to access an "out of range" sector address (0x09B2FFFF).
>
> It is seen that without the patch the response received by the user
> is not OUT_OF_RANGE error (R1 response 31st bit is not set) as per
> JEDEC specification. After applying the patch proper response is seen.
> This is because the function returns without copying the response to
> the user in case of failure. This patch fixes the issue.
>
> The test code and the output of only the CMD17 is included in the
> commit to limit the message length.
>
> CMD17 (Test Code Snippet):
> ==========================
>         printf("Forming CMD%d\n", opt_idx);
>         /*  single block read */
>         cmd.blksz = 512;
>         cmd.blocks = 1;
>         cmd.write_flag = 0;
>         cmd.opcode = 17;
>         //cmd.arg = atoi(argv[3]);
>         cmd.arg = 0x09B2FFFF;
>         /* Expecting response R1B */
>         cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
>
>         memset(data, 0, sizeof(__u8) * 512);
>         mmc_ioc_cmd_set_data(cmd, data);
>
>         printf("Sending CMD%d: ARG[0x%08x]\n", opt_idx, cmd.arg);
>         if(ioctl(fd, MMC_IOC_CMD, &cmd))
>                 perror("Error");
>
>         printf("\nResponse: %08x\n", cmd.response[0]);
>
> CMD17 (Output without patch):
> =============================
> test@test-LIVA-Z:~$ sudo ./mmc cmd_test /dev/mmcblk0 17
> Entering the do_mmc_commands:Device: /dev/mmcblk0 nargs:4
> Entering the do_mmc_commands:Device: /dev/mmcblk0 options[17, 0x09B2FFF]
> Forming CMD17
> Sending CMD17: ARG[0x09b2ffff]
> Error: Connection timed out
>
> Response: 00000000
> (Incorrect response)
>
> CMD17 (Output with patch):
> ==========================
> test@test-LIVA-Z:~$ sudo ./mmc cmd_test /dev/mmcblk0 17
> [sudo] password for test:
> Entering the do_mmc_commands:Device: /dev/mmcblk0 nargs:4
> Entering the do_mmc_commands:Device: /dev/mmcblk0 options[17, 09B2FFFF]
> Forming CMD17
> Sending CMD17: ARG[0x09b2ffff]
> Error: Connection timed out
>
> Response: 80000900
> (Correct OUT_OF_ERROR response as per JEDEC specification)
>
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> ---
> Changes in v2:
>   - Make commit message clearer by adding test cases as outputs.
> Changes in v3:
>   - Shorten the commit message to include only CMD17 related
>     code and test.
>
>  drivers/mmc/core/block.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index a9ad9f5fa9491..efa92aa7e2368 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -522,11 +522,13 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>         if (cmd.error) {
>                 dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
>                                                 __func__, cmd.error);
> +               memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));
>                 return cmd.error;
>         }
>         if (data.error) {
>                 dev_err(mmc_dev(card->host), "%s: data error %d\n",
>                                                 __func__, data.error);
> +               memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));

It looks like we should do this memcpy, no matter whether we get an
error response or not.

In other words, I suggest you move the existing
"memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));" from a
couple of lines further done in the code, up to immediately after we
have called mmc_wait_for_req(). That should make it more clear as
well, I think.

>                 return data.error;
>         }
>
> --
> 2.17.1
>

Kind regards
Uffe

Re: [PATCH v3] mmc: core: Return correct emmc response in case of ioctl error

[Nishad Kamdar]

On Tue, Aug 24, 2021 at 02:44:42PM +0200, Ulf Hansson wrote:
> On Thu, 12 Aug 2021 at 08:57, Nishad Kamdar <nishadkamdar@gmail.com> wrote:
> >
> > When a read/write command is sent via ioctl to the kernel,
> > and the command fails, the actual error response of the emmc
> > is not sent to the user.
> >
> > IOCTL read/write tests are carried out using commands
> > 17 (Single BLock Read), 24 (Single Block Write),
> > 18 (Multi Block Read), 25 (Multi Block Write)
> >
> > The tests are carried out on a 64Gb emmc device. All of these
> > tests try to access an "out of range" sector address (0x09B2FFFF).
> >
> > It is seen that without the patch the response received by the user
> > is not OUT_OF_RANGE error (R1 response 31st bit is not set) as per
> > JEDEC specification. After applying the patch proper response is seen.
> > This is because the function returns without copying the response to
> > the user in case of failure. This patch fixes the issue.
> >
> > The test code and the output of only the CMD17 is included in the
> > commit to limit the message length.
> >
> > CMD17 (Test Code Snippet):
> > ==========================
> >         printf("Forming CMD%d\n", opt_idx);
> >         /*  single block read */
> >         cmd.blksz = 512;
> >         cmd.blocks = 1;
> >         cmd.write_flag = 0;
> >         cmd.opcode = 17;
> >         //cmd.arg = atoi(argv[3]);
> >         cmd.arg = 0x09B2FFFF;
> >         /* Expecting response R1B */
> >         cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> >
> >         memset(data, 0, sizeof(__u8) * 512);
> >         mmc_ioc_cmd_set_data(cmd, data);
> >
> >         printf("Sending CMD%d: ARG[0x%08x]\n", opt_idx, cmd.arg);
> >         if(ioctl(fd, MMC_IOC_CMD, &cmd))
> >                 perror("Error");
> >
> >         printf("\nResponse: %08x\n", cmd.response[0]);
> >
> > CMD17 (Output without patch):
> > =============================
> > test@test-LIVA-Z:~$ sudo ./mmc cmd_test /dev/mmcblk0 17
> > Entering the do_mmc_commands:Device: /dev/mmcblk0 nargs:4
> > Entering the do_mmc_commands:Device: /dev/mmcblk0 options[17, 0x09B2FFF]
> > Forming CMD17
> > Sending CMD17: ARG[0x09b2ffff]
> > Error: Connection timed out
> >
> > Response: 00000000
> > (Incorrect response)
> >
> > CMD17 (Output with patch):
> > ==========================
> > test@test-LIVA-Z:~$ sudo ./mmc cmd_test /dev/mmcblk0 17
> > [sudo] password for test:
> > Entering the do_mmc_commands:Device: /dev/mmcblk0 nargs:4
> > Entering the do_mmc_commands:Device: /dev/mmcblk0 options[17, 09B2FFFF]
> > Forming CMD17
> > Sending CMD17: ARG[0x09b2ffff]
> > Error: Connection timed out
> >
> > Response: 80000900
> > (Correct OUT_OF_ERROR response as per JEDEC specification)
> >
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > Reviewed-by: Avri Altman <avri.altman@wdc.com>
> > ---
> > Changes in v2:
> >   - Make commit message clearer by adding test cases as outputs.
> > Changes in v3:
> >   - Shorten the commit message to include only CMD17 related
> >     code and test.
> >
> >  drivers/mmc/core/block.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index a9ad9f5fa9491..efa92aa7e2368 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -522,11 +522,13 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
> >         if (cmd.error) {
> >                 dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
> >                                                 __func__, cmd.error);
> > +               memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));
> >                 return cmd.error;
> >         }
> >         if (data.error) {
> >                 dev_err(mmc_dev(card->host), "%s: data error %d\n",
> >                                                 __func__, data.error);
> > +               memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp));
> 
> It looks like we should do this memcpy, no matter whether we get an
> error response or not.
> 
> In other words, I suggest you move the existing
> "memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));" from a
> couple of lines further done in the code, up to immediately after we
> have called mmc_wait_for_req(). That should make it more clear as
> well, I think.
> 
I agree. I Have sent the updated version of the patch with this change.
Kindly review the same as well.

Thanks for the comment and review.

Regards,
Nishad
> >                 return data.error;
> >         }
> >
> > --
> > 2.17.1
> >
> 
> Kind regards
> Uffe