Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

["Bjørn Mork" <bjorn@mork.no>]

Alan Stern <stern@rowland.harvard.edu> writes:
> On Wed, 6 Apr 2016, Bjørn Mork wrote:
>> Alan Stern <stern@rowland.harvard.edu> writes:
>> > On Wed, 6 Apr 2016, Mathias Nyman wrote:
>> >
>> >> We don't want to runtime suspend a bus if there is an event pending.
>> >> The roothub on a NEC uPD720200 host with a single USB3 device connected
>> >> might go back to runtime suspend immediately after runtime resume as
>> >> hub might not yet see any port changes in resume.
>> >> 
>> >> Prevent this by checking if there is a unhandled event pending when
>> >> calling runtime suspend.
>> >> 
>> >> Cc: <stable@vger.kernel.org>
>> >> Tested-by: Mike Murdoch <main.haarp@gmail.com>
>> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> >> ---
>> >>  drivers/usb/host/xhci-hub.c | 6 ++++++
>> >>  drivers/usb/host/xhci.c     | 1 +
>> >>  2 files changed, 7 insertions(+)
>> >> 
>> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> >> index d61fcc4..7e8999a 100644
>> >> --- a/drivers/usb/host/xhci-hub.c
>> >> +++ b/drivers/usb/host/xhci-hub.c
>> >> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>> >>  	__le32 __iomem **port_array;
>> >>  	struct xhci_bus_state *bus_state;
>> >>  	unsigned long flags;
>> >> +	u32 status;
>> >>  
>> >>  	max_ports = xhci_get_ports(hcd, &port_array);
>> >>  	bus_state = &xhci->bus_state[hcd_index(hcd)];
>> >>  
>> >>  	spin_lock_irqsave(&xhci->lock, flags);
>> >>  
>> >> +	/* Don't suspend root hub if there's an event pending. */
>> >> +	status = readl(&xhci->op_regs->status);
>> >> +	if (status & STS_EINT)
>> >> +		return -EBUSY;
>> >
>> > Does anybody else see a problem here?
>> 
>> Like the EBUSY being propagated all the way to usb_suspend(), which is
>> called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?
>> 
>> This will not only prevent runtime suspend, but also system suspend,
>> wont it?  Or did I miss something on the way?  The call chain between
>> the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..
>
> I was thinking of returning with a private spinlock held.

No, I didn't see that. So I am happy to support Mathias' case by
demonstrating that it wasn't *that* bloody obvious :)


Bjørn