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