Hi Shuah, On Mon, May 06, 2019 at 09:13:02AM -0600, shuah wrote: > On 5/6/19 6:55 AM, Suwan Kim wrote: > > When hcd suspends execution, hcd_bus_suspend() calls vhci_bus_suspend() > > which sets hcd->state as HC_STATE_SUSPENDED. But after calling > > vhci_bus_suspend(), hcd_bus_suspend() also sets hcd->state as > > HC_STATE_SUSPENDED. > > So, setting hcd->state in vhci_hcd_suspend() is unnecessary. > > > > Signed-off-by: Suwan Kim <suwan.kim027@xxxxxxxxx> > > --- > > drivers/usb/usbip/vhci_hcd.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > > index 667d9c0ec905..e6f378d00fb6 100644 > > --- a/drivers/usb/usbip/vhci_hcd.c > > +++ b/drivers/usb/usbip/vhci_hcd.c > > @@ -1238,10 +1238,6 @@ static int vhci_bus_suspend(struct usb_hcd *hcd) > > dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__); > > - spin_lock_irqsave(&vhci->lock, flags); > > - hcd->state = HC_STATE_SUSPENDED; > > - spin_unlock_irqrestore(&vhci->lock, flags); > > - > > return 0; > > } > > > > Tell me more about why you think this change is needed? How did you test > this change? I think that host controller specific functions, vhci_bus_resume() or vhci_bus_suspend() in the case of vhci, usually process host controller specific data (struct vhci_hcd) not a generic data (struct usb_hcd). The generic data is usually processed by generic HCD layer. But vhci_bus_resume() and vhci_bus_suspend() set generic data (hcd->state) and moreover this variable is set in generic HCD layer once again(hcd_bus_resume() and hcd_bus_suspend()). So, i think host controller specific functions (vhci_bus_resume() and vhci_bus_suspend()) don't need to set the generic data that is "hcd->state = HC_STATE_RUNNING or HC_STATE_SUSPEND". For test, I loaded vhci-hcd module, suspended and resumed my computer and checked hcd->state variable. There is no difference compared with not modified version because my patch just removes repeated and unnecessary part. Regards Suwan Kim
