| Message ID | 20220617102256.3253019-1-raychi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: hub: add module parameters to usbcore for port init retries | expand |
On Fri, Jun 17, 2022 at 06:22:56PM +0800, Ray Chi wrote: > Currently, there is a Kconfig (CONFIG_USB_FEW_INIT_RETRIES) to > reduce retries when the port initialization is failed. The retry > times are fixed and assigned in compile time. To improve the > flexibility, this patch add four module parameters: > port_reset_tries, set_address_tries, get_descriptor_tries, > and get_maxpacket0_tries, to replace the original default values. > > The default value of module parameters is the same as before > to preserve the existing behavior. > > Signed-off-by: Ray Chi <raychi@google.com> > --- > .../admin-guide/kernel-parameters.txt | 16 ++++++++++ > drivers/usb/core/hub.c | 31 ++++++++++++++++--- > 2 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 8090130b544b..c467b2778128 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -6277,6 +6277,22 @@ > USB_REQ_GET_DESCRIPTOR request in milliseconds > (default 5000 = 5.0 seconds). > > + usbcore.port_reset_tries= > + [USB] Set the retry time of port reset for each > + port initialization (default PORT_RESET_TRIES = 5). > + > + usbcore.set_address_tries= > + [USB] set the retry time of set address for each > + port initialization (default SET_ADDRESS_TRIES = 2). > + > + usbcore.get_descriptor_tries= > + [USB] set the retry time of set address for each > + port initialization (default GET_DESCRIPTOR_TRIES = 2). > + > + usbcore.get_maxpacket0_tries= > + [USB] set the retry time of get maxpacket0 for each > + port initialization (default GET_MAXPACKET0_TRIES = 3). > + > usbcore.nousb [USB] Disable the USB subsystem > > usbcore.quirks= > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index b7f66dcd1fe0..c5c695886424 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2788,6 +2788,27 @@ static unsigned hub_is_wusb(struct usb_hub *hub) > #define HUB_LONG_RESET_TIME 200 > #define HUB_RESET_TIMEOUT 800 > > +/* define retry time for port reset */ > +static int port_reset_tries = PORT_RESET_TRIES; > +module_param(port_reset_tries, int, S_IRUGO|S_IWUSR); > +MODULE_PARM_DESC(port_reset_tries, "retry times of port reset for each port initialization"); Please no. Module parameters are from the 1990's, let us never add new ones if at all possible. These are global options, for all devices in the system. Instead, use per-device settings if you really need to change these values. But I would even push back on that and ask why these values need to be changed at all. What hardware is broken so badly that our timeout settings do not work properly? Can we modify them gracefully to "just work" without any need for tweaking or requiring any modification by a user at all? That would be the better solution instead of requiring users to do this on their own when confronted by misbehaving hardware. thanks, greg k-h
On Tue, Jun 21, 2022 at 3:20 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Jun 17, 2022 at 06:22:56PM +0800, Ray Chi wrote: > > Currently, there is a Kconfig (CONFIG_USB_FEW_INIT_RETRIES) to > > reduce retries when the port initialization is failed. The retry > > times are fixed and assigned in compile time. To improve the > > flexibility, this patch add four module parameters: > > port_reset_tries, set_address_tries, get_descriptor_tries, > > and get_maxpacket0_tries, to replace the original default values. > > > > The default value of module parameters is the same as before > > to preserve the existing behavior. > > > > Signed-off-by: Ray Chi <raychi@google.com> > > --- > > .../admin-guide/kernel-parameters.txt | 16 ++++++++++ > > drivers/usb/core/hub.c | 31 ++++++++++++++++--- > > 2 files changed, 42 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 8090130b544b..c467b2778128 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -6277,6 +6277,22 @@ > > USB_REQ_GET_DESCRIPTOR request in milliseconds > > (default 5000 = 5.0 seconds). > > > > + usbcore.port_reset_tries= > > + [USB] Set the retry time of port reset for each > > + port initialization (default PORT_RESET_TRIES = 5). > > + > > + usbcore.set_address_tries= > > + [USB] set the retry time of set address for each > > + port initialization (default SET_ADDRESS_TRIES = 2). > > + > > + usbcore.get_descriptor_tries= > > + [USB] set the retry time of set address for each > > + port initialization (default GET_DESCRIPTOR_TRIES = 2). > > + > > + usbcore.get_maxpacket0_tries= > > + [USB] set the retry time of get maxpacket0 for each > > + port initialization (default GET_MAXPACKET0_TRIES = 3). > > + > > usbcore.nousb [USB] Disable the USB subsystem > > > > usbcore.quirks= > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index b7f66dcd1fe0..c5c695886424 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -2788,6 +2788,27 @@ static unsigned hub_is_wusb(struct usb_hub *hub) > > #define HUB_LONG_RESET_TIME 200 > > #define HUB_RESET_TIMEOUT 800 > > > > +/* define retry time for port reset */ > > +static int port_reset_tries = PORT_RESET_TRIES; > > +module_param(port_reset_tries, int, S_IRUGO|S_IWUSR); > > +MODULE_PARM_DESC(port_reset_tries, "retry times of port reset for each port initialization"); > > Please no. Module parameters are from the 1990's, let us never add new > ones if at all possible. > > These are global options, for all devices in the system. Instead, use > per-device settings if you really need to change these values. Sorry for the late reply. Since the driver is using define macro to decide the retry time currently, we can't modify the value directly. Do you mean setting by device tree for per-device settings? or other methods? > But I would even push back on that and ask why these values need to be > changed at all. What hardware is broken so badly that our timeout > settings do not work properly? Can we modify them gracefully to "just > work" without any need for tweaking or requiring any modification by a > user at all? That would be the better solution instead of requiring > users to do this on their own when confronted by misbehaving hardware. I got some reports from end users, but I couldn't see the hardware information due to enumeration not being complete. There are too many hardwares owned by end users. It is hard to make work for all of them. In addition, some users just tried to reboot the Host device when they found their connected hardware not working. It would cause the device reset or hang due to the retry mechanism. This is why I want to modify the retry times. > thanks, > > greg k-h Thanks, Ray
On Fri, Jul 01, 2022 at 05:46:42PM +0800, Ray Chi wrote: > On Tue, Jun 21, 2022 at 3:20 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Jun 17, 2022 at 06:22:56PM +0800, Ray Chi wrote: > > > Currently, there is a Kconfig (CONFIG_USB_FEW_INIT_RETRIES) to > > > reduce retries when the port initialization is failed. The retry > > > times are fixed and assigned in compile time. To improve the > > > flexibility, this patch add four module parameters: > > > port_reset_tries, set_address_tries, get_descriptor_tries, > > > and get_maxpacket0_tries, to replace the original default values. > > > > > > The default value of module parameters is the same as before > > > to preserve the existing behavior. > > > > > > Signed-off-by: Ray Chi <raychi@google.com> > > > --- > > > .../admin-guide/kernel-parameters.txt | 16 ++++++++++ > > > drivers/usb/core/hub.c | 31 ++++++++++++++++--- > > > 2 files changed, 42 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > index 8090130b544b..c467b2778128 100644 > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > @@ -6277,6 +6277,22 @@ > > > USB_REQ_GET_DESCRIPTOR request in milliseconds > > > (default 5000 = 5.0 seconds). > > > > > > + usbcore.port_reset_tries= > > > + [USB] Set the retry time of port reset for each > > > + port initialization (default PORT_RESET_TRIES = 5). > > > + > > > + usbcore.set_address_tries= > > > + [USB] set the retry time of set address for each > > > + port initialization (default SET_ADDRESS_TRIES = 2). > > > + > > > + usbcore.get_descriptor_tries= > > > + [USB] set the retry time of set address for each > > > + port initialization (default GET_DESCRIPTOR_TRIES = 2). > > > + > > > + usbcore.get_maxpacket0_tries= > > > + [USB] set the retry time of get maxpacket0 for each > > > + port initialization (default GET_MAXPACKET0_TRIES = 3). > > > + > > > usbcore.nousb [USB] Disable the USB subsystem > > > > > > usbcore.quirks= > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > > index b7f66dcd1fe0..c5c695886424 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -2788,6 +2788,27 @@ static unsigned hub_is_wusb(struct usb_hub *hub) > > > #define HUB_LONG_RESET_TIME 200 > > > #define HUB_RESET_TIMEOUT 800 > > > > > > +/* define retry time for port reset */ > > > +static int port_reset_tries = PORT_RESET_TRIES; > > > +module_param(port_reset_tries, int, S_IRUGO|S_IWUSR); > > > +MODULE_PARM_DESC(port_reset_tries, "retry times of port reset for each port initialization"); > > > > Please no. Module parameters are from the 1990's, let us never add new > > ones if at all possible. > > > > These are global options, for all devices in the system. Instead, use > > per-device settings if you really need to change these values. > > Sorry for the late reply. > Since the driver is using define macro to decide the retry time > currently, we can't > modify the value directly. Do you mean setting by device tree for > per-device settings? or other methods? Yes, anything other than a module parameter as you just modified the value of ALL devices in the system, which I do not think you really want, right? Odds are you just want to be able to work around a broken internal USB hub, and do not want this option changed for anything that a user plugs into the system, right? > > But I would even push back on that and ask why these values need to be > > changed at all. What hardware is broken so badly that our timeout > > settings do not work properly? Can we modify them gracefully to "just > > work" without any need for tweaking or requiring any modification by a > > user at all? That would be the better solution instead of requiring > > users to do this on their own when confronted by misbehaving hardware. > > I got some reports from end users, but I couldn't see the hardware > information due to > enumeration not being complete. There are too many hardwares owned by end users. > It is hard to make work for all of them. In addition, some users just > tried to reboot the Host device > when they found their connected hardware not working. It would cause > the device reset or hang > due to the retry mechanism. This is why I want to modify the retry times. So this is for external devices? Then just change the kernel build option for those systems? In all the 20+ years, we haven't seen a real need for this yet, what just changed to require it? thanks, greg k-h
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 8090130b544b..c467b2778128 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -6277,6 +6277,22 @@ USB_REQ_GET_DESCRIPTOR request in milliseconds (default 5000 = 5.0 seconds). + usbcore.port_reset_tries= + [USB] Set the retry time of port reset for each + port initialization (default PORT_RESET_TRIES = 5). + + usbcore.set_address_tries= + [USB] set the retry time of set address for each + port initialization (default SET_ADDRESS_TRIES = 2). + + usbcore.get_descriptor_tries= + [USB] set the retry time of set address for each + port initialization (default GET_DESCRIPTOR_TRIES = 2). + + usbcore.get_maxpacket0_tries= + [USB] set the retry time of get maxpacket0 for each + port initialization (default GET_MAXPACKET0_TRIES = 3). + usbcore.nousb [USB] Disable the USB subsystem usbcore.quirks= diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index b7f66dcd1fe0..c5c695886424 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2788,6 +2788,27 @@ static unsigned hub_is_wusb(struct usb_hub *hub) #define HUB_LONG_RESET_TIME 200 #define HUB_RESET_TIMEOUT 800 +/* define retry time for port reset */ +static int port_reset_tries = PORT_RESET_TRIES; +module_param(port_reset_tries, int, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(port_reset_tries, "retry times of port reset for each port initialization"); + +/* define retry time of set address */ +static int set_address_tries = SET_ADDRESS_TRIES; +module_param(set_address_tries, int, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(set_address_tries, "retry times of set address for each port initialization"); + +/* define retry time of get descriptor */ +static int get_descriptor_tries = GET_DESCRIPTOR_TRIES; +module_param(get_descriptor_tries, int, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(get_descriptor_tries, "retry times of set address for each port initialization"); + +/* define retry time of get maxpacket0 */ +static int get_maxpacket0_tries = GET_MAXPACKET0_TRIES; +module_param(get_maxpacket0_tries, int, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(get_maxpacket0_tries, + "retry times of get maxpacket0 for each port initialization"); + static bool use_new_scheme(struct usb_device *udev, int retry, struct usb_port *port_dev) { @@ -2965,7 +2986,7 @@ static int hub_port_reset(struct usb_hub *hub, int port1, clear_bit(port1, hub->warm_reset_bits); /* Reset the port */ - for (i = 0; i < PORT_RESET_TRIES; i++) { + for (i = 0; i < port_reset_tries; i++) { status = set_port_feature(hub->hdev, port1, (warm ? USB_PORT_FEAT_BH_PORT_RESET : USB_PORT_FEAT_RESET)); @@ -2989,7 +3010,7 @@ static int hub_port_reset(struct usb_hub *hub, int port1, * reset attempts to avoid warm reset loop. */ if (status == 0 || status == -ENOTCONN || status == -ENODEV || - (status == -EBUSY && i == PORT_RESET_TRIES - 1)) { + (status == -EBUSY && i == port_reset_tries - 1)) { usb_clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_C_RESET); @@ -4788,7 +4809,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, */ do_new_scheme = use_new_scheme(udev, retry_counter, port_dev); - for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) { + for (retries = 0; retries < get_descriptor_tries; (++retries, msleep(100))) { if (do_new_scheme) { struct usb_device_descriptor *buf; int r = 0; @@ -4812,7 +4833,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, * 255 is for WUSB devices, we actually need to use * 512 (WUSB1.0[4.8.1]). */ - for (operations = 0; operations < GET_MAXPACKET0_TRIES; + for (operations = 0; operations < get_maxpacket0_tries; ++operations) { buf->bMaxPacketSize0 = 0; r = usb_control_msg(udev, usb_rcvaddr0pipe(), @@ -4873,7 +4894,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, * authorization will assign the final address. */ if (udev->wusb == 0) { - for (operations = 0; operations < SET_ADDRESS_TRIES; ++operations) { + for (operations = 0; operations < set_address_tries; ++operations) { retval = hub_set_address(udev, devnum); if (retval >= 0) break;
Currently, there is a Kconfig (CONFIG_USB_FEW_INIT_RETRIES) to reduce retries when the port initialization is failed. The retry times are fixed and assigned in compile time. To improve the flexibility, this patch add four module parameters: port_reset_tries, set_address_tries, get_descriptor_tries, and get_maxpacket0_tries, to replace the original default values. The default value of module parameters is the same as before to preserve the existing behavior. Signed-off-by: Ray Chi <raychi@google.com> --- .../admin-guide/kernel-parameters.txt | 16 ++++++++++ drivers/usb/core/hub.c | 31 ++++++++++++++++--- 2 files changed, 42 insertions(+), 5 deletions(-)