[PATCH v1 0/2] Two changes for eMMC

[PATCH v1 0/2] Two changes for eMMC

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Hi Ullf,

Patch 1/2 here is a missing patch since the last commit, I've fixed the
warning: "Assign discards 'const' qualifier from pointer target type".

Patch 2/2 is new, please take a look.

thanks,
Bean Huo

Bean Huo (2):
  mmc: sdhci-omap: Use of_device_get_match_data() helper
  mmc: core: Allows to override the timeout value for ioctl() path

 drivers/mmc/core/block.c      | 8 ++++----
 drivers/mmc/host/sdhci-omap.c | 9 ++-------
 2 files changed, 6 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] mmc: sdhci-omap: Use of_device_get_match_data() helper

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Only the device data is needed, not the entire struct of_device_id.
Use of_device_get_match_data() instead of open coding of_match_device().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/mmc/host/sdhci-omap.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index 64e27c2821f9..86e867ffbb10 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -1219,16 +1219,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_omap_host *omap_host;
 	struct mmc_host *mmc;
-	const struct of_device_id *match;
-	struct sdhci_omap_data *data;
+	const struct sdhci_omap_data *data;
 	const struct soc_device_attribute *soc;
 	struct resource *regs;
 
-	match = of_match_device(omap_sdhci_match, dev);
-	if (!match)
-		return -EINVAL;
-
-	data = (struct sdhci_omap_data *)match->data;
+	data = of_device_get_match_data(&pdev->dev);
 	if (!data) {
 		dev_err(dev, "no sdhci omap data\n");
 		return -EINVAL;
-- 
2.34.1

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

[Bean Huo]

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(-)

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

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

[Avri Altman]

> 
> 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>

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

[Linus Walleij]

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

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

[Bean Huo]

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

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

[Linus Walleij]

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

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

[Ulf Hansson]

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

Re: [PATCH v1 1/2] mmc: sdhci-omap: Use of_device_get_match_data() helper

[Ulf Hansson]

On Sun, 24 Apr 2022 at 00:16, Bean Huo <huobean@gmail.com> wrote:
>
> From: Bean Huo <beanhuo@micron.com>
>
> Only the device data is needed, not the entire struct of_device_id.
> Use of_device_get_match_data() instead of open coding of_match_device().
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-omap.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 64e27c2821f9..86e867ffbb10 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -1219,16 +1219,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>         struct sdhci_pltfm_host *pltfm_host;
>         struct sdhci_omap_host *omap_host;
>         struct mmc_host *mmc;
> -       const struct of_device_id *match;
> -       struct sdhci_omap_data *data;
> +       const struct sdhci_omap_data *data;
>         const struct soc_device_attribute *soc;
>         struct resource *regs;
>
> -       match = of_match_device(omap_sdhci_match, dev);
> -       if (!match)
> -               return -EINVAL;
> -
> -       data = (struct sdhci_omap_data *)match->data;
> +       data = of_device_get_match_data(&pdev->dev);
>         if (!data) {
>                 dev_err(dev, "no sdhci omap data\n");
>                 return -EINVAL;
> --
> 2.34.1
>

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

[Ulf Hansson]

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
>

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

[Linus Walleij]

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