linux-kernel - Re: [PATCH v24 05/34] usb: host: xhci-mem: Cleanup pending secondary event ring events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b8e67410-49a0-4ee8-a0a5-4b7ba03483c8@linux.intel.com>
Date: Tue, 6 Aug 2024 16:50:06 +0200
From: Amadeusz Sławiński
 <amadeuszx.slawinski@...ux.intel.com>
To: Wesley Cheng <quic_wcheng@...cinc.com>, srinivas.kandagatla@...aro.org,
 mathias.nyman@...el.com, perex@...ex.cz, conor+dt@...nel.org,
 corbet@....net, broonie@...nel.org, lgirdwood@...il.com, krzk+dt@...nel.org,
 Thinh.Nguyen@...opsys.com, bgoswami@...cinc.com, tiwai@...e.com,
 gregkh@...uxfoundation.org, robh@...nel.org
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-sound@...r.kernel.org, linux-usb@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-doc@...r.kernel.org,
 alsa-devel@...a-project.org
Subject: Re: [PATCH v24 05/34] usb: host: xhci-mem: Cleanup pending secondary
 event ring events

On 8/1/2024 3:17 AM, Wesley Cheng wrote:
> As part of xHCI bus suspend, the XHCI is halted.  However, if there are

XHCI -> xHCI

> pending events in the secondary event ring, it is observed that the xHCI
> controller stops responding to further commands upon host or device
> initiated bus resume.  Iterate through all pending events and update the
> dequeue pointer to the beginning of the event ring.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
>   drivers/usb/host/xhci-mem.c  |  7 ++++++-
>   drivers/usb/host/xhci-ring.c | 33 ++++++++++++++++++++++++++++++++-
>   drivers/usb/host/xhci.c      |  2 +-
>   drivers/usb/host/xhci.h      |  6 ++++++
>   4 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 2ca5937b73f4..60dfc59260d8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1816,7 +1816,7 @@ xhci_remove_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>   		tmp &= ERST_SIZE_MASK;
>   		writel(tmp, &ir->ir_set->erst_size);
>   
> -		xhci_write_64(xhci, ERST_EHB, &ir->ir_set->erst_dequeue);
> +		xhci_update_erst_dequeue(xhci, ir, true);
>   	}
>   }
>   
> @@ -1859,6 +1859,11 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
>   		return;
>   	}
>   
> +	/*
> +	 * Cleanup secondary interrupter to ensure there are no pending events.
> +	 * This also updates event ring dequeue pointer back to the start.
> +	 */
> +	xhci_skip_sec_intr_events(xhci, ir->event_ring, ir);
>   	intr_num = ir->intr_num;
>   
>   	xhci_remove_interrupter(xhci, ir);
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 80dca780317a..a012ddf94fb5 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3012,7 +3012,7 @@ static int xhci_handle_event_trb(struct xhci_hcd *xhci, struct xhci_interrupter
>    * - When all events have finished
>    * - To avoid "Event Ring Full Error" condition
>    */
> -static void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
> +void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
>   				     struct xhci_interrupter *ir,
>   				     bool clear_ehb)
>   {
> @@ -3112,6 +3112,37 @@ static int xhci_handle_events(struct xhci_hcd *xhci, struct xhci_interrupter *ir
>   	return 0;
>   }
>   
> +/*
> + * Move the event ring dequeue pointer to skip events kept in the secondary
> + * event ring.  This is used to ensure that pending events in the ring are
> + * acknowledged, so the XHCI HCD can properly enter suspend/resume.  The

XHCI -> xHCI

> + * secondary ring is typically maintained by an external component.
> + */
> +void xhci_skip_sec_intr_events(struct xhci_hcd *xhci,
> +	struct xhci_ring *ring,	struct xhci_interrupter *ir)
> +{
> +	union xhci_trb *current_trb;
> +	u64 erdp_reg;
> +	dma_addr_t deq;
> +
> +	/* disable irq, ack pending interrupt and ack all pending events */
> +	xhci_disable_interrupter(ir);

That seems like a bit weird place to disable interrupter? Perhaps 
suspend and resume functions should be updated to handle more 
interrupters instead?