linux-kernel - Re: [PATCH v4 04/32] 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: <7b31b220-6fd5-0f5d-7e1a-df3f38bd792f@linux.intel.com>
Date:   Thu, 3 Aug 2023 13:33:56 +0300
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Wesley Cheng <quic_wcheng@...cinc.com>, agross@...nel.org,
        andersson@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        catalin.marinas@....com, will@...nel.org, mathias.nyman@...el.com,
        gregkh@...uxfoundation.org, lgirdwood@...il.com,
        broonie@...nel.org, perex@...ex.cz, tiwai@...e.com,
        srinivas.kandagatla@...aro.org, bgoswami@...cinc.com,
        Thinh.Nguyen@...opsys.com
Cc:     linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-usb@...r.kernel.org, alsa-devel@...a-project.org,
        quic_jackp@...cinc.com, pierre-louis.bossart@...ux.intel.com,
        oneukum@...e.com, albertccwang@...gle.com, o-takashi@...amocchi.jp
Subject: Re: [PATCH v4 04/32] usb: host: xhci-mem: Cleanup pending secondary
 event ring events

On 25.7.2023 5.33, Wesley Cheng wrote:
> As part of xHCI bus suspend, the XHCI is halted.  However, if there are
> 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 updating the
> dequeue pointer to the last pending event trb.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
>   drivers/usb/host/xhci-mem.c | 74 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 69 insertions(+), 5 deletions(-)

This sounds more like ring handling code.
Maybe xhci-ring.c would be a better place

> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index c51150af22f2..6b01d56c176f 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1799,17 +1799,85 @@ int xhci_alloc_erst(struct xhci_hcd *xhci,
>   	return 0;
>   }
>   
> +static void xhci_handle_sec_intr_events(struct xhci_hcd *xhci,
> +	struct xhci_ring *ring,	struct xhci_intr_reg __iomem *ir_set,
> +	struct xhci_erst *erst)
> +{

The function name is a bit misleading as it doesn't handle
any of the pending events, it just marks them all handled.

> +	union xhci_trb *erdp_trb, *current_trb;
> +	struct xhci_segment	*seg;
> +	u64 erdp_reg;
> +	u32 iman_reg;
> +	dma_addr_t deq;
> +	unsigned long segment_offset;
> +
> +	/* disable irq, ack pending interrupt and ack all pending events */
> +	iman_reg = readl_relaxed(&ir_set->irq_pending);
> +	iman_reg &= ~IMAN_IE;
> +	writel_relaxed(iman_reg, &ir_set->irq_pending);
> +	iman_reg = readl_relaxed(&ir_set->irq_pending);
> +	if (iman_reg & IMAN_IP)
> +		writel_relaxed(iman_reg, &ir_set->irq_pending);

maybe use xhci_disable_interrupter() helper, it does most of this already.

> +
> +	/* last acked event trb is in erdp reg  */
> +	erdp_reg = xhci_read_64(xhci, &ir_set->erst_dequeue);
> +	deq = (dma_addr_t)(erdp_reg & ~ERST_PTR_MASK);
> +	if (!deq) {
> +		xhci_dbg(xhci, "event ring handling not required\n");
> +		return;
> +	}
> +
> +	seg = ring->first_seg;
> +	segment_offset = deq - seg->dma;
> +
> +	/* find out virtual address of the last acked event trb */
> +	erdp_trb = current_trb = &seg->trbs[0] +
> +				(segment_offset/sizeof(*current_trb));
> +
> +	/* read cycle state of the last acked trb to find out CCS */
> +	ring->cycle_state = le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE;
> +
> +	while (1) {
> +		/* last trb of the event ring: toggle cycle state */
> +		if (current_trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) {
> +			ring->cycle_state ^= 1;
> +			current_trb = &seg->trbs[0];
> +		} else {
> +			current_trb++;
> +		}
> +
> +		/* cycle state transition */
> +		if ((le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE) !=
> +		    ring->cycle_state)
> +			break;
> +	}
> +
> +	if (erdp_trb != current_trb) {
> +		deq = xhci_trb_virt_to_dma(ring->deq_seg, current_trb);
> +		if (deq == 0)
> +			xhci_warn(xhci,
> +				"WARN invalid SW event ring dequeue ptr.\n");
> +		/* Update HC event ring dequeue pointer */
> +		erdp_reg &= ERST_PTR_MASK;
> +		erdp_reg |= ((u64) deq & (u64) ~ERST_PTR_MASK);
> +	}
> +
> +	/* Clear the event handler busy flag (RW1C); event ring is empty. */
> +	erdp_reg |= ERST_EHB;
> +	xhci_write_64(xhci, erdp_reg, &ir_set->erst_dequeue);

There are some helpers like inc_deq() and  xhci_update_erst_dequeue()
that could be used here.

> +}
> +
>   static void
>   xhci_free_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>   {
>   	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>   	size_t erst_size;
> -	u64 tmp64;
>   	u32 tmp;
>   
>   	if (!ir)
>   		return;
>   
> +	xhci_handle_sec_intr_events(xhci, ir->event_ring, ir->ir_set, &ir->erst);

Cleaning up the interrupter event ring should be called earlier.
  
Probably from xhci_remove_secondary_interrupter(), before it calls xhci_free_interrupter()

Thanks
-Mathias