usb_stor_control_msg() timeout argument

[Mark Knibbs]

Hi,

In include/linux/usb.h, there is
/*
* timeouts, in milliseconds, used for sending/receiving control messages
* they typically complete within a few frames (msec) after they're issued
* USB identifies 5 second timeouts, maybe more in a few cases, and a few
* slow devices (like some MGE Ellipse UPSes) actually push that limit.
*/
#define USB_CTRL_GET_TIMEOUT 5000
#define USB_CTRL_SET_TIMEOUT 5000

However several callers of usb_stor_control_msg() have timeout argument
specified as n*HZ, for example in drivers/usb/storage/transport.c:

result = usb_stor_control_msg(us, us->send_ctrl_pipe,
USB_REQ_CLEAR_FEATURE, USB_RECIP_ENDPOINT,
USB_ENDPOINT_HALT, endp,
NULL, 0, 3*HZ);

result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
US_BULK_GET_MAX_LUN,
USB_DIR_IN | USB_TYPE_CLASS |
USB_RECIP_INTERFACE,
0, us->ifnum, us->iobuf, 1, 10*HZ);

result = usb_stor_control_msg(us, us->send_ctrl_pipe,
request, requesttype, value, index, data, size,
5*HZ);

I'm just wondering whether those callers should have timeouts in ms (so
3*HZ -> 3000), or whether the definitions of USB_CTRL_GET/SET_TIMEOUT
should be 5*HZ? Or is HZ always 1000 these days?


Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Mark Knibbs
Hi,
In include/linux/usb.h, there is
/*
* timeouts, in milliseconds, used for sending/receiving control messages
* they typically complete within a few frames (msec) after they're issued
* USB identifies 5 second timeouts, maybe more in a few cases, and a few
* slow devices (like some MGE Ellipse UPSes) actually push that limit.
*/
#define USB_CTRL_GET_TIMEOUT 5000
#define USB_CTRL_SET_TIMEOUT 5000

Those timeout values are used with usb_control_msg(), which takes
timeout values in milliseconds.

Post by Mark Knibbs
However several callers of usb_stor_control_msg() have timeout argument
result = usb_stor_control_msg(us, us->send_ctrl_pipe,
USB_REQ_CLEAR_FEATURE, USB_RECIP_ENDPOINT,
USB_ENDPOINT_HALT, endp,
NULL, 0, 3*HZ);
result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
US_BULK_GET_MAX_LUN,
USB_DIR_IN | USB_TYPE_CLASS |
USB_RECIP_INTERFACE,
0, us->ifnum, us->iobuf, 1, 10*HZ);
result = usb_stor_control_msg(us, us->send_ctrl_pipe,
request, requesttype, value, index, data, size,
5*HZ);

This is because usb_stor_control_msg accepts timeout values in jiffies.

Post by Mark Knibbs
I'm just wondering whether those callers should have timeouts in ms (so
3*HZ -> 3000), or whether the definitions of USB_CTRL_GET/SET_TIMEOUT
should be 5*HZ? Or is HZ always 1000 these days?

No, no, and no.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Mark Knibbs]

On Fri, 17 Oct 2014 12:53:45 -0400 (EDT)

Post by Alan Stern

Post by Mark Knibbs
In include/linux/usb.h, there is
/*
* timeouts, in milliseconds, used for sending/receiving control messages
* they typically complete within a few frames (msec) after they're issued
* USB identifies 5 second timeouts, maybe more in a few cases, and a few
* slow devices (like some MGE Ellipse UPSes) actually push that limit.
*/
#define USB_CTRL_GET_TIMEOUT 5000
#define USB_CTRL_SET_TIMEOUT 5000

Those timeout values are used with usb_control_msg(), which takes
timeout values in milliseconds.

Post by Mark Knibbs
However several callers of usb_stor_control_msg() have timeout argument
...

This is because usb_stor_control_msg accepts timeout values in jiffies.

Post by Mark Knibbs
I'm just wondering whether those callers should have timeouts in ms (so
3*HZ -> 3000), or whether the definitions of USB_CTRL_GET/SET_TIMEOUT
should be 5*HZ? Or is HZ always 1000 these days?

No, no, and no.

Ah, thanks. Given that, it seems my recent patch
"[PATCH] storage: Replace magic number with define in usb_stor_euscsi_init()"
which replaced 5000 with USB_CTRL_SET_TIMEOUT wasn't correct, though that
patch wouldn't change the compiled code.

It looks like the 5000 timeout in usb_stor_euscsi_init() should be replaced
by 5*HZ then?


Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Mark Knibbs
Ah, thanks. Given that, it seems my recent patch
"[PATCH] storage: Replace magic number with define in usb_stor_euscsi_init()"
which replaced 5000 with USB_CTRL_SET_TIMEOUT wasn't correct, though that
patch wouldn't change the compiled code.
It looks like the 5000 timeout in usb_stor_euscsi_init() should be replaced
by 5*HZ then?

Yes, it does look wrong. There may be other places that also call the
function with a timeout value in ms rather than jiffies...

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Mark Knibbs]

On Fri, 17 Oct 2014 15:22:02 -0400 (EDT)

Post by Alan Stern

Post by Mark Knibbs
...
It looks like the 5000 timeout in usb_stor_euscsi_init() should be replaced
by 5*HZ then?

Yes, it does look wrong. There may be other places that also call the
function with a timeout value in ms rather than jiffies...

The only other function that I could see with that problem is
usb_stor_huawei_e220_init(), also in drivers/usb/storage/initializers.c

It uses 1000 for timeout, which presumably should be HZ (or 1*HZ to make
things clearer?).

Any idea why the two similar functions have different ways of specifying
the timeout?


Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Mark Knibbs
On Fri, 17 Oct 2014 15:22:02 -0400 (EDT)

Post by Alan Stern

Post by Mark Knibbs
...
It looks like the 5000 timeout in usb_stor_euscsi_init() should be replaced
by 5*HZ then?

Yes, it does look wrong. There may be other places that also call the
function with a timeout value in ms rather than jiffies...

The only other function that I could see with that problem is
usb_stor_huawei_e220_init(), also in drivers/usb/storage/initializers.c
It uses 1000 for timeout, which presumably should be HZ (or 1*HZ to make
things clearer?).
Any idea why the two similar functions have different ways of specifying
the timeout?

They were the same originally, and then one of them was changed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html