[PATCH] mmc: block: workaround long ioctl busy timeout

[PATCH] mmc: block: workaround long ioctl busy timeout

[Christian Löhle]

The ioctl interface allowed to set cmd_timeout_ms when polling for
busy on R1B commands. This was often limited by the max hw timeout
so work around it like for the sanitize command.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/block.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 20da7ed43e6d..ba3bc9014179 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -472,6 +472,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	struct scatterlist sg;
 	int err;
 	unsigned int target_part;
+	unsigned int busy_timeout = MMC_BLK_TIMEOUT_MS;
+	int poll_prog = false;
 
 	if (!card || !md || !idata)
 		return -EINVAL;
@@ -493,6 +495,12 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	cmd.opcode = idata->ic.opcode;
 	cmd.arg = idata->ic.arg;
 	cmd.flags = idata->ic.flags;
+	/* R1B flag might be removed here to work around hw, so save it */
+	poll_prog = (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B);
+	busy_timeout = idata->ic.cmd_timeout_ms ? :
+		MMC_BLK_TIMEOUT_MS;
+	if (poll_prog)
+		mmc_prepare_busy_cmd(card->host, &cmd, busy_timeout);
 
 	if (idata->buf_bytes) {
 		data.sg = &sg;
@@ -596,7 +604,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	if (idata->ic.postsleep_min_us)
 		usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
 
-	if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
+	if (poll_prog) {
 		/*
 		 * Ensure RPMB/R1B command has completed by polling CMD13 "Send Status". Here we
 		 * allow to override the default timeout value if a custom timeout is specified.
-- 
2.37.3


Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

RE: [PATCH] mmc: block: workaround long ioctl busy timeout

[Avri Altman]

> 
> The ioctl interface allowed to set cmd_timeout_ms when polling for busy on
> R1B commands. This was often limited by the max hw timeout so work around
> it like for the sanitize command.
> 
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/block.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> 20da7ed43e6d..ba3bc9014179 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -472,6 +472,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> *card, struct mmc_blk_data *md,
>         struct scatterlist sg;
>         int err;
>         unsigned int target_part;
> +       unsigned int busy_timeout = MMC_BLK_TIMEOUT_MS;
> +       int poll_prog = false;
> 
>         if (!card || !md || !idata)
>                 return -EINVAL;
> @@ -493,6 +495,12 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> *card, struct mmc_blk_data *md,
>         cmd.opcode = idata->ic.opcode;
>         cmd.arg = idata->ic.arg;
>         cmd.flags = idata->ic.flags;
> +       /* R1B flag might be removed here to work around hw, so save it */
> +       poll_prog = (idata->rpmb || (cmd.flags & MMC_RSP_R1B) ==
> MMC_RSP_R1B);
> +       busy_timeout = idata->ic.cmd_timeout_ms ? :
> +               MMC_BLK_TIMEOUT_MS;
Isn't commit 23e09be254f9 already introduced the very same thing?
Meaning mmc_poll_for_busy() is already called with the appropriate timeout?

Thanks,
Avri

> +       if (poll_prog)
> +               mmc_prepare_busy_cmd(card->host, &cmd, busy_timeout);
> 
>         if (idata->buf_bytes) {
>                 data.sg = &sg;
> @@ -596,7 +604,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> *card, struct mmc_blk_data *md,
>         if (idata->ic.postsleep_min_us)
>                 usleep_range(idata->ic.postsleep_min_us, idata-
> >ic.postsleep_max_us);
> 
> -       if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
> +       if (poll_prog) {
>                 /*
>                  * Ensure RPMB/R1B command has completed by polling CMD13
> "Send Status". Here we
>                  * allow to override the default timeout value if a custom timeout is
> specified.
> --
> 2.37.3
> 
> 
> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz Managing Director:
> Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782

RE: [PATCH] mmc: block: workaround long ioctl busy timeout

[Christian Löhle]

>>Subject: RE: [PATCH] mmc: block: workaround long ioctl busy timeout
>>         cmd.opcode = idata->ic.opcode;
>>         cmd.arg = idata->ic.arg;
>>         cmd.flags = idata->ic.flags;
>> +       /* R1B flag might be removed here to work around hw, so save it */
>> +       poll_prog = (idata->rpmb || (cmd.flags & MMC_RSP_R1B) ==
>> MMC_RSP_R1B);
>> +       busy_timeout = idata->ic.cmd_timeout_ms ? :
>> +               MMC_BLK_TIMEOUT_MS;
> Isn't commit 23e09be254f9 already introduced the very same thing?
> Meaning mmc_poll_for_busy() is already called with the appropriate timeout?

mmc_poll_for_busy() is, but the problem is already at
mmc_wait_for_req(card->host, &mrq);
Drivers like SDHCI will setup their hardware timer for (in SDHCI) the data inhibit bit.
Drivers dont check if the set timeout is above their capabilities, that's why sanitize also
removes the busy flag.
So without this patch and issuing a secure erase that takes reasonably long, it will fail like so:

[  464.749702] Polling 19507500ms / 19507500ms for busy: CMD38 : 80000000
[  545.761530] mmc2: Timeout waiting for hardware interrupt.
[  545.762623] mmc2: sdhci: ============ SDHCI REGISTER DUMP ===========
[  545.763199] mmc2: sdhci: Sys addr:  0x00000000 | Version:  0x00001002
[  545.763776] mmc2: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000000
[  545.764353] mmc2: sdhci: Argument:  0x80000000 | Trn mode: 0x00000013
[  545.764928] mmc2: sdhci: Present:   0x1fef0006 | Host ctl: 0x00000035
[  545.765504] mmc2: sdhci: Power:     0x0000000b | Blk gap:  0x00000080
[  545.766080] mmc2: sdhci: Wake-up:   0x00000000 | Clock:    0x00000207
[  545.766656] mmc2: sdhci: Timeout:   0x0000000e | Int stat: 0x00000000
[  545.767231] mmc2: sdhci: Int enab:  0x02ff000b | Sig enab: 0x02ff000b
[  545.767807] mmc2: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[  545.768382] mmc2: sdhci: Caps:      0x44edc880 | Caps_1:   0x800020f7
[  545.768959] mmc2: sdhci: Cmd:       0x0000261b | Max curr: 0x00000000
[  545.769534] mmc2: sdhci: Resp[0]:   0x00000800 | Resp[1]:  0xfff6dbff
[  545.770110] mmc2: sdhci: Resp[2]:   0x329f5903 | Resp[3]:  0x00900f00
[  545.770686] mmc2: sdhci: Host ctl2: 0x00000000
[  545.771089] mmc2: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0x0b4b1208
[  545.771665] mmc2: sdhci: ============================================
[  545.773325] sdhci-arasan fe330000.mmc: __mmc_blk_ioctl_cmd: CMD38 cmd error -110

(First print added by me, shows cmd_timeout_ms set by mmc-utils)
Erroring out already at
if (cmd.error) {
	dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
					__func__, cmd.error);
	return cmd.error;
}
i.e. timeout set by user space is  being limited by the max hardware timeout.

Regards,
Christian

>
> Thanks,
> Avri

Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

RE: [PATCH] mmc: block: workaround long ioctl busy timeout

[Christian Löhle]

Any more comments regarding this patch?

-----Original Message-----
From: Christian Löhle 
Sent: Montag, 16. Januar 2023 15:38
To: adrian.hunter@intel.com; 'Avri Altman' <Avri.Altman@wdc.com>; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: ulf.hansson@linaro.org
Subject: [PATCH] mmc: block: workaround long ioctl busy timeout

The ioctl interface allowed to set cmd_timeout_ms when polling for busy on R1B commands. This was often limited by the max hw timeout so work around it like for the sanitize command.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/block.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 20da7ed43e6d..ba3bc9014179 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -472,6 +472,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	struct scatterlist sg;
 	int err;
 	unsigned int target_part;
+	unsigned int busy_timeout = MMC_BLK_TIMEOUT_MS;
+	int poll_prog = false;
 
 	if (!card || !md || !idata)
 		return -EINVAL;
@@ -493,6 +495,12 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	cmd.opcode = idata->ic.opcode;
 	cmd.arg = idata->ic.arg;
 	cmd.flags = idata->ic.flags;
+	/* R1B flag might be removed here to work around hw, so save it */
+	poll_prog = (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B);
+	busy_timeout = idata->ic.cmd_timeout_ms ? :
+		MMC_BLK_TIMEOUT_MS;
+	if (poll_prog)
+		mmc_prepare_busy_cmd(card->host, &cmd, busy_timeout);
 
 	if (idata->buf_bytes) {
 		data.sg = &sg;
@@ -596,7 +604,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 	if (idata->ic.postsleep_min_us)
 		usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
 
-	if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
+	if (poll_prog) {
 		/*
 		 * Ensure RPMB/R1B command has completed by polling CMD13 "Send Status". Here we
 		 * allow to override the default timeout value if a custom timeout is specified.
--
2.37.3


Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCH] mmc: block: workaround long ioctl busy timeout

[Ulf Hansson]

On Mon, 16 Jan 2023 at 15:38, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> The ioctl interface allowed to set cmd_timeout_ms when polling for
> busy on R1B commands. This was often limited by the max hw timeout
> so work around it like for the sanitize command.
>
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/block.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 20da7ed43e6d..ba3bc9014179 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -472,6 +472,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>         struct scatterlist sg;
>         int err;
>         unsigned int target_part;
> +       unsigned int busy_timeout = MMC_BLK_TIMEOUT_MS;
> +       int poll_prog = false;
>
>         if (!card || !md || !idata)
>                 return -EINVAL;
> @@ -493,6 +495,12 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>         cmd.opcode = idata->ic.opcode;
>         cmd.arg = idata->ic.arg;
>         cmd.flags = idata->ic.flags;
> +       /* R1B flag might be removed here to work around hw, so save it */
> +       poll_prog = (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B);
> +       busy_timeout = idata->ic.cmd_timeout_ms ? :
> +               MMC_BLK_TIMEOUT_MS;
> +       if (poll_prog)
> +               mmc_prepare_busy_cmd(card->host, &cmd, busy_timeout);

This may end up using R1B for rpmb commands, we don't want that.

>
>         if (idata->buf_bytes) {
>                 data.sg = &sg;
> @@ -596,7 +604,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>         if (idata->ic.postsleep_min_us)
>                 usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
>
> -       if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
> +       if (poll_prog) {

If we end up using R1B and the host supports HW busy detection, we
should skip polling.

>                 /*
>                  * Ensure RPMB/R1B command has completed by polling CMD13 "Send Status". Here we
>                  * allow to override the default timeout value if a custom timeout is specified.

I believe I understand the problem you are trying to address in the
$subject patch. Let me try to help out, by sending a re-worked patch
to try to cover the things I pointed out above in a more consistent
way.

Kind regards
Uffe

Re: [PATCH] mmc: block: workaround long ioctl busy timeout

[kernel test robot]

Hi Christian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc8 next-20230213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-L-hle/mmc-block-workaround-long-ioctl-busy-timeout/20230213-183603
patch link:    https://lore.kernel.org/r/8f4a0fc6f2e64ef091784c5cd704c113%40hyperstone.com
patch subject: [PATCH] mmc: block: workaround long ioctl busy timeout
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20230214/202302140314.idOxiF7x-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/94406ad8626aa2d7761fb7eb20a918a4805ea667
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-L-hle/mmc-block-workaround-long-ioctl-busy-timeout/20230213-183603
        git checkout 94406ad8626aa2d7761fb7eb20a918a4805ea667
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302140314.idOxiF7x-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "mmc_prepare_busy_cmd" [drivers/mmc/core/mmc_block.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH] mmc: block: workaround long ioctl busy timeout

[kernel test robot]

Hi Christian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc8 next-20230213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-L-hle/mmc-block-workaround-long-ioctl-busy-timeout/20230213-183603
patch link:    https://lore.kernel.org/r/8f4a0fc6f2e64ef091784c5cd704c113%40hyperstone.com
patch subject: [PATCH] mmc: block: workaround long ioctl busy timeout
config: x86_64-randconfig-a003-20230213 (https://download.01.org/0day-ci/archive/20230214/202302140909.X9OHVtn2-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/94406ad8626aa2d7761fb7eb20a918a4805ea667
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-L-hle/mmc-block-workaround-long-ioctl-busy-timeout/20230213-183603
        git checkout 94406ad8626aa2d7761fb7eb20a918a4805ea667
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302140909.X9OHVtn2-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "mmc_prepare_busy_cmd" [drivers/mmc/core/mmc_block.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests