public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
* re: [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume
@ 2025-03-21  6:47 liudingyuan
  2025-03-21 10:06 ` Mathias Nyman
  0 siblings, 1 reply; 6+ messages in thread
From: liudingyuan @ 2025-03-21  6:47 UTC (permalink / raw)
  To: Mathias Nyman, gregkh@linuxfoundation.org,
	patchwork-bot@kernel.org, mricon@kernel.org,
	linux-usb@vger.kernel.org
  Cc: Fangjian (Jay), Kangfenglong, yangxingui, fengsheng (A),
	lingmingqiang, liulongfang, zhonghaoquan, yanzhili (A),
	huyihua (A), Zengtao (B), shenjian (K), liuyonglong,
	Jonathan Cameron

I have already tested that this solution can solve the problem.
And the specific verification details were described in the previous reply.

Thanks!

Tested-by: Devyn Liu <liudingyuan@huawei.com>
>
>  [RFT PATCH] xhci: Limit time spent with interrupts disabled during 
> bus resume
>
>  Current xhci bus resume implementation prevents xHC host from generating interrupts during high-speed USB 2 and super-speed USB 3 bus resume.
>
>  Only reason to disable interrupts during bus resume would be to prevent the interrupt handler from interfering with the resume process of USB 2 ports.
>
>  Host initiated resume of USB 2 ports is done in two stages.
>
>  The xhci driver first transitions the port from 'U3' to 'Resume' state, then wait in Resume for 20ms, and finally moves port to U0 state.
>  xhci driver can't prevent interrupts by taking and keeping the xhci spinlock with spin_lock_irqsave() due to this 20ms sleep.
>
>  Limit interrupt disabling to the USB 2 port resume case only.
>  resuming USB 2 ports in bus resume is only done in special cases where USB 2 ports had to be forced to suspend during bus suspend.
>
>  The current way of preventing interrupts by clearing the 'Interrupt Enable' (INTE) bit in USBCMD register won't prevent the Interrupter registers 'Interrupt Pending' (IP), 'Event Handler Busy' (EHB) and USBSTS register Event Interrupt (EINT) bits from being set.
>
>  New interrupts can't be issued before those bits are properly clered.
>
>  This way IP, EHB and INTE won't be set before IE is enabled again and a new interrupt is triggered.
>
>  Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>  ---
>  drivers/usb/host/xhci-hub.c | 30 ++++++++++++++++--------------
>  drivers/usb/host/xhci.c     |  4 ++--
>  drivers/usb/host/xhci.h     |  2 ++
>  3 files changed, 20 insertions(+), 16 deletions(-)
>
>  diff --git a/drivers/usb/host/xhci-hub.c 
> b/drivers/usb/host/xhci-hub.c index 9693464c0520..d11b60f705bb 100644
>  --- a/drivers/usb/host/xhci-hub.c
>  +++ b/drivers/usb/host/xhci-hub.c
>  @@ -1870,9 +1870,10 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   	int max_ports, port_index;
>   	int sret;
>   	u32 next_state;
>  -	    u32 temp, portsc;
>  +	u32 portsc;
>   	struct xhci_hub *rhub;
>  	    struct xhci_port **ports;
>  +	bool disabled_irq = false;
>  
>  	    rhub = xhci_get_rhub(hcd);
>  	    ports = rhub->ports;
> @@ -1888,17 +1889,20 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   		return -ESHUTDOWN;
>   	}
>   
>  -	    /* delay the irqs */
>  -	    temp = readl(&xhci->op_regs->command);
>  -	    temp &= ~CMD_EIE;
>  -	    writel(temp, &xhci->op_regs->command);
>  -
>    	/* bus specific resume for ports we suspended at bus_suspend */
>  - 	if (hcd->speed >= HCD_USB3)
>  +	if (hcd->speed >= HCD_USB3) {
>  		next_state = XDEV_U0;
>  - 	else
>  +	} else {
>  		next_state = XDEV_RESUME;
>  -
>  +	if (bus_state->bus_suspended) {
>  +			/*
>  +			 * prevent port event interrupts from interfering
>  +			 * with usb2 port resume process
>  +			 */
>  +			xhci_disable_interrupter(xhci->interrupters[0]);
>  +			disabled_irq = true;
>  +		}
>  +	}
>   	    port_index = max_ports;
>   	    while (port_index--) {
>   		portsc = readl(ports[port_index]->addr); @@ -1966,11 +1970,9 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   	(void) readl(&xhci->op_regs->command);
>   
>   	bus_state->next_statechange = jiffies + msecs_to_jiffies(5);
>  -	    /* re-enable irqs */
>  -  	temp = readl(&xhci->op_regs->command);
>  -  	temp |= CMD_EIE;
>  - 	writel(temp, &xhci->op_regs->command);
>  -	    temp = readl(&xhci->op_regs->command);
>  +	/* re-enable interrupter */
>  +	if (disabled_irq)
>  +		xhci_enable_interrupter(xhci->interrupters[0]);
>   
>   	spin_unlock_irqrestore(&xhci->lock, flags);
>   	return 0;
>  diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 
> 0c22b78358b9..ad229cb9a90b 100644
>  --- a/drivers/usb/host/xhci.c
>  +++ b/drivers/usb/host/xhci.c
>  @@ -322,7 +322,7 @@ static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
>   		xhci_info(xhci, "Fault detected\n");
>    }
>   
>  -static int xhci_enable_interrupter(struct xhci_interrupter *ir)
> +int xhci_enable_interrupter(struct xhci_interrupter *ir)
>  
>   	u32 iman;
>  
>  @@ -335,7 +335,7 @@ static int xhci_enable_interrupter(struct xhci_interrupter *ir)
>   	return 0;
>   }
>   
>  -static int xhci_disable_interrupter(struct xhci_interrupter *ir)
> +int xhci_disable_interrupter(struct xhci_interrupter *ir)
>   {
>   	u32 iman;
>   
>  diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 
> 6c00062a9acc..10572336fabe 100644
>  --- a/drivers/usb/host/xhci.h
>  +++ b/drivers/usb/host/xhci.h
>  @@ -1891,6 +1891,8 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
>   		struct usb_tt *tt, gfp_t mem_flags);
>   int xhci_set_interrupter_moderation(struct xhci_interrupter *ir,
>   				    u32 imod_interval);
>  +int xhci_enable_interrupter(struct xhci_interrupter *ir); int
> +xhci_disable_interrupter(struct xhci_interrupter *ir);
>   
>   /* xHCI ring, segment, TRB, and TD functions */  dma_addr_t 
> xhci_trb_virt_to_dma(struct xhci_segment *seg, union xhci_trb *trb);
>  --
>  2.43.0



^ permalink raw reply	[flat|nested] 6+ messages in thread
* re: [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume
@ 2025-03-24  2:05 liudingyuan
  0 siblings, 0 replies; 6+ messages in thread
From: liudingyuan @ 2025-03-24  2:05 UTC (permalink / raw)
  To: Mathias Nyman, gregkh@linuxfoundation.org,
	patchwork-bot@kernel.org, mricon@kernel.org,
	linux-usb@vger.kernel.org
  Cc: Fangjian (Jay), Kangfenglong, yangxingui, fengsheng (A),
	lingmingqiang, liulongfang, zhonghaoquan, yanzhili (A),
	huyihua (A), Zengtao (B), shenjian (K), liuyonglong,
	Jonathan Cameron

Thank you very much for the fix!

It's ok to add me to the "Reported-by:" tag.

Thanks
Best regards
Devyn

> Thanks for testing it

> I'll submit the patch after the merge window, once 6.15-rc1 is out.

> Would you like me to also add a "Reported-by:" tag for you, or someone else in the team?

> Thanks
> Mathias


^ permalink raw reply	[flat|nested] 6+ messages in thread
* re: [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume
@ 2025-03-21  3:22 liudingyuan
  0 siblings, 0 replies; 6+ messages in thread
From: liudingyuan @ 2025-03-21  3:22 UTC (permalink / raw)
  To: Mathias Nyman, gregkh@linuxfoundation.org,
	patchwork-bot@kernel.org, mricon@kernel.org,
	linux-usb@vger.kernel.org
  Cc: Fangjian (Jay), Kangfenglong, yangxingui, fengsheng (A),
	lingmingqiang, liulongfang, zhonghaoquan, yanzhili (A),
	huyihua (A), Zengtao (B), shenjian (K), liuyonglong,
	Jonathan Cameron

Hi

Applying the patch based on the 6.14-rc6 kernel, we conducted verification and found that this patch can
resolve the issue of interrupt loss when a USB3 controller is connected only to a USB2 device.

Additionally, by constructing a scenario with a lower probability where a USB3 controller is connected only
to a USB3 device, causing the USB2 port to enter the resume process and leading to interrupt loss, we confirmed
that the operation of disabling interrupts through IE configuration in this patch can solve the problem of the
controller being unable to trigger new interrupts after interrupt enablement.

Thank you very much!  I believe this fix solution has been verified to be acceptable.

We also conducted more USB basic functionality tests , including connecting mice, keyboards, hard drives, 
and other devices, as well as read/write functions and multi-level USB device testing. Based on the current
results, this patch does not seem to affect the basic functionality of USB.

Hope these test results can help get the patch pushed to the mainline.

Thanks.
Best regards!
Devyn

>
>  [RFT PATCH] xhci: Limit time spent with interrupts disabled during 
> bus resume
>
>  Current xhci bus resume implementation prevents xHC host from generating interrupts during high-speed USB 2 and super-speed USB 3 bus resume.
>
>  Only reason to disable interrupts during bus resume would be to prevent the interrupt handler from interfering with the resume process of USB 2 ports.
>
>  Host initiated resume of USB 2 ports is done in two stages.
>
>  The xhci driver first transitions the port from 'U3' to 'Resume' state, then wait in Resume for 20ms, and finally moves port to U0 state.
>  xhci driver can't prevent interrupts by taking and keeping the xhci spinlock with spin_lock_irqsave() due to this 20ms sleep.
>
>  Limit interrupt disabling to the USB 2 port resume case only.
>  resuming USB 2 ports in bus resume is only done in special cases where USB 2 ports had to be forced to suspend during bus suspend.
>
>  The current way of preventing interrupts by clearing the 'Interrupt Enable' (INTE) bit in USBCMD register won't prevent the Interrupter registers 'Interrupt Pending' (IP), 'Event Handler Busy' (EHB) and USBSTS register Event Interrupt (EINT) bits from being set.
>
>  New interrupts can't be issued before those bits are properly clered.
>
>  This way IP, EHB and INTE won't be set before IE is enabled again and a new interrupt is triggered.
>
>  Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>  ---
>  drivers/usb/host/xhci-hub.c | 30 ++++++++++++++++--------------
>  drivers/usb/host/xhci.c     |  4 ++--
>  drivers/usb/host/xhci.h     |  2 ++
>  3 files changed, 20 insertions(+), 16 deletions(-)
>
>  diff --git a/drivers/usb/host/xhci-hub.c 
> b/drivers/usb/host/xhci-hub.c index 9693464c0520..d11b60f705bb 100644
>  --- a/drivers/usb/host/xhci-hub.c
>  +++ b/drivers/usb/host/xhci-hub.c
>  @@ -1870,9 +1870,10 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   	int max_ports, port_index;
>   	int sret;
>   	u32 next_state;
>  -	    u32 temp, portsc;
>  +	u32 portsc;
>   	struct xhci_hub *rhub;
>  	    struct xhci_port **ports;
>  +	bool disabled_irq = false;
>  
>  	    rhub = xhci_get_rhub(hcd);
>  	    ports = rhub->ports;
> @@ -1888,17 +1889,20 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   		return -ESHUTDOWN;
>   	}
>   
>  -	    /* delay the irqs */
>  -	    temp = readl(&xhci->op_regs->command);
>  -	    temp &= ~CMD_EIE;
>  -	    writel(temp, &xhci->op_regs->command);
>  -
>    	/* bus specific resume for ports we suspended at bus_suspend */
>  - 	if (hcd->speed >= HCD_USB3)
>  +	if (hcd->speed >= HCD_USB3) {
>  		next_state = XDEV_U0;
>  - 	else
>  +	} else {
>  		next_state = XDEV_RESUME;
>  -
>  +	if (bus_state->bus_suspended) {
>  +			/*
>  +			 * prevent port event interrupts from interfering
>  +			 * with usb2 port resume process
>  +			 */
>  +			xhci_disable_interrupter(xhci->interrupters[0]);
>  +			disabled_irq = true;
>  +		}
>  +	}
>   	    port_index = max_ports;
>   	    while (port_index--) {
>   		portsc = readl(ports[port_index]->addr); @@ -1966,11 +1970,9 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   	(void) readl(&xhci->op_regs->command);
>   
>   	bus_state->next_statechange = jiffies + msecs_to_jiffies(5);
>  -	    /* re-enable irqs */
>  -  	temp = readl(&xhci->op_regs->command);
>  -  	temp |= CMD_EIE;
>  - 	writel(temp, &xhci->op_regs->command);
>  -	    temp = readl(&xhci->op_regs->command);
>  +	/* re-enable interrupter */
>  +	if (disabled_irq)
>  +		xhci_enable_interrupter(xhci->interrupters[0]);
>   
>   	spin_unlock_irqrestore(&xhci->lock, flags);
>   	return 0;
>  diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 
> 0c22b78358b9..ad229cb9a90b 100644
>  --- a/drivers/usb/host/xhci.c
>  +++ b/drivers/usb/host/xhci.c
>  @@ -322,7 +322,7 @@ static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
>   		xhci_info(xhci, "Fault detected\n");
>    }
>   
>  -static int xhci_enable_interrupter(struct xhci_interrupter *ir)  
> +int xhci_enable_interrupter(struct xhci_interrupter *ir)
>  
>   	u32 iman;
>  
>  @@ -335,7 +335,7 @@ static int xhci_enable_interrupter(struct xhci_interrupter *ir)
>   	return 0;
>   }
>   
>  -static int xhci_disable_interrupter(struct xhci_interrupter *ir)  
> +int xhci_disable_interrupter(struct xhci_interrupter *ir)
>   {
>   	u32 iman;
>   
>  diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 
> 6c00062a9acc..10572336fabe 100644
>  --- a/drivers/usb/host/xhci.h
>  +++ b/drivers/usb/host/xhci.h
>  @@ -1891,6 +1891,8 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
>   		struct usb_tt *tt, gfp_t mem_flags);
>   int xhci_set_interrupter_moderation(struct xhci_interrupter *ir,
>   				    u32 imod_interval);
>  +int xhci_enable_interrupter(struct xhci_interrupter *ir); int  
> +xhci_disable_interrupter(struct xhci_interrupter *ir);
>   
>   /* xHCI ring, segment, TRB, and TD functions */  dma_addr_t 
> xhci_trb_virt_to_dma(struct xhci_segment *seg, union xhci_trb *trb);
>  --
>  2.43.0



^ permalink raw reply	[flat|nested] 6+ messages in thread
* re: [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume
@ 2025-03-11 12:10 liudingyuan
  0 siblings, 0 replies; 6+ messages in thread
From: liudingyuan @ 2025-03-11 12:10 UTC (permalink / raw)
  To: Mathias Nyman, gregkh@linuxfoundation.org,
	patchwork-bot@kernel.org, mricon@kernel.org,
	linux-usb@vger.kernel.org
  Cc: Fangjian (Jay), Kangfenglong, yangxingui, fengsheng (A),
	lingmingqiang, liulongfang, zhonghaoquan, yanzhili (A),
	huyihua (A), Zengtao (B), shenjian (K), liuyonglong,
	Jonathan Cameron

Hi

We have received the patch changing the way of interrupt disabling, thank you very much!
Over the past couple of days, we have also conducted tests using the modified kernel in the problematic scenarios.
I believe that this patch can resolve the issue and avoid some potential interrupt loss problems. Thank you once again!

We plan to conduct further testing on some basic USB functionalities based on the latest Linux kernel and our kernel in the coming period 
(including read/write operations with USB drives or hard disks, multi-level devices, and basic read/write stress tests, etc.). 

The testing is expected to take approximately two weeks to complete. We will finish the further verification process and provide you with feedback
about the results as soon as possible. 

During this period, we may also engage in further discussions with hardware personnel to ensure that the hardware implementation aligns with expectations.

Thanks.
Best regards!
Devyn

>
>  [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume
>
>  Current xhci bus resume implementation prevents xHC host from generating interrupts during high-speed USB 2 and super-speed USB 3 bus resume.
>
>  Only reason to disable interrupts during bus resume would be to prevent the interrupt handler from interfering with the resume process of USB 2 ports.
>
>  Host initiated resume of USB 2 ports is done in two stages.
>
>  The xhci driver first transitions the port from 'U3' to 'Resume' state, then wait in Resume for 20ms, and finally moves port to U0 state.
>  xhci driver can't prevent interrupts by taking and keeping the xhci spinlock with spin_lock_irqsave() due to this 20ms sleep.
>
>  Limit interrupt disabling to the USB 2 port resume case only.
>  resuming USB 2 ports in bus resume is only done in special cases where USB 2 ports had to be forced to suspend during bus suspend.
>
>  The current way of preventing interrupts by clearing the 'Interrupt Enable' (INTE) bit in USBCMD register won't prevent the Interrupter registers 'Interrupt Pending' (IP), 'Event Handler Busy' (EHB) and USBSTS register Event Interrupt (EINT) bits from being set.
>
>  New interrupts can't be issued before those bits are properly clered.
>
>  This way IP, EHB and INTE won't be set before IE is enabled again and a new interrupt is triggered.
>
>  Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>  ---
>  drivers/usb/host/xhci-hub.c | 30 ++++++++++++++++--------------
>  drivers/usb/host/xhci.c     |  4 ++--
>  drivers/usb/host/xhci.h     |  2 ++
>  3 files changed, 20 insertions(+), 16 deletions(-)
>
>  diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 9693464c0520..d11b60f705bb 100644
>  --- a/drivers/usb/host/xhci-hub.c
>  +++ b/drivers/usb/host/xhci-hub.c
>  @@ -1870,9 +1870,10 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   	int max_ports, port_index;
>   	int sret;
>   	u32 next_state;
>  -	    u32 temp, portsc;
>  +	u32 portsc;
>   	struct xhci_hub *rhub;
>  	    struct xhci_port **ports;
>  +	bool disabled_irq = false;
>  
>  	    rhub = xhci_get_rhub(hcd);
>  	    ports = rhub->ports;
> @@ -1888,17 +1889,20 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   		return -ESHUTDOWN;
>   	}
>   
>  -	    /* delay the irqs */
>  -	    temp = readl(&xhci->op_regs->command);
>  -	    temp &= ~CMD_EIE;
>  -	    writel(temp, &xhci->op_regs->command);
>  -
>    	/* bus specific resume for ports we suspended at bus_suspend */
>  - 	if (hcd->speed >= HCD_USB3)
>  +	if (hcd->speed >= HCD_USB3) {
>  		next_state = XDEV_U0;
>  - 	else
>  +	} else {
>  		next_state = XDEV_RESUME;
>  -
>  +	if (bus_state->bus_suspended) {
>  +			/*
>  +			 * prevent port event interrupts from interfering
>  +			 * with usb2 port resume process
>  +			 */
>  +			xhci_disable_interrupter(xhci->interrupters[0]);
>  +			disabled_irq = true;
>  +		}
>  +	}
>   	    port_index = max_ports;
>   	    while (port_index--) {
>   		portsc = readl(ports[port_index]->addr); @@ -1966,11 +1970,9 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>   	(void) readl(&xhci->op_regs->command);
>   
>   	bus_state->next_statechange = jiffies + msecs_to_jiffies(5);
>  -	    /* re-enable irqs */
>  -  	temp = readl(&xhci->op_regs->command);
>  -  	temp |= CMD_EIE;
>  - 	writel(temp, &xhci->op_regs->command);
>  -	    temp = readl(&xhci->op_regs->command);
>  +	/* re-enable interrupter */
>  +	if (disabled_irq)
>  +		xhci_enable_interrupter(xhci->interrupters[0]);
>   
>   	spin_unlock_irqrestore(&xhci->lock, flags);
>   	return 0;
>  diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 0c22b78358b9..ad229cb9a90b 100644
>  --- a/drivers/usb/host/xhci.c
>  +++ b/drivers/usb/host/xhci.c
>  @@ -322,7 +322,7 @@ static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
>   		xhci_info(xhci, "Fault detected\n");
>    } 
>   
>  -static int xhci_enable_interrupter(struct xhci_interrupter *ir)
>  +int xhci_enable_interrupter(struct xhci_interrupter *ir)
>  
>   	u32 iman;
>  
>  @@ -335,7 +335,7 @@ static int xhci_enable_interrupter(struct xhci_interrupter *ir)
>   	return 0;
>   }
>   
>  -static int xhci_disable_interrupter(struct xhci_interrupter *ir)
>  +int xhci_disable_interrupter(struct xhci_interrupter *ir)
>   {
>   	u32 iman;
>   
>  diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 6c00062a9acc..10572336fabe 100644
>  --- a/drivers/usb/host/xhci.h
>  +++ b/drivers/usb/host/xhci.h
>  @@ -1891,6 +1891,8 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
>   		struct usb_tt *tt, gfp_t mem_flags);
>   int xhci_set_interrupter_moderation(struct xhci_interrupter *ir,
>   				    u32 imod_interval);
>  +int xhci_enable_interrupter(struct xhci_interrupter *ir); int 
>  +xhci_disable_interrupter(struct xhci_interrupter *ir);
>   
>   /* xHCI ring, segment, TRB, and TD functions */  dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, union xhci_trb *trb);
>  --
>  2.43.0



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PROBLEM] usb: xhci_bus_resume cause irq lost issue
@ 2025-03-07 16:20 Mathias Nyman
  2025-03-10 15:11 ` [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume Mathias Nyman
  0 siblings, 1 reply; 6+ messages in thread
From: Mathias Nyman @ 2025-03-07 16:20 UTC (permalink / raw)
  To: liudingyuan, linux-usb@vger.kernel.org, greg@kroah.com,
	patchwork-bot@kernel.org, mricon@kernel.org
  Cc: Fangjian (Jay), Kangfenglong, yangxingui, fengsheng (A),
	lingmingqiang, liulongfang, zhonghaoquan, yanzhili (A),
	huyihua (A), Zengtao (B), shenjian (K), liuyonglong,
	Jonathan Cameron

On 6.3.2025 16.29, liudingyuan wrote:
> 
> I compiled a new kernel based on the fix code you provided below and conducted some preliminary tests.> In the repeated unregister/register tests of the xHCI controller that previously caused issues,  both the driver and USB-related functionalities are now working normally.
> (Moreover, this fix code, in theory, should completely resolve the issues we encountered in our USB3-USB2 device-only scenario.)
> 
> Based on the logic mentioned in analysis, we currently may not have implemented a better solution to avoid disabling interrupts during the USB2 resume process. I would like to ask if we need to
> be concerned about the issue of interrupt loss caused by disabling interrupts in other scenarios where resume and enumeration processes or transfer operations might conflict?
> 
> For example, when a user inserts a device during the USB2/USB3 port resume, or when the USB3 controller is only connected to a USB3 devices, and the USB2 port enters this resume flow due to auto-suspend?
>   (However, it seems that the probability of these two scenarios is very low, as we have not yet been able to reproduce errors under these conditions.)
> 
> This fix indeed helps us avoid the current issue, so I would like to ask if it is possible to push this modification as a patch to the mainline code?
> If possible, we also plan to conduct a comprehensive test of USB functionality based on this modification to further validate it.
> 
> 
> Considering that the fix cannot completely avoid all possible scenarios where interrupts might be lost due to the hardware IE (Interrupt Enable) being turned off.
> I would like to ask whether the hardware design is reasonable in the following case:
> when a hardware edge-triggered interrupt is lost due to IE being disabled, and the subsequent interrupts cannot be triggered because the software didn't
> clear the EHB (Event Handler Busy) bit.
> 

I think we can avoid this situation by disabling the primary interrupter instead of all interrupts.
Meaning we would clear the 'Interrupt enable (IE)' bit:1  in  Interrupter Management Register (IMAN)
instead of the 'Interrupter Enable' (INTE) bit:2 in USBCMD register.

This way EHB and IP shouldn't be set at all, and thus not prevent future interrupts.

In practice this just means calling xhci_disable_interrupter() xhci_enable_interrupter()
instead of clearing and setting CMD_EIE bit.

I'll write a patch for this next week
Grateful if you could run the more comprehensive test before I queue it for
upstream (mainline)

Thanks
Mathias


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-24  2:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-21  6:47 [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume liudingyuan
2025-03-21 10:06 ` Mathias Nyman
  -- strict thread matches above, loose matches on Subject: below --
2025-03-24  2:05 liudingyuan
2025-03-21  3:22 liudingyuan
2025-03-11 12:10 liudingyuan
2025-03-07 16:20 [PROBLEM] usb: xhci_bus_resume cause irq lost issue Mathias Nyman
2025-03-10 15:11 ` [RFT PATCH] xhci: Limit time spent with interrupts disabled during bus resume Mathias Nyman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox