linux-kernel - RE: [PATCH v7 1/5] usb: phy: add usb phy notify port status API

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 for Android: free password hash cracker in your pocket

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

Message-ID: <8d6ee5765dc34d5fa042195b27aa7eec@realtek.com>
Date:   Mon, 24 Jul 2023 07:33:39 +0000
From:   Stanley Chang[昌育德] 
        <stanley_chang@...ltek.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Roy Luo <royluo@...gle.com>,
        Matthias Kaehlcke <mka@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Flavio Suligoi <f.suligoi@...m.it>,
        Ray Chi <raychi@...gle.com>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: RE: [PATCH v7 1/5] usb: phy: add usb phy notify port status API

Hi Greg,

> > >
> > > How do you know that the disconnect will not have already been
> > > triggered at this point, when the status changes?
> >
> > The status change of connection is before port reset.
> > In this stage, the device is not port enable, and it will not trigger
> disconnection.
> 
> Ok, then say that here please :)

Okay. I will add it.

> > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > > > a739403a9e45..8433ff89dea6 100644
> > > > --- a/drivers/usb/core/hub.c
> > > > +++ b/drivers/usb/core/hub.c
> > > > @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub
> > > > *hub,
> > > int port1, int type,
> > > >               ret = 0;
> > > >       }
> > > >       mutex_unlock(&hub->status_mutex);
> > > > +
> > > > +     if (!ret) {
> > > > +             struct usb_device *hdev = hub->hdev;
> > > > +
> > > > +             if (hdev && !hdev->parent) {
> > >
> > > Why the check for no parent?  Please document that here in a comment.
> >
> > I will add a comment :
> > /* Only notify roothub. That is, when hdev->parent is empty. */
> 
> Also document this that this will only happen for root hub status changes, that's
> not obvious in the callback name or documentation or anywhere else here.

All usb phy notifications (connection, disconnection) are only for roothub.
So I don't special to doc this.

> > > > +                     struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
> > > > +
> > > > +                     if (hcd->usb_phy)
> > > > +
> > > usb_phy_notify_port_status(hcd->usb_phy,
> > > > +
> port1 -
> > > 1, *status, *change);
> > > > +             }
> > > > +     }
> > > > +
> > >
> > > This is safe to notify with the hub mutex unlocked?  Again, a
> > > comment would be helpful to future people explaining why that is so.
> > >
> >
> > I will add a comment:
> > /*
> >  * There is no need to lock status_mutex here, because status_mutex
> >  * protects hub->status, and the phy driver only checks the port
> >  * status without changing the status.
> >  */
> 
> Looks good, if you do it without the trailing whitespace :)
> 
Okay


Thanks,
Stanley