linux-kernel - Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Aug 2022 15:55:25 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alexander Stein <alexander.stein@...tq-group.com>,
        Linux USB List <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

Hi,

On Fri, Aug 5, 2022 at 12:56 PM Matthias Kaehlcke <mka@...omium.org> wrote:
>
> Hi Doug,
>
> On Fri, Aug 05, 2022 at 12:26:35PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@...omium.org> wrote:
> > >
> > > onboard_hub_power_off() currently has a delay after asserting the
> > > reset of the hub. There is already a delay in onboard_hub_power_on()
> > > before de-asserting the reset, which ensures that the reset is
> > > asserted for the required time, so the delay in _power_off() is not
> > > needed.
> > >
> > > Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
> > > the function returns early when the GPIO descriptor is NULL.
> > >
> > > Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> > > ---
> > >
> > >  drivers/usb/misc/onboard_usb_hub.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > I was trying to figure out what this "reset" was defined to do and I
> > looked for the device tree bindings. They don't seem to exist. Was
> > that an oversight?
>
> It's not in the binding of the RTS5411 which I guess you looked at,
> because that hub doesn't have a reset line.
>
> The reset functionality was initially added for the TI USB8041, the
> binding has the reset, but I found it hasn't landed yet:
>
> https://patchwork.kernel.org/project/linux-usb/patch/20220727093801.687361-1-alexander.stein@ew.tq-group.com/

Ah, I see. I actually clicked on the "Link" for the patches that
landed and found that they landed without a bindings patch. I guess it
dropped on v4, which I see you yelled about in v3. ;-)


> > In any case, I'm not convinced that your patch is correct. Timing
> > diagrams often show a needed delay between adjusting a reset GPIO and
> > turning on/off the power. The timing diagrams can sometimes show a
> > required delay on both sides. I guess at the moment the only user of
> > this reset GPIO has a symmetric delay, but I can totally expect that
> > someone could come along and say that they needed 10 ms on one side
> > and 1 ms on the other side...
>
> As of now none of the supported hubs (there are only two of them) has
> an asymmetric delay. The RTS5411 doesn't have a reset line, and the
> TI USB8041 only specifies a power on delay (in my interpretation).
>
> [1] has some discussion between Alexander and me about this second
> reset. The patch that added the delay was merged before this
> discussion concluded.
>
> If the driver is going to support a hub that needs an additional
> reset delay when the hub is powered off I'm totally in favor of
> adding that delay, however that isn't currently the case in my
> understanding. If you draw a different conclusion from the TI
> USB8041 datasheet please let me know.
>
> [1] https://patchwork.kernel.org/project/linux-usb/patch/20220727141117.909361-1-alexander.stein@ew.tq-group.com/

Ah, OK. With this context:

Reviewed-by: Douglas Anderson <dianders@...omium.org>