| Message ID | 20240424013134.17307-1-Norihiko.Hama@alpsalpine.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] usb-storage: Optimize scan delay more precisely | expand |
On Wed, Apr 24, 2024 at 10:31:34AM +0900, Norihiko Hama wrote: > Current storage scan delay is reduced by the following old commit. > > a4a47bc03fe5 ("Lower USB storage settling delay to something more reasonable") > > It means that delay is at least 'one second', or zero with delay_use=0. > 'one second' is still long delay especially for embedded system but > when delay_use is set to 0 (no delay), still error observed on some USB drives. > > So delay_use should not be set to 0 but 'one second' is quite long. > Especially for embedded system, it's important for end user > how quickly access to USB drive when it's connected. > That's why we have a chance to minimize such a constant long delay. > > This patch optimizes scan delay more precisely > to minimize delay time but not to have any problems on USB drives > by extending module parameter 'delay_use' in milliseconds internally. > The parameter 'delay_use' is changed to be parsed as 3 decimal point value > if it has digit values with '.'. > It makes the range of value to 1 / 1000 in internal 32-bit value > but it's still enough to set the delay time. > By default, delay time is 'one second' for backward compatibility. > > For example, it seems to be good by changing delay_use=0.1, > that is 100 millisecond delay without issues for most USB pen drives. > > Signed-off-by: Norihiko Hama <Norihiko.Hama@alpsalpine.com> > --- > V3 -> V4: Separate parser functions from module parameter set/get > V2 -> V3: Change to use kstrtouint only for parsing decimal point > V1 -> V2: Extend existing module parameter 'delay_use' to support decimal points This is pretty good. I've got a couple of minor suggestions. > .../admin-guide/kernel-parameters.txt | 5 + > drivers/usb/storage/usb.c | 120 +++++++++++++++++- > 2 files changed, 121 insertions(+), 4 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 561d0dd776c7..8248426524d5 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -6190,6 +6190,11 @@ > usb-storage.delay_use= > [UMS] The delay in seconds before a new device is > scanned for Logical Units (default 1). > + The delay can have up to 3 decimal places, giving a > + resolution of one millisecond. > + Example: > + delay_use=2.567 This could go on the previous line. Example: delay_use = 2.567 > + 2.567 second delay This is obvious; I don't think you need to say it. > > usb-storage.quirks= > [UMS] A list of quirks entries to supplement or > diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c > index 90aa9c12ffac..c908cf2a8027 100644 > --- a/drivers/usb/storage/usb.c > +++ b/drivers/usb/storage/usb.c > @@ -67,9 +67,121 @@ MODULE_AUTHOR("Matthew Dharm <mdharm-usb@one-eyed-alien.net>"); > MODULE_DESCRIPTION("USB Mass Storage driver for Linux"); > MODULE_LICENSE("GPL"); > > -static unsigned int delay_use = 1; > -module_param(delay_use, uint, S_IRUGO | S_IWUSR); > -MODULE_PARM_DESC(delay_use, "seconds to delay before using a new device"); > +static unsigned int delay_use = 1 * MSEC_PER_SEC; > + > +/** > + * str_to_fixed_point_uint - parse an unsigned fixed-point decimal integer > + * @str: String to parse. > + * @ndecimals: Number of decimal places in the fixed-point value. > + * @val: Where to store the parsed value. > + * > + * Parse an unsigned fixed-point decimal value in @str, containing at > + * most ndecimal digits to the right of the decimal point. > + * Stores the parsed value in @val, scaled by 10^(@ndecimal). > + * > + * As with kstrtouint(), the string must be NUL-terminated and may > + * include a single newline before the terminating NUL. The first > + * character may be a plus sign but not a minus sign. The decimal > + * point and fractional digits are optional. > + * > + * Returns 0 on success, a negative error code otherwise. > + */ > +static int str_to_fixed_point_uint(const char *str, int ndecimals, > + unsigned int *val) > +{ > + int n, n1, n2; > + const char *p; > + char *q; > + char buf[16]; > + > + n = strlen(str); > + if (n > 0 && str[n - 1] == '\n') > + --n; > + > + p = strnchr(str, n, '.'); > + if (p) { > + n1 = p++ - str; > + n2 = n - (n1 + 1); > + if (n2 == 0 || n2 > ndecimals) > + return -EINVAL; > + } else { > + n1 = n; > + n2 = 0; > + } > + if (n1 == 0 || n1 + ndecimals > sizeof(buf) - 1) > + return -EINVAL; There's no reason to rule out strings like ".5" or "2.". Allow either n1 or n2 to be 0, as long as they aren't both 0. (And even then, the code would accept "+" or "+.", which is bizarre but not terrible.) > + > + memcpy(buf, str, n1); > + if (p) > + memcpy(buf + n1, p, n2); > + for (q = buf + n1 + n2; n2 < ndecimals; ++n2) > + *q++ = '0'; > + *q = 0; > + > + return kstrtouint(buf, 10, val); > +} > + > +/** > + * fixed_point_uint_to_str - parse into fixed-point decimal integer string > + * @val: Integer value to parse. > + * @ndecimals: Number of decimal places in the fixed-point value. > + * @str: Where to store the parsed string. > + * @size: The size of buffer for @str. > + * > + * Stores the parsed @val scaled by 10^(@ndecimal) into @str. > + * The string trailing fractional part '0' is trimmed. "Parse" is the wrong word here. You parse a string to understand what it means (so the previous function parses @str). But here you're converting an integer to a string; that's called "formatting". So the function's description should be: format a fixed-point decimal value into a string. @val is the integer value to format, scaled by 10^(@ndecimals), and @str is where to store the formatted string. > + * > + * Returns the number of characters written into @str. > + */ > +static int fixed_point_uint_to_str(unsigned int val, int ndecimals, > + char *str, int size) > +{ > + unsigned int delay_ms = val; > + unsigned int rem = do_div(delay_ms, int_pow(10, ndecimals)); > + int len; > + char buf[16]; > + > + len = scnprintf(buf, sizeof(buf), "%d", delay_ms); > + if (rem) { > + char format[8]; > + > + snprintf(format, sizeof(format) - 1, ".%%0%dd", ndecimals); > + len += scnprintf(buf + len, sizeof(buf) - len, format, rem); > + while (buf[len - 1] == '0') { > + buf[len - 1] = '\0'; > + if (--len <= 1) > + break; > + } Small simplification: You know that len can never reach 0 because rem is nonzero, and in any case buf contains a '.' somewhere. So this loop can be written as: while (buf[--len] == '0') buf[len] = 0; Alan Stern > + } > + return scnprintf(str, size, "%s\n", buf); > +} > + > +static int delay_use_set(const char *s, const struct kernel_param *kp) > +{ > + unsigned int delay_ms; > + int ret; > + > + ret = str_to_fixed_point_uint(skip_spaces(s), 3, &delay_ms); > + if (ret < 0) > + return ret; > + > + *((unsigned int *)kp->arg) = delay_ms; > + return 0; > +} > + > +static int delay_use_get(char *s, const struct kernel_param *kp) > +{ > + unsigned int delay_ms = *((unsigned int *)kp->arg); > + > + return fixed_point_uint_to_str(delay_ms, 3, s, PAGE_SIZE); > +} > + > +static const struct kernel_param_ops delay_use_ops = { > + .set = delay_use_set, > + .get = delay_use_get, > +}; > +module_param_cb(delay_use, &delay_use_ops, &delay_use, 0644); > +MODULE_PARM_DESC(delay_use, "time to delay before using a new device"); > > static char quirks[128]; > module_param_string(quirks, quirks, sizeof(quirks), S_IRUGO | S_IWUSR); > @@ -1066,7 +1178,7 @@ int usb_stor_probe2(struct us_data *us) > if (delay_use > 0) > dev_dbg(dev, "waiting for device to settle before scanning\n"); > queue_delayed_work(system_freezable_wq, &us->scan_dwork, > - delay_use * HZ); > + msecs_to_jiffies(delay_use)); > return 0; > > /* We come here if there are any problems */ > -- > 2.17.1 > >
On Wed, Apr 24, 2024 at 10:51:57AM -0400, Alan Stern wrote: > On Wed, Apr 24, 2024 at 10:31:34AM +0900, Norihiko Hama wrote: > > + Example: > > + delay_use=2.567 > > This could go on the previous line. > > Example: delay_use = 2.567 Oops, this shouldn't have the extra spaces around the '=' sign. Example: delay_use=2.567 Alan Stern
> On Wed, Apr 24, 2024 at 10:51:57AM -0400, Alan Stern wrote: > > On Wed, Apr 24, 2024 at 10:31:34AM +0900, Norihiko Hama wrote: > > > + Example: > > > + delay_use=2.567 > > > > This could go on the previous line. > > > > Example: delay_use = 2.567 > > Oops, this shouldn't have the extra spaces around the '=' sign. > > Example: delay_use=2.567 > Thank you for reviewing patch. I'll fix it based on your proposal (also on your previous reply) and submit it as v5.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 561d0dd776c7..8248426524d5 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -6190,6 +6190,11 @@ usb-storage.delay_use= [UMS] The delay in seconds before a new device is scanned for Logical Units (default 1). + The delay can have up to 3 decimal places, giving a + resolution of one millisecond. + Example: + delay_use=2.567 + 2.567 second delay usb-storage.quirks= [UMS] A list of quirks entries to supplement or diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 90aa9c12ffac..c908cf2a8027 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -67,9 +67,121 @@ MODULE_AUTHOR("Matthew Dharm <mdharm-usb@one-eyed-alien.net>"); MODULE_DESCRIPTION("USB Mass Storage driver for Linux"); MODULE_LICENSE("GPL"); -static unsigned int delay_use = 1; -module_param(delay_use, uint, S_IRUGO | S_IWUSR); -MODULE_PARM_DESC(delay_use, "seconds to delay before using a new device"); +static unsigned int delay_use = 1 * MSEC_PER_SEC; + +/** + * str_to_fixed_point_uint - parse an unsigned fixed-point decimal integer + * @str: String to parse. + * @ndecimals: Number of decimal places in the fixed-point value. + * @val: Where to store the parsed value. + * + * Parse an unsigned fixed-point decimal value in @str, containing at + * most ndecimal digits to the right of the decimal point. + * Stores the parsed value in @val, scaled by 10^(@ndecimal). + * + * As with kstrtouint(), the string must be NUL-terminated and may + * include a single newline before the terminating NUL. The first + * character may be a plus sign but not a minus sign. The decimal + * point and fractional digits are optional. + * + * Returns 0 on success, a negative error code otherwise. + */ +static int str_to_fixed_point_uint(const char *str, int ndecimals, + unsigned int *val) +{ + int n, n1, n2; + const char *p; + char *q; + char buf[16]; + + n = strlen(str); + if (n > 0 && str[n - 1] == '\n') + --n; + + p = strnchr(str, n, '.'); + if (p) { + n1 = p++ - str; + n2 = n - (n1 + 1); + if (n2 == 0 || n2 > ndecimals) + return -EINVAL; + } else { + n1 = n; + n2 = 0; + } + if (n1 == 0 || n1 + ndecimals > sizeof(buf) - 1) + return -EINVAL; + + memcpy(buf, str, n1); + if (p) + memcpy(buf + n1, p, n2); + for (q = buf + n1 + n2; n2 < ndecimals; ++n2) + *q++ = '0'; + *q = 0; + + return kstrtouint(buf, 10, val); +} + +/** + * fixed_point_uint_to_str - parse into fixed-point decimal integer string + * @val: Integer value to parse. + * @ndecimals: Number of decimal places in the fixed-point value. + * @str: Where to store the parsed string. + * @size: The size of buffer for @str. + * + * Stores the parsed @val scaled by 10^(@ndecimal) into @str. + * The string trailing fractional part '0' is trimmed. + * + * Returns the number of characters written into @str. + */ +static int fixed_point_uint_to_str(unsigned int val, int ndecimals, + char *str, int size) +{ + unsigned int delay_ms = val; + unsigned int rem = do_div(delay_ms, int_pow(10, ndecimals)); + int len; + char buf[16]; + + len = scnprintf(buf, sizeof(buf), "%d", delay_ms); + if (rem) { + char format[8]; + + snprintf(format, sizeof(format) - 1, ".%%0%dd", ndecimals); + len += scnprintf(buf + len, sizeof(buf) - len, format, rem); + while (buf[len - 1] == '0') { + buf[len - 1] = '\0'; + if (--len <= 1) + break; + } + } + return scnprintf(str, size, "%s\n", buf); +} + +static int delay_use_set(const char *s, const struct kernel_param *kp) +{ + unsigned int delay_ms; + int ret; + + ret = str_to_fixed_point_uint(skip_spaces(s), 3, &delay_ms); + if (ret < 0) + return ret; + + *((unsigned int *)kp->arg) = delay_ms; + return 0; +} + +static int delay_use_get(char *s, const struct kernel_param *kp) +{ + unsigned int delay_ms = *((unsigned int *)kp->arg); + + return fixed_point_uint_to_str(delay_ms, 3, s, PAGE_SIZE); +} + +static const struct kernel_param_ops delay_use_ops = { + .set = delay_use_set, + .get = delay_use_get, +}; +module_param_cb(delay_use, &delay_use_ops, &delay_use, 0644); +MODULE_PARM_DESC(delay_use, "time to delay before using a new device"); static char quirks[128]; module_param_string(quirks, quirks, sizeof(quirks), S_IRUGO | S_IWUSR); @@ -1066,7 +1178,7 @@ int usb_stor_probe2(struct us_data *us) if (delay_use > 0) dev_dbg(dev, "waiting for device to settle before scanning\n"); queue_delayed_work(system_freezable_wq, &us->scan_dwork, - delay_use * HZ); + msecs_to_jiffies(delay_use)); return 0; /* We come here if there are any problems */
Current storage scan delay is reduced by the following old commit. a4a47bc03fe5 ("Lower USB storage settling delay to something more reasonable") It means that delay is at least 'one second', or zero with delay_use=0. 'one second' is still long delay especially for embedded system but when delay_use is set to 0 (no delay), still error observed on some USB drives. So delay_use should not be set to 0 but 'one second' is quite long. Especially for embedded system, it's important for end user how quickly access to USB drive when it's connected. That's why we have a chance to minimize such a constant long delay. This patch optimizes scan delay more precisely to minimize delay time but not to have any problems on USB drives by extending module parameter 'delay_use' in milliseconds internally. The parameter 'delay_use' is changed to be parsed as 3 decimal point value if it has digit values with '.'. It makes the range of value to 1 / 1000 in internal 32-bit value but it's still enough to set the delay time. By default, delay time is 'one second' for backward compatibility. For example, it seems to be good by changing delay_use=0.1, that is 100 millisecond delay without issues for most USB pen drives. Signed-off-by: Norihiko Hama <Norihiko.Hama@alpsalpine.com> --- V3 -> V4: Separate parser functions from module parameter set/get V2 -> V3: Change to use kstrtouint only for parsing decimal point V1 -> V2: Extend existing module parameter 'delay_use' to support decimal points .../admin-guide/kernel-parameters.txt | 5 + drivers/usb/storage/usb.c | 120 +++++++++++++++++- 2 files changed, 121 insertions(+), 4 deletions(-)