Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220617102256.3253019-1-raychi@google.com> <YrFxLYibDtyuxSO6@kroah.com>
In-Reply-To: <YrFxLYibDtyuxSO6@kroah.com>
From:   Ray Chi <raychi@google.com>
Date:   Fri, 1 Jul 2022 17:46:42 +0800
Message-ID: <CAPBYUsBbP7ssGXSRyWN46u1-Qaa712QLm748FhJ-M3pANZUsng@mail.gmail.com>
Subject: Re: [PATCH] USB: hub: add module parameters to usbcore for port init retries
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     corbet@lwn.net, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        Albert Wang <albertccwang@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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