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;
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
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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Avri Altman -
Bean Huo -
Linus Walleij -
Ulf Hansson