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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > .../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
