* [PATCH 2/4] xhci: turn off port power in shutdown [not found] <20220623111945.1557702-1-mathias.nyman@linux.intel.com> @ 2022-06-23 11:19 ` Mathias Nyman 2022-07-19 13:42 ` Joey Corleone [not found] ` <20220623111945.1557702-2-mathias.nyman@linux.intel.com> 1 sibling, 1 reply; 5+ messages in thread From: Mathias Nyman @ 2022-06-23 11:19 UTC (permalink / raw) To: gregkh; +Cc: linux-usb, Mathias Nyman, stable If ports are not turned off in shutdown then runtime suspended self-powered USB devices may survive in U3 link state over S5. During subsequent boot, if firmware sends an IPC command to program the port in DISCONNECT state, it will time out, causing significant delay in the boot time. Turning off roothub port power is also recommended in xhci specification 4.19.4 "Port Power" in the additional note. Cc: stable@vger.kernel.org Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> --- drivers/usb/host/xhci-hub.c | 2 +- drivers/usb/host/xhci.c | 15 +++++++++++++-- drivers/usb/host/xhci.h | 2 ++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index c54f2bc23d3f..0fdc014c9401 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -652,7 +652,7 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd) * It will release and re-aquire the lock while calling ACPI * method. */ -static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, +void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, u16 index, bool on, unsigned long *flags) __must_hold(&xhci->lock) { diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index cb99bed5f755..65858f607437 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -791,6 +791,8 @@ static void xhci_stop(struct usb_hcd *hcd) void xhci_shutdown(struct usb_hcd *hcd) { struct xhci_hcd *xhci = hcd_to_xhci(hcd); + unsigned long flags; + int i; if (xhci->quirks & XHCI_SPURIOUS_REBOOT) usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); @@ -806,12 +808,21 @@ void xhci_shutdown(struct usb_hcd *hcd) del_timer_sync(&xhci->shared_hcd->rh_timer); } - spin_lock_irq(&xhci->lock); + spin_lock_irqsave(&xhci->lock, flags); xhci_halt(xhci); + + /* Power off USB2 ports*/ + for (i = 0; i < xhci->usb2_rhub.num_ports; i++) + xhci_set_port_power(xhci, xhci->main_hcd, i, false, &flags); + + /* Power off USB3 ports*/ + for (i = 0; i < xhci->usb3_rhub.num_ports; i++) + xhci_set_port_power(xhci, xhci->shared_hcd, i, false, &flags); + /* Workaround for spurious wakeups at shutdown with HSW */ if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) xhci_reset(xhci, XHCI_RESET_SHORT_USEC); - spin_unlock_irq(&xhci->lock); + spin_unlock_irqrestore(&xhci->lock, flags); xhci_cleanup_msix(xhci); diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 0bd76c94a4b1..28aaf031f9a8 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -2196,6 +2196,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex, int xhci_hub_status_data(struct usb_hcd *hcd, char *buf); int xhci_find_raw_port_number(struct usb_hcd *hcd, int port1); struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd); +void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, u16 index, + bool on, unsigned long *flags); void xhci_hc_died(struct xhci_hcd *xhci); -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] xhci: turn off port power in shutdown 2022-06-23 11:19 ` [PATCH 2/4] xhci: turn off port power in shutdown Mathias Nyman @ 2022-07-19 13:42 ` Joey Corleone 2022-07-19 16:49 ` Mathias Nyman 0 siblings, 1 reply; 5+ messages in thread From: Joey Corleone @ 2022-07-19 13:42 UTC (permalink / raw) To: Mathias Nyman, gregkh; +Cc: linux-usb, stable, regressions On 23.06.22 13:19, Mathias Nyman wrote: > If ports are not turned off in shutdown then runtime suspended > self-powered USB devices may survive in U3 link state over S5. > > During subsequent boot, if firmware sends an IPC command to program > the port in DISCONNECT state, it will time out, causing significant > delay in the boot time. > > Turning off roothub port power is also recommended in xhci > specification 4.19.4 "Port Power" in the additional note. > > Cc: stable@vger.kernel.org > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > drivers/usb/host/xhci-hub.c | 2 +- > drivers/usb/host/xhci.c | 15 +++++++++++++-- > drivers/usb/host/xhci.h | 2 ++ > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index c54f2bc23d3f..0fdc014c9401 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -652,7 +652,7 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd) > * It will release and re-aquire the lock while calling ACPI > * method. > */ > -static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, > +void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, > u16 index, bool on, unsigned long *flags) > __must_hold(&xhci->lock) > { > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index cb99bed5f755..65858f607437 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -791,6 +791,8 @@ static void xhci_stop(struct usb_hcd *hcd) > void xhci_shutdown(struct usb_hcd *hcd) > { > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + unsigned long flags; > + int i; > > if (xhci->quirks & XHCI_SPURIOUS_REBOOT) > usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); > @@ -806,12 +808,21 @@ void xhci_shutdown(struct usb_hcd *hcd) > del_timer_sync(&xhci->shared_hcd->rh_timer); > } > > - spin_lock_irq(&xhci->lock); > + spin_lock_irqsave(&xhci->lock, flags); > xhci_halt(xhci); > + > + /* Power off USB2 ports*/ > + for (i = 0; i < xhci->usb2_rhub.num_ports; i++) > + xhci_set_port_power(xhci, xhci->main_hcd, i, false, &flags); > + > + /* Power off USB3 ports*/ > + for (i = 0; i < xhci->usb3_rhub.num_ports; i++) > + xhci_set_port_power(xhci, xhci->shared_hcd, i, false, &flags); > + > /* Workaround for spurious wakeups at shutdown with HSW */ > if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) > xhci_reset(xhci, XHCI_RESET_SHORT_USEC); > - spin_unlock_irq(&xhci->lock); > + spin_unlock_irqrestore(&xhci->lock, flags); > > xhci_cleanup_msix(xhci); > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 0bd76c94a4b1..28aaf031f9a8 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -2196,6 +2196,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex, > int xhci_hub_status_data(struct usb_hcd *hcd, char *buf); > int xhci_find_raw_port_number(struct usb_hcd *hcd, int port1); > struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd); > +void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, u16 index, > + bool on, unsigned long *flags); > > void xhci_hc_died(struct xhci_hcd *xhci); > fyi: there is a report [1] about a regression introduced by this patch. Joey [1] https://bugzilla.kernel.org/show_bug.cgi?id=216243 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] xhci: turn off port power in shutdown 2022-07-19 13:42 ` Joey Corleone @ 2022-07-19 16:49 ` Mathias Nyman 0 siblings, 0 replies; 5+ messages in thread From: Mathias Nyman @ 2022-07-19 16:49 UTC (permalink / raw) To: Joey Corleone, gregkh; +Cc: linux-usb, stable, regressions On 19.7.2022 16.42, Joey Corleone wrote: > On 23.06.22 13:19, Mathias Nyman wrote: >> If ports are not turned off in shutdown then runtime suspended >> self-powered USB devices may survive in U3 link state over S5. >> >> During subsequent boot, if firmware sends an IPC command to program >> the port in DISCONNECT state, it will time out, causing significant >> delay in the boot time. >> >> Turning off roothub port power is also recommended in xhci >> specification 4.19.4 "Port Power" in the additional note. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> --- >> drivers/usb/host/xhci-hub.c | 2 +- >> drivers/usb/host/xhci.c | 15 +++++++++++++-- >> drivers/usb/host/xhci.h | 2 ++ >> 3 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c >> index c54f2bc23d3f..0fdc014c9401 100644 >> --- a/drivers/usb/host/xhci-hub.c >> +++ b/drivers/usb/host/xhci-hub.c >> @@ -652,7 +652,7 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd) >> * It will release and re-aquire the lock while calling ACPI >> * method. >> */ >> -static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, >> +void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, >> u16 index, bool on, unsigned long *flags) >> __must_hold(&xhci->lock) >> { >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index cb99bed5f755..65858f607437 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -791,6 +791,8 @@ static void xhci_stop(struct usb_hcd *hcd) >> void xhci_shutdown(struct usb_hcd *hcd) >> { >> struct xhci_hcd *xhci = hcd_to_xhci(hcd); >> + unsigned long flags; >> + int i; >> if (xhci->quirks & XHCI_SPURIOUS_REBOOT) >> usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); >> @@ -806,12 +808,21 @@ void xhci_shutdown(struct usb_hcd *hcd) >> del_timer_sync(&xhci->shared_hcd->rh_timer); >> } >> - spin_lock_irq(&xhci->lock); >> + spin_lock_irqsave(&xhci->lock, flags); >> xhci_halt(xhci); >> + >> + /* Power off USB2 ports*/ >> + for (i = 0; i < xhci->usb2_rhub.num_ports; i++) >> + xhci_set_port_power(xhci, xhci->main_hcd, i, false, &flags); >> + >> + /* Power off USB3 ports*/ >> + for (i = 0; i < xhci->usb3_rhub.num_ports; i++) >> + xhci_set_port_power(xhci, xhci->shared_hcd, i, false, &flags); >> + >> /* Workaround for spurious wakeups at shutdown with HSW */ >> if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) >> xhci_reset(xhci, XHCI_RESET_SHORT_USEC); >> - spin_unlock_irq(&xhci->lock); >> + spin_unlock_irqrestore(&xhci->lock, flags); >> xhci_cleanup_msix(xhci); >> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h >> index 0bd76c94a4b1..28aaf031f9a8 100644 >> --- a/drivers/usb/host/xhci.h >> +++ b/drivers/usb/host/xhci.h >> @@ -2196,6 +2196,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex, >> int xhci_hub_status_data(struct usb_hcd *hcd, char *buf); >> int xhci_find_raw_port_number(struct usb_hcd *hcd, int port1); >> struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd); >> +void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, u16 index, >> + bool on, unsigned long *flags); >> void xhci_hc_died(struct xhci_hcd *xhci); > > fyi: there is a report [1] about a regression introduced by this patch. > > Joey > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=216243 Thanks for the notice, I'm away until August. If it starts to cause more issues then this one incident then it should be reverted. Will take a look at this when I return. Meanwhile a log with xhci dynamic debug enabled could better show what is going on mount -t debugfs none /sys/kernel/debug echo 'module xhci_hcd =p' >/sys/kernel/debug/dynamic_debug/control Thanks Mathias ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20220623111945.1557702-2-mathias.nyman@linux.intel.com>]
* Re: [PATCH 1/4] xhci: Keep interrupt disabled in initialization until host is running. [not found] ` <20220623111945.1557702-2-mathias.nyman@linux.intel.com> @ 2023-09-13 6:00 ` Prashanth K 2023-09-13 7:16 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Prashanth K @ 2023-09-13 6:00 UTC (permalink / raw) To: Mathias Nyman, gregkh; +Cc: linux-usb, Hongyu Xie, stable, Hongyu Xie, # 5 . 15 On 23-06-22 04:49 pm, Mathias Nyman wrote: > From: Hongyu Xie <xy521521@gmail.com> > > irq is disabled in xhci_quiesce(called by xhci_halt, with bit:2 cleared > in USBCMD register), but xhci_run(called by usb_add_hcd) re-enable it. > It's possible that you will receive thousands of interrupt requests > after initialization for 2.0 roothub. And you will get a lot of > warning like, "xHCI dying, ignoring interrupt. Shouldn't IRQs be > disabled?". This amount of interrupt requests will cause the entire > system to freeze. > This problem was first found on a device with ASM2142 host controller > on it. > > [tidy up old code while moving it, reword header -Mathias] > Cc: stable@kernel.org > Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > drivers/usb/host/xhci.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 9ac56e9ffc64..cb99bed5f755 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -611,15 +611,37 @@ static int xhci_init(struct usb_hcd *hcd) > > static int xhci_run_finished(struct xhci_hcd *xhci) > { > + unsigned long flags; > + u32 temp; > + > + /* > + * Enable interrupts before starting the host (xhci 4.2 and 5.5.2). > + * Protect the short window before host is running with a lock > + */ > + spin_lock_irqsave(&xhci->lock, flags); > + > + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Enable interrupts"); > + temp = readl(&xhci->op_regs->command); > + temp |= (CMD_EIE); > + writel(temp, &xhci->op_regs->command); > + > + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Enable primary interrupter"); > + temp = readl(&xhci->ir_set->irq_pending); > + writel(ER_IRQ_ENABLE(temp), &xhci->ir_set->irq_pending); > + > if (xhci_start(xhci)) { > xhci_halt(xhci); > + spin_unlock_irqrestore(&xhci->lock, flags); > return -ENODEV; > } > + > xhci->cmd_ring_state = CMD_RING_STATE_RUNNING; > > if (xhci->quirks & XHCI_NEC_HOST) > xhci_ring_cmd_db(xhci); > > + spin_unlock_irqrestore(&xhci->lock, flags); > + > return 0; > } > > @@ -668,19 +690,6 @@ int xhci_run(struct usb_hcd *hcd) > temp |= (xhci->imod_interval / 250) & ER_IRQ_INTERVAL_MASK; > writel(temp, &xhci->ir_set->irq_control); > > - /* Set the HCD state before we enable the irqs */ > - temp = readl(&xhci->op_regs->command); > - temp |= (CMD_EIE); > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > - "// Enable interrupts, cmd = 0x%x.", temp); > - writel(temp, &xhci->op_regs->command); > - > - temp = readl(&xhci->ir_set->irq_pending); > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > - "// Enabling event ring interrupter %p by writing 0x%x to irq_pending", > - xhci->ir_set, (unsigned int) ER_IRQ_ENABLE(temp)); > - writel(ER_IRQ_ENABLE(temp), &xhci->ir_set->irq_pending); > - > if (xhci->quirks & XHCI_NEC_HOST) { > struct xhci_command *command; > This is not available to older kernels [< 5.19]. Can we get this backported to 5.15 as well? Please let me know if there is some other way to do it. Cc: <stable@vger.kernel.org> # 5.15 Thanks, Prashanth K ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/4] xhci: Keep interrupt disabled in initialization until host is running. 2023-09-13 6:00 ` [PATCH 1/4] xhci: Keep interrupt disabled in initialization until host is running Prashanth K @ 2023-09-13 7:16 ` Greg KH 0 siblings, 0 replies; 5+ messages in thread From: Greg KH @ 2023-09-13 7:16 UTC (permalink / raw) To: Prashanth K Cc: Mathias Nyman, linux-usb, Hongyu Xie, stable, Hongyu Xie, # 5 . 15 On Wed, Sep 13, 2023 at 11:30:41AM +0530, Prashanth K wrote: > > > On 23-06-22 04:49 pm, Mathias Nyman wrote: > > From: Hongyu Xie <xy521521@gmail.com> > > > > irq is disabled in xhci_quiesce(called by xhci_halt, with bit:2 cleared > > in USBCMD register), but xhci_run(called by usb_add_hcd) re-enable it. > > It's possible that you will receive thousands of interrupt requests > > after initialization for 2.0 roothub. And you will get a lot of > > warning like, "xHCI dying, ignoring interrupt. Shouldn't IRQs be > > disabled?". This amount of interrupt requests will cause the entire > > system to freeze. > > This problem was first found on a device with ASM2142 host controller > > on it. > > > > [tidy up old code while moving it, reword header -Mathias] > > Cc: stable@kernel.org > > Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > --- > > drivers/usb/host/xhci.c | 35 ++++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 9ac56e9ffc64..cb99bed5f755 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -611,15 +611,37 @@ static int xhci_init(struct usb_hcd *hcd) > > static int xhci_run_finished(struct xhci_hcd *xhci) > > { > > + unsigned long flags; > > + u32 temp; > > + > > + /* > > + * Enable interrupts before starting the host (xhci 4.2 and 5.5.2). > > + * Protect the short window before host is running with a lock > > + */ > > + spin_lock_irqsave(&xhci->lock, flags); > > + > > + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Enable interrupts"); > > + temp = readl(&xhci->op_regs->command); > > + temp |= (CMD_EIE); > > + writel(temp, &xhci->op_regs->command); > > + > > + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Enable primary interrupter"); > > + temp = readl(&xhci->ir_set->irq_pending); > > + writel(ER_IRQ_ENABLE(temp), &xhci->ir_set->irq_pending); > > + > > if (xhci_start(xhci)) { > > xhci_halt(xhci); > > + spin_unlock_irqrestore(&xhci->lock, flags); > > return -ENODEV; > > } > > + > > xhci->cmd_ring_state = CMD_RING_STATE_RUNNING; > > if (xhci->quirks & XHCI_NEC_HOST) > > xhci_ring_cmd_db(xhci); > > + spin_unlock_irqrestore(&xhci->lock, flags); > > + > > return 0; > > } > > @@ -668,19 +690,6 @@ int xhci_run(struct usb_hcd *hcd) > > temp |= (xhci->imod_interval / 250) & ER_IRQ_INTERVAL_MASK; > > writel(temp, &xhci->ir_set->irq_control); > > - /* Set the HCD state before we enable the irqs */ > > - temp = readl(&xhci->op_regs->command); > > - temp |= (CMD_EIE); > > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > > - "// Enable interrupts, cmd = 0x%x.", temp); > > - writel(temp, &xhci->op_regs->command); > > - > > - temp = readl(&xhci->ir_set->irq_pending); > > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > > - "// Enabling event ring interrupter %p by writing 0x%x to irq_pending", > > - xhci->ir_set, (unsigned int) ER_IRQ_ENABLE(temp)); > > - writel(ER_IRQ_ENABLE(temp), &xhci->ir_set->irq_pending); > > - > > if (xhci->quirks & XHCI_NEC_HOST) { > > struct xhci_command *command; > This is not available to older kernels [< 5.19]. Can we get this backported > to 5.15 as well? Please let me know if there is some other way to do it. > > Cc: <stable@vger.kernel.org> # 5.15 <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-13 7:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220623111945.1557702-1-mathias.nyman@linux.intel.com>
2022-06-23 11:19 ` [PATCH 2/4] xhci: turn off port power in shutdown Mathias Nyman
2022-07-19 13:42 ` Joey Corleone
2022-07-19 16:49 ` Mathias Nyman
[not found] ` <20220623111945.1557702-2-mathias.nyman@linux.intel.com>
2023-09-13 6:00 ` [PATCH 1/4] xhci: Keep interrupt disabled in initialization until host is running Prashanth K
2023-09-13 7:16 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox