diff mbox series

USB: hub: add module parameters to usbcore for port init retries

Message ID 20220617102256.3253019-1-raychi@google.com
State New
Headers show
Series USB: hub: add module parameters to usbcore for port init retries | expand

Commit Message

Ray Chi June 17, 2022, 10:22 a.m. UTC
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(-)

Comments

Ray Chi July 1, 2022, 9:46 a.m. UTC | #1
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
diff mbox series

Patch

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;