Re: [PATCH] USB: hub: add module parameters to usbcore for port init retries

[Greg KH <gregkh@linuxfoundation.org>]

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