diff mbox series

[v1,2/2] mmc: core: Allows to override the timeout value for ioctl() path

Message ID 20220423221623.1074556-3-huobean@gmail.com
State New
Headers show
Series None | expand

Commit Message

Bean Huo April 23, 2022, 10:16 p.m. UTC 
From: Bean Huo <beanhuo@micron.com>

Occasionally, user-land applications initiate longer timeout values for certain commands
through ioctl() system call. But so far we are still using a fixed timeout of 10 seconds
in mmc_poll_for_busy() on the ioctl() path, even if a custom timeout is specified in the
userspace application. This patch allows custom timeout values to override this default
timeout values on the ioctl path.

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/mmc/core/block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Avri Altman April 24, 2022, 11:52 a.m. UTC | #1 
> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Occasionally, user-land applications initiate longer timeout values for certain
> commands through ioctl() system call. But so far we are still using a fixed
> timeout of 10 seconds in mmc_poll_for_busy() on the ioctl() path, even if a
> custom timeout is specified in the userspace application. This patch allows
> custom timeout values to override this default timeout values on the ioctl
> path.
> 
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>
Linus Walleij April 24, 2022, 1:29 p.m. UTC | #2 
On Sun, Apr 24, 2022 at 12:16 AM Bean Huo <huobean@gmail.com> wrote:

> From: Bean Huo <beanhuo@micron.com>
>
> Occasionally, user-land applications initiate longer timeout values for certain commands
> through ioctl() system call. But so far we are still using a fixed timeout of 10 seconds
> in mmc_poll_for_busy() on the ioctl() path, even if a custom timeout is specified in the
> userspace application. This patch allows custom timeout values to override this default
> timeout values on the ioctl path.
>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
(...)
>         if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
>                 /*
> -                * Ensure RPMB/R1B command has completed by polling CMD13
> -                * "Send Status".
> +                * 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.
>                  */
> -               err = mmc_poll_for_busy(card, MMC_BLK_TIMEOUT_MS, false,
> -                                       MMC_BUSY_IO);
> +               err = mmc_poll_for_busy(card, idata->ic.cmd_timeout_ms ? : MMC_BLK_TIMEOUT_MS,
> +                                       false, MMC_BUSY_IO);

I suppose it's OK (albeit dubious) that we have a userspace interface setting
a hardware-specific thing such as a timeout.

However: is MMC_BLK_TIMEOUT_MS even reasonable here? If you guys
know a better timeout for RPMB operations (from your experience)
what about defining MMC_RPMB_TIMEOUT_MS to something more
reasonable (and I suppose longer) and use that as fallback instead
of MMC_BLK_TIMEOUT_MS?

This knowledge (that RPMB commands can have long timeouts) is not
something that should be hidden in userspace.

Yours,
Linus Walleij
Bean Huo April 25, 2022, 8:01 p.m. UTC | #3 
On Sun, 2022-04-24 at 15:29 +0200, Linus Walleij wrote:
> >          if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) ==
> > MMC_RSP_R1B) {
> >                  /*
> > -                * Ensure RPMB/R1B command has completed by polling
> > CMD13
> > -                * "Send Status".
> > +                * 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.
> >                   */
> > -               err = mmc_poll_for_busy(card, MMC_BLK_TIMEOUT_MS,
> > false,
> > -                                       MMC_BUSY_IO);
> > +               err = mmc_poll_for_busy(card, idata-
> > >ic.cmd_timeout_ms ? : MMC_BLK_TIMEOUT_MS,
> > +                                       false, MMC_BUSY_IO);
> 
> I suppose it's OK (albeit dubious) that we have a userspace interface
> setting
> a hardware-specific thing such as a timeout.
> 
> However: is MMC_BLK_TIMEOUT_MS even reasonable here? If you guys
> know a better timeout for RPMB operations (from your experience)
> what about defining MMC_RPMB_TIMEOUT_MS to something more
> reasonable (and I suppose longer) and use that as fallback instead
> of MMC_BLK_TIMEOUT_MS?
> 
> This knowledge (that RPMB commands can have long timeouts) is not
> something that should be hidden in userspace.
> 

Hi Linus,


understand what you mean. I must say, it's hard to come up with a
uniform timeout value that works for all commands but also for all
vendors. Meanwhile, the MMC_BLK_TIMEOUT_MS here is not only for RPMB
commands but also for all commands (with R1B responses) issued by the
ioctl() system call. The current 10s timeout can cover almost 99% of
the scenarios. There are very few special cases that take more than
10s.

I think the current solution is the most flexible way, if the customer
wants to override the kernel default timeout, they know how to initiate
the correct timeout value in ioctl() based on their specific
hardware/software system. I don't know how to convince every maintainer
and reviewer if we don't want to give this permission or want to
maintain a unified timeout value in the kernel driver. Given that we
already have eMMC ioctl() support, and we've opened the door to allow
users to specify specific cmd_timeout_ms in struct mmc_ioc_cmd{},
please consider my change.

struct mmc_ioc_cmd {
      ...
        /*
         *Override driver-computed timeouts.  Note the difference in
units!
         */
        unsigned int data_timeout_ns;
        unsigned int cmd_timeout_ms;
...}



Kind regards,
Bean
Linus Walleij April 25, 2022, 8:15 p.m. UTC | #4 
On Mon, Apr 25, 2022 at 10:02 PM Bean Huo <huobean@gmail.com> wrote:

> I think the current solution is the most flexible way, if the customer
> wants to override the kernel default timeout, they know how to initiate
> the correct timeout value in ioctl() based on their specific
> hardware/software system. I don't know how to convince every maintainer
> and reviewer if we don't want to give this permission or want to
> maintain a unified timeout value in the kernel driver. Given that we
> already have eMMC ioctl() support, and we've opened the door to allow
> users to specify specific cmd_timeout_ms in struct mmc_ioc_cmd{},
> please consider my change.

The patch is fine, I'm just saying we should put another patch on
top that defines a RPMB default timeout and set it to something
high, such as a minute.

Yours,
Linus Walleij
Ulf Hansson April 26, 2022, 1:32 p.m. UTC | #5 
On Mon, 25 Apr 2022 at 22:15, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Apr 25, 2022 at 10:02 PM Bean Huo <huobean@gmail.com> wrote:
>
> > I think the current solution is the most flexible way, if the customer
> > wants to override the kernel default timeout, they know how to initiate
> > the correct timeout value in ioctl() based on their specific
> > hardware/software system. I don't know how to convince every maintainer
> > and reviewer if we don't want to give this permission or want to
> > maintain a unified timeout value in the kernel driver. Given that we
> > already have eMMC ioctl() support, and we've opened the door to allow
> > users to specify specific cmd_timeout_ms in struct mmc_ioc_cmd{},
> > please consider my change.
>
> The patch is fine, I'm just saying we should put another patch on
> top that defines a RPMB default timeout and set it to something
> high, such as a minute.

I am also okay with $subject patch - and I agree with you Linus, that
it sounds reasonable to pick something specific for RPMB. I guess the
question is rather what value to pick, but I guess Bean can have some
ideas for that, at least for Micron eMMCs. BTW, we do something very
similar for mmc_sanitize() already.

Kind regards
Uffe
Ulf Hansson April 26, 2022, 1:55 p.m. UTC | #6 
On Sun, 24 Apr 2022 at 00:16, Bean Huo <huobean@gmail.com> wrote:
>
> From: Bean Huo <beanhuo@micron.com>
>
> Occasionally, user-land applications initiate longer timeout values for certain commands
> through ioctl() system call. But so far we are still using a fixed timeout of 10 seconds
> in mmc_poll_for_busy() on the ioctl() path, even if a custom timeout is specified in the
> userspace application. This patch allows custom timeout values to override this default
> timeout values on the ioctl path.
>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Bean Huo <beanhuo@micron.com>

Applied for next, thanks!

Linus, I interpreted your earlier reply as a reviewed-by tag, so I
have added that. Please tell me, if you want me to drop it.

Kind regards
Uffe


> ---
>  drivers/mmc/core/block.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index b35e7a95798b..6cb701aa1abc 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -609,11 +609,11 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>
>         if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
>                 /*
> -                * Ensure RPMB/R1B command has completed by polling CMD13
> -                * "Send Status".
> +                * 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.
>                  */
> -               err = mmc_poll_for_busy(card, MMC_BLK_TIMEOUT_MS, false,
> -                                       MMC_BUSY_IO);
> +               err = mmc_poll_for_busy(card, idata->ic.cmd_timeout_ms ? : MMC_BLK_TIMEOUT_MS,
> +                                       false, MMC_BUSY_IO);
>         }
>
>         return err;
> --
> 2.34.1
>
Linus Walleij April 26, 2022, 3:35 p.m. UTC | #7 
On Tue, Apr 26, 2022 at 3:55 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Sun, 24 Apr 2022 at 00:16, Bean Huo <huobean@gmail.com> wrote:
> >
> > From: Bean Huo <beanhuo@micron.com>
> >
> > Occasionally, user-land applications initiate longer timeout values for certain commands
> > through ioctl() system call. But so far we are still using a fixed timeout of 10 seconds
> > in mmc_poll_for_busy() on the ioctl() path, even if a custom timeout is specified in the
> > userspace application. This patch allows custom timeout values to override this default
> > timeout values on the ioctl path.
> >
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
>
> Applied for next, thanks!
>
> Linus, I interpreted your earlier reply as a reviewed-by tag, so I
> have added that. Please tell me, if you want me to drop it.

That's fine, sorry for being unclear!

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b35e7a95798b..6cb701aa1abc 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -609,11 +609,11 @@  static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 
 	if (idata->rpmb || (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
 		/*
-		 * Ensure RPMB/R1B command has completed by polling CMD13
-		 * "Send Status".
+		 * 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.
 		 */
-		err = mmc_poll_for_busy(card, MMC_BLK_TIMEOUT_MS, false,
-					MMC_BUSY_IO);
+		err = mmc_poll_for_busy(card, idata->ic.cmd_timeout_ms ? : MMC_BLK_TIMEOUT_MS,
+					false, MMC_BUSY_IO);
 	}
 
 	return err;