On 5/7/19 9:49 AM, Suwan Kim wrote:
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".
It depends. In this case, vhci_hcd is virtual driver and maintains
port status for devices that are remote. It checks hcd->state in
vhci_hub_status().
It updates the hcd->state in suspend with vhci->lock hold and checks
status from vhci_hub_status(). Removing updating hcd->state from
vhci_bus_suspend()will open a window where vhci_hub_status() might
find it wrong state.
Same thing for HC_STATE_RUNNING. There are few other drivers that
do the same for similar reasons. xhci_hcd, and dummy_hcd and a few
more. This is the same comment on your patch that removes
HC_STATE_RUNNING update from resume.
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.
This won't fully test the condition since there are remote devices
attached and triggering suspend while attach/detach is in progress.
thanks,
-- Shuah
