On Mon, Feb 26, 2024 at 02:01:12PM +0800, quic_zijuhu wrote:
> On 2/23/2024 7:33 PM, Greg KH wrote:
> > On Fri, Feb 23, 2024 at 06:00:16PM +0800, Zijun Hu wrote:
> >> USB driver defines macro @USB_CTRL_SET_TIMEOUT for sending control message
> >> and @USB_CTRL_GET_TIMEOUT for receiving, but sierra_get_swoc_info() wrongly
> >> uses @USB_CTRL_SET_TIMEOUT as argument of usb_control_msg() to receive
> >> control message, fixed by using @USB_CTRL_GET_TIMEOUT to receive message.
> >
> > You do realize they are both the same value, right? Why don't we just
> > change it to USB_CTRL_TIMEOUT so that people don't think changing this
> > matters?
> >
> 1)
> will optimize this change title if this change is worthy after code review
It's wrong as-is so we can't take it anyway, so yes, please fix.
> 2)
> yes, i noticed both macros have the same value and carefully read below code block.
>
> include/linux/usb.h:
> /*
> * 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
And so your changelog comments are wrong.
> 3)
> these two macros are introduced at the same time by Linus Torvalds with commit 1da177e4c3f4 ("Linux-2.6.12-rc2")
> below is my points why it is better to keep current two macros than unifying both to one USB_CTRL_TIMEOUT
>
> point A)
> we can't confirm that sending always have the same timeout as receiving for various devices, it is easy to adjust individual
> macro value if sending potentially does not have the same value as receiving in future.
As it has been a few decades without needing this change, I think it's
safe to make now.
> point B)
> current two macros defined by usb.h has been used by many usb drivers, there are more drivers need to be corrected if macro NAME are changed.
That is fine, there is no issue with changing all uses in the kernel
tree, right?
thanks,
greg k-h
