| Message ID | 20240327055130.43206-1-Norihiko.Hama@alpsalpine.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb-storage: Optimize scan delay more precisely | expand |
On Wed, Mar 27, 2024 at 02:51:30PM +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), error still observed on some USB drives. > > So delay_use should not be set to 0 but 'one second' is quite long. > > This patch optimizes scan delay more precisely > to minimize delay time but not to have any problems on USB drives > by adding module parameter 'delay_scale' of delay-time divisor. > By default, delay time is 'one second' for backward compatibility. > For example, it seems to be good by changing delay_scale=100, > that is 100 millisecond delay. > > Signed-off-by: Norihiko Hama <Norihiko.Hama@alpsalpine.com> > --- > drivers/usb/storage/usb.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c > index 90aa9c12ffac..f4a755e364da 100644 > --- a/drivers/usb/storage/usb.c > +++ b/drivers/usb/storage/usb.c > @@ -70,6 +70,9 @@ 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_scale = MSEC_PER_SEC; > +module_param(delay_scale, uint, 0644); > +MODULE_PARM_DESC(delay_scale, "time scale of delay_use"); Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. thanks, greg k-h
> Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. 1 second does not meet with performance requirement. I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. Do you have any other better solution? Best regards, Norihiko Hama
On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > 1 second does not meet with performance requirement. Who is requiring such a performance requirement? The USB specification? Or something else? > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > Do you have any other better solution? How long do you exactly need to wait? Why not figure out how long the device takes and if it fails, slowly back off until the full time delay happens and then you can abort? thanks, greg k-h
On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > 1 second does not meet with performance requirement. > > Who is requiring such a performance requirement? The USB specification? > Or something else? This is our customer requirement. > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > Do you have any other better solution? > How long do you exactly need to wait? Why not figure out how long the device takes and if it fails, slowly back off until the full time delay happens and then you can abort? It's IOP issue and difficult to figure out because it depends on device itself. I know we have multiple devices with delay_use=0, but then it's recovered and detected by reset after 30s timeout, that is too long than 1 sec.
On Wed, Mar 27, 2024 at 07:57:52AM +0000, Norihiko Hama wrote: > On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > > 1 second does not meet with performance requirement. > > > > Who is requiring such a performance requirement? The USB specification? > > Or something else? > This is our customer requirement. If it is impossible to do, why are they making you do it? :) > > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > > Do you have any other better solution? > > How long do you exactly need to wait? Why not figure out how long the device takes and if it fails, slowly back off until the full time delay happens and then you can abort? > It's IOP issue and difficult to figure out because it depends on device itself. Agreed. > I know we have multiple devices with delay_use=0, but then it's recovered and detected by reset after 30s timeout, that is too long than 1 sec. So how do you know that making this smaller will help? There are many many odd and broken devices out there that take a long time to spin up before they are able to be accessed. That's what that option is there for, if you "know" you don't need to wait, you don't have to wait. Otherwise you HAVE to wait as you do not know how long things take. sorry, greg k-h
On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > 1 second does not meet with performance requirement. > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > Do you have any other better solution? Can you accomplish what you want with a quirk flag? Alan Stern
> > On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > > > 1 second does not meet with performance requirement. > > > > > > Who is requiring such a performance requirement? The USB specification? > > > Or something else? > > This is our customer requirement. > > If it is impossible to do, why are they making you do it? :) It's possible to do because we've changed code to minimize delay by ourselves, Then no issue observed when we configured delay to 100 msec as far as we have tested. The background for the requirement, it's important for end user how quickly access to USB drive when it's connected. Of course there are a lot of overhead to do that but that's why we would like to have a chance to minimize such a constant long delay. > > I know we have multiple devices with delay_use=0, but then it's recovered and detected by reset after 30s timeout, that is too long than 1 sec. > > So how do you know that making this smaller will help? There are many many odd and broken devices out there that take a long time to spin up before they are able to be > accessed. That's what that option is there for, if you "know" you don't need to wait, you don't have to wait. > Otherwise you HAVE to wait as you do not know how long things take. As previous my comment, we've changed code to minimize delay by ourselves, Then no issue observed when we configured delay to 100 msec as far as we have tested. Sorry, we have no theoretical proof but I think it's same situation with current 1 sec delay. So we want to have a chance to change such a constant delay.
> On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > 1 second does not meet with performance requirement. > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > Do you have any other better solution? > > Can you accomplish what you want with a quirk flag? I think that it's hard to do that because 'quirk' is specified for a device but it's difficult to identify the devices to make quirk, especially for future introduced devices. Can we change the design of existing 'delay_use' ? For example, 'delay_use' is 32-bit value and the value "1000 secs" does not make sense to set it, So if it's set to '1100', it's treated as "100 / 1000 = 0.1 sec". Is this possible?
On Thu, Mar 28, 2024 at 03:04:47AM +0000, Norihiko Hama wrote: > > On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > > 1 second does not meet with performance requirement. > > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > > Do you have any other better solution? > > > > Can you accomplish what you want with a quirk flag? > > I think that it's hard to do that because 'quirk' is specified for a device > but it's difficult to identify the devices to make quirk, especially for future introduced devices. > > Can we change the design of existing 'delay_use' ? > For example, 'delay_use' is 32-bit value and the value "1000 secs" does not make sense to set it, > So if it's set to '1100', it's treated as "100 / 1000 = 0.1 sec". Is this possible? Here's an approach that Greg might accept. Since we already have a delay_use module parameter, we could add a delay_use_ms parameter. The two module parameters would display the same value, but delay_use_ms would be in milliseconds instead of in seconds. (This is similar to what we did for the autosuspend and autosuspend_delay_ms sysfs attributes.) To make it work, you would have to add the new module parameter and store its value in milliseconds. You would also have to change the existing module_param() definition for delay_use to module_param_cb() so that the get/set routines could divide/multiply the delay_use_ms/user-specified value by 1000 to convert to/from seconds. When you write the patch, don't forget to update Documentation/admin-guide/kernel-parameters.txt. Alan Stern
On Thu, Mar 28, 2024 at 7:51 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, Mar 28, 2024 at 03:04:47AM +0000, Norihiko Hama wrote: > > > On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > > > 1 second does not meet with performance requirement. > > > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > > > Do you have any other better solution? > > > > > > Can you accomplish what you want with a quirk flag? > > > > I think that it's hard to do that because 'quirk' is specified for a device > > but it's difficult to identify the devices to make quirk, especially for future introduced devices. > > > > Can we change the design of existing 'delay_use' ? > > For example, 'delay_use' is 32-bit value and the value "1000 secs" does not make sense to set it, > > So if it's set to '1100', it's treated as "100 / 1000 = 0.1 sec". Is this possible? > > Here's an approach that Greg might accept. > > Since we already have a delay_use module parameter, we could add a > delay_use_ms parameter. The two module parameters would display the > same value, but delay_use_ms would be in milliseconds instead of in > seconds. (This is similar to what we did for the autosuspend and > autosuspend_delay_ms sysfs attributes.) What about just changing the parser on the currently delay_use parameter to accept an optional suffix? If it's just digits, it is in seconds. If it ends in "ms", then interpret it as milliseconds. This would be backwards compatible with existing uses, give you the flexibility you want, avoid adding another modules parameter, and potentially be expandable in the future (if, for some reason, someone wanted microseconds or kiloseconds). Matt
On Thu, Mar 28, 2024 at 08:21:18AM -0700, Matthew Dharm wrote: > On Thu, Mar 28, 2024 at 7:51 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, Mar 28, 2024 at 03:04:47AM +0000, Norihiko Hama wrote: > > > > On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > > > > 1 second does not meet with performance requirement. > > > > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > > > > Do you have any other better solution? > > > > > > > > Can you accomplish what you want with a quirk flag? > > > > > > I think that it's hard to do that because 'quirk' is specified for a device > > > but it's difficult to identify the devices to make quirk, especially for future introduced devices. > > > > > > Can we change the design of existing 'delay_use' ? > > > For example, 'delay_use' is 32-bit value and the value "1000 secs" does not make sense to set it, > > > So if it's set to '1100', it's treated as "100 / 1000 = 0.1 sec". Is this possible? > > > > Here's an approach that Greg might accept. > > > > Since we already have a delay_use module parameter, we could add a > > delay_use_ms parameter. The two module parameters would display the > > same value, but delay_use_ms would be in milliseconds instead of in > > seconds. (This is similar to what we did for the autosuspend and > > autosuspend_delay_ms sysfs attributes.) > > What about just changing the parser on the currently delay_use > parameter to accept an optional suffix? If it's just digits, it is in > seconds. If it ends in "ms", then interpret it as milliseconds. This > would be backwards compatible with existing uses, give you the > flexibility you want, avoid adding another modules parameter, and > potentially be expandable in the future (if, for some reason, someone > wanted microseconds or kiloseconds). A little unconventional, I think (at least, I don't know offhand of any other module parameters or sysfs attributes that work this way), but it would work. Noriko, would you like to write a patch to do this? Alan Stern
> > > Here's an approach that Greg might accept. > > > > > > Since we already have a delay_use module parameter, we could add a > > > delay_use_ms parameter. The two module parameters would display the > > > same value, but delay_use_ms would be in milliseconds instead of in > > > seconds. (This is similar to what we did for the autosuspend and > > > autosuspend_delay_ms sysfs attributes.) > > > > What about just changing the parser on the currently delay_use > > parameter to accept an optional suffix? If it's just digits, it is in > > seconds. If it ends in "ms", then interpret it as milliseconds. This > > would be backwards compatible with existing uses, give you the > > flexibility you want, avoid adding another modules parameter, and > > potentially be expandable in the future (if, for some reason, someone > > wanted microseconds or kiloseconds). > > A little unconventional, I think (at least, I don't know offhand of any other module parameters or sysfs attributes that work this way), but it would work. > > Noriko, would you like to write a patch to do this? Thank you for your advice. I understand and will try to do that. Best regards, Norihiko Hama
On Thu, Mar 28, 2024 at 9:18 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, Mar 28, 2024 at 08:21:18AM -0700, Matthew Dharm wrote: > > On Thu, Mar 28, 2024 at 7:51 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Thu, Mar 28, 2024 at 03:04:47AM +0000, Norihiko Hama wrote: > > > > > On Wed, Mar 27, 2024 at 07:39:55AM +0000, Norihiko Hama wrote: > > > > > > > Sorry, but module parameters are from the 1990's, we will not go back to that if at all possible as it's not easy to maintain and will not work properly for multiple devices. > > > > > > > > > > > > > > I can understand wanting something between 1 and 0 seconds, but adding yet-another-option isn't probably the best way, sorry. > > > > > > 1 second does not meet with performance requirement. > > > > > > I have no good idea except module parameter so that we can maintain backward compatibility but be configurable out of module. > > > > > > Do you have any other better solution? > > > > > > > > > > Can you accomplish what you want with a quirk flag? > > > > > > > > I think that it's hard to do that because 'quirk' is specified for a device > > > > but it's difficult to identify the devices to make quirk, especially for future introduced devices. > > > > > > > > Can we change the design of existing 'delay_use' ? > > > > For example, 'delay_use' is 32-bit value and the value "1000 secs" does not make sense to set it, > > > > So if it's set to '1100', it's treated as "100 / 1000 = 0.1 sec". Is this possible? > > > > > > Here's an approach that Greg might accept. > > > > > > Since we already have a delay_use module parameter, we could add a > > > delay_use_ms parameter. The two module parameters would display the > > > same value, but delay_use_ms would be in milliseconds instead of in > > > seconds. (This is similar to what we did for the autosuspend and > > > autosuspend_delay_ms sysfs attributes.) > > > > What about just changing the parser on the currently delay_use > > parameter to accept an optional suffix? If it's just digits, it is in > > seconds. If it ends in "ms", then interpret it as milliseconds. This > > would be backwards compatible with existing uses, give you the > > flexibility you want, avoid adding another modules parameter, and > > potentially be expandable in the future (if, for some reason, someone > > wanted microseconds or kiloseconds). > > A little unconventional, I think (at least, I don't know offhand of any > other module parameters or sysfs attributes that work this way), but it > would work. Actually, I got the idea from the existing parameters such as "mem=" and similar, which accept K, M, or G as suffixes to denote the units for the number. Credit where credit is due. Matt
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 90aa9c12ffac..f4a755e364da 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -70,6 +70,9 @@ 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_scale = MSEC_PER_SEC; +module_param(delay_scale, uint, 0644); +MODULE_PARM_DESC(delay_scale, "time scale of delay_use"); static char quirks[128]; module_param_string(quirks, quirks, sizeof(quirks), S_IRUGO | S_IWUSR); @@ -1066,7 +1069,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 * delay_scale)); 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), error still observed on some USB drives. So delay_use should not be set to 0 but 'one second' is quite long. This patch optimizes scan delay more precisely to minimize delay time but not to have any problems on USB drives by adding module parameter 'delay_scale' of delay-time divisor. By default, delay time is 'one second' for backward compatibility. For example, it seems to be good by changing delay_scale=100, that is 100 millisecond delay. Signed-off-by: Norihiko Hama <Norihiko.Hama@alpsalpine.com> --- drivers/usb/storage/usb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)