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

[Stanley Chang[昌育德]]

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