| Message ID | 68590206e8b044a2a71457cbbeda0794@hyperstone.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: block: workaround long ioctl busy timeout | expand |
> > 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
>>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
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
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
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!
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!
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.
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(-)