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

From: Bean Huo <huobean@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: ulf.hansson@linaro.org, adrian.hunter@intel.com,
	linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
	beanhuo@micron.com, stable <stable@vger.kernel.org>
Subject: Re: [PATCH v1 2/2] mmc: core: Allows to override the timeout value for ioctl() path
Date: Mon, 25 Apr 2022 22:01:59 +0200	[thread overview]
Message-ID: <89845bec6c827d7012cda916ee50b16c8eb08755.camel@gmail.com> (raw)
In-Reply-To: <CACRpkdahf5QhUJ-vG6Qs7i0VYbSS02zBrQOtN8EVFu9SyHZA1Q@mail.gmail.com>

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