[PATCH] usb-storage: Optimize scan delay more precisely

[PATCH] usb-storage: Optimize scan delay more precisely

[Norihiko Hama]

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");
 
 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 */
-- 
2.17.1

Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Greg KH]

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

RE: [PATCH] usb-storage: Optimize scan delay more precisely

[Norihiko Hama]

> 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

Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Greg KH]

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

RE: [PATCH] usb-storage: Optimize scan delay more precisely

[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?
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.

Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Greg KH]

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

Re: [PATCH] usb-storage: Optimize scan delay more precisely

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

RE: [PATCH] usb-storage: Optimize scan delay more precisely

[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?
> > 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.

RE: [PATCH] usb-storage: Optimize scan delay more precisely

[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.
> > 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?

Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Alan Stern]

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

Re: [usb-storage] Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Matthew Dharm]

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

Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Alan Stern]

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

RE: [PATCH] usb-storage: Optimize scan delay more precisely

[Norihiko Hama]

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

Re: [PATCH] usb-storage: Optimize scan delay more precisely

[Matthew Dharm]

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


-- 
Matthew Dharm
Former Maintainer, USB Mass Storage driver for Linux