Re: [PATCH v3 3/6] usb: gadget: udc: crg: Add ring and transfer management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] usb: gadget: crg: cycle bit corruption in crg_udc_squeeze_xfer_ring()
- [High] usb: gadget: crg: premature loop break in crg_udc_queue_pending_trbs()
- [High] usb: gadget: crg: unsafe queue iteration across crg_udc_req_done() lock drop
- [High] usb: gadget: udc: crg: stale request state in crg_udc_squeeze_xfer_ring()
- [High] usb: gadget: crg: race updating evt_dq_pt after lock drop in crg_udc_process_event_ring()
--

commit ac2a648308943ddb5f71e791a2ef85c73d0020b9
Author: Petar Stepanovic <pstepanovic@axiado.com>

usb: gadget: udc: crg: Add ring and transfer management

This patch adds the memory management and ring buffer logic for TRBs and
event rings used by the Axiado AX3000/AX3005 USB device controller.

> diff --git a/drivers/usb/gadget/udc/crg/crg-udc-ring.c b/drivers/usb/gadget/udc/crg/crg-udc-ring.c
> new file mode 100644
> index 0000000000000..1c57dc1787d42
> --- /dev/null
> +++ b/drivers/usb/gadget/udc/crg/crg-udc-ring.c

[ ... ]

> +int crg_udc_process_event_ring(struct crg_gadget_dev *crg_udc, int index,
> +			       unsigned long *flags)
> +{
> +	struct crg_event_trb *event;
> +	struct crg_udc_event *udc_event;
> +	struct crg_uicr __iomem *uicr = crg_udc->uicr[index];
> +	u32 val;
> +	dma_addr_t erdp;
> +	int ret;
> +	unsigned int processed = 0;
> +
> +	if (!uicr)
> +		return -ENODEV;
> +
> +	val = readl(&uicr->iman);
> +	val |= CRG_U3DC_IMAN_INT_PEND;
> +	writel(val, &uicr->iman);
> +
> +	udc_event = &crg_udc->udc_event[index];
> +	while (udc_event->evt_dq_pt) {
> +		event = (struct crg_event_trb *)udc_event->evt_dq_pt;
> +
> +		if (FIELD_GET(EVE_TRB_CYCLE_BIT_MASK,
> +			      le32_to_cpu(event->dw3)) != udc_event->ccs)
> +			break;
> +		/* Read the payload only after observing device ownership. */
> +		dma_rmb();
> +
> +		ret = crg_udc_handle_event(crg_udc, event, flags);
> +		if (ret == -ECONNRESET) {
> +			/* The disconnect path rebuilt and reprogrammed the ring. */
> +			return 0;
> +		}
> +		if (ret < 0)
> +			dev_dbg(crg_udc->dev, "failed to process event: %d\n",
> +				ret);
> +
> +		/* Consume malformed events so they cannot pin the ring head. */
> +		if (event == udc_event->evt_seg0_last_trb) {
> +			udc_event->ccs = udc_event->ccs ? 0 : 1;
> +			udc_event->evt_dq_pt = udc_event->event_ring.cpu_addr;
> +		} else {
> +			udc_event->evt_dq_pt++;
> +		}

[Severity: High]
Can crg_udc_process_event_ring() corrupt udc_event->evt_dq_pt and
udc_event->ccs if the event ring is reinitialized while crg_udc->udc_lock
is dropped inside crg_udc_handle_event()?

Several paths inside crg_udc_handle_event() drop and re-acquire udc_lock
and then return 0 (for example, crg_udc_req_done() during transfer
completion, or crg_udc_handle_setup_pkt() during flush_work() and
driver->setup()).

If crg_gadget_stop() runs concurrently on another CPU during that lock
drop, it acquires udc_lock and calls:

crg_gadget_stop()
  crg_udc_reset_data_struct()
    crg_udc_init_event_ring()

which zeroes the event ring, resets udc_event->evt_dq_pt to
event_ring.cpu_addr, sets udc_event->ccs to 1, and programs uicr->erdplo
and uicr->erdphi to the start of the ring.

When crg_udc_handle_event() re-acquires udc_lock and returns 0,
crg_udc_process_event_ring() uses the stale pre-call event pointer to
advance udc_event->evt_dq_pt to index 1 (or, if event was
evt_seg0_last_trb, toggles udc_event->ccs from 1 to 0, causing the
subsequent loop iterations to treat all zeroed TRBs in the freshly cleared
ring as valid events).

[ ... ]

> +int crg_udc_queue_pending_trbs(struct crg_udc_ep *udc_ep_ptr)
> +{
> +	struct crg_udc_request *udc_req_ptr, *tmp;
> +	int ret;
> +
> +	list_for_each_entry_safe(udc_req_ptr, tmp, &udc_ep_ptr->queue, queue) {
> +		if (udc_req_ptr->all_trbs_queued)
> +			continue;
> +
> +		ret = crg_udc_build_td(udc_ep_ptr, udc_req_ptr);
> +		if (ret == -ENOSPC || ret == -EBUSY) {
> +			/*
> +			 * Ring is full; leave this request queued and retry
> +			 * it (and anything behind it) on the next TRB
> +			 * completion via crg_udc_queue_pending_trbs().
> +			 */
> +			return ret;
> +		} else if (ret) {
> +			/*
> +			 * A non-capacity failure is not transient: fail this
> +			 * request instead of leaving it stuck forever, and
> +			 * keep rebuilding the rest of the queue.
> +			 */
> +			dev_err(udc_ep_ptr->crg_udc->dev,
> +				"failed to build pending transfer: %d\n", ret);
> +			list_del_init(&udc_req_ptr->queue);
> +			crg_udc_req_done(udc_ep_ptr, udc_req_ptr, ret);
> +			continue;
> +		}

[Severity: High]
Is it safe to continue list_for_each_entry_safe() after calling
crg_udc_req_done() here (and in crg_udc_squeeze_xfer_ring() below)?

As documented on crg_udc_req_done() in crg-udc.h, crg_udc_req_done() drops
crg_udc->udc_lock around usb_gadget_giveback_request() and re-acquires it
before returning.

While udc_lock is dropped, the completion callback or another CPU (via
crg_udc_ep_dequeue(), crg_udc_ep_disable(), or crg_udc_ep_nuke()) can
unlink via list_del_init() and free the pre-fetched tmp request. Resuming
iteration with the cached tmp pointer can then trigger a use-after-free or
spin forever if tmp->queue was reinitialized with list_del_init().

> +		break;
> +	}

[Severity: High]
Should this unconditional break instead check udc_ep_ptr->tran_ring_full
like crg_udc_squeeze_xfer_ring() does?

With an unconditional break after the first crg_udc_build_td() succeeds,
crg_udc_queue_pending_trbs() schedules at most one pending request per
call even when space remains on the transfer ring. If multiple requests
were queued while the ring was full and the first rebuilt request has
usb_req.no_interrupt set, no completion interrupt will fire and the
remaining pending requests in udc_ep_ptr->queue will stall.

[ ... ]

> +void crg_udc_setup_link_trb(struct crg_transfer_trb *link_trb, bool toggle,
> +			    dma_addr_t next_trb)
> +{
> +	u32 dw = 0;
> +
> +	link_trb->dw0 = cpu_to_le32(lower_32_bits(next_trb));
> +	link_trb->dw1 = cpu_to_le32(upper_32_bits(next_trb));
> +	link_trb->dw2 = cpu_to_le32(0);
> +
> +	FIELD_MODIFY(TRB_TYPE_MASK, &dw, TRB_TYPE_LINK);
> +	if (toggle)
> +		FIELD_MODIFY(TRB_LINK_TOGGLE_CYCLE_MASK, &dw, 1);
> +	else
> +		FIELD_MODIFY(TRB_LINK_TOGGLE_CYCLE_MASK, &dw, 0);
> +
> +	/* Publish ownership only after the link address is visible. */
> +	dma_wmb();
> +	link_trb->dw3 = cpu_to_le32(dw);
> +}

[ ... ]

> +void crg_udc_squeeze_xfer_ring(struct crg_udc_ep *udc_ep_ptr,
> +			       struct crg_udc_request *udc_req_ptr)
> +{
> +	struct crg_transfer_trb *trb = udc_req_ptr->first_trb;
> +	struct crg_udc_request *next_req, *tmp;
> +	u8 new_pcs;
> +
> +	if (!trb)
> +		return;
> +
> +	/* The canceled request becomes the new enqueue point. Its original
> +	 * cycle bit is the cycle expected at that point after the tail is
> +	 * removed.
> +	 */
> +	new_pcs = FIELD_GET(TRB_CYCLE_BIT_MASK, le32_to_cpu(trb->dw3));
> +
> +	while (trb != udc_ep_ptr->enq_pt) {
> +		memset(trb, 0, sizeof(*trb));
> +		trb++;
> +		if (trb == udc_ep_ptr->last_trb)
> +			trb = udc_ep_ptr->first_trb;
> +	}
> +
> +	udc_ep_ptr->enq_pt = udc_req_ptr->first_trb;
> +	udc_ep_ptr->pcs = new_pcs;
> +	udc_ep_ptr->tran_ring_full = false;
> +	crg_udc_setup_link_trb(udc_ep_ptr->last_trb, true,
> +			udc_ep_ptr->tran_ring_info.dma_addr);
> +	dma_wmb();

[Severity: High]
Does zeroing the TRBs and unconditionally resetting udc_ep_ptr->last_trb
corrupt the transfer ring cycle state when new_pcs is 0 or when an active
request wraps the ring?

Because the link TRB toggles the cycle state on each ring wrap, the
expected producer/consumer cycle state alternates between 1 and 0.

First, memset(trb, 0, sizeof(*trb)) clears TRB_CYCLE_BIT_MASK to 0 and
TRB_TYPE_MASK to 0 (TRB_TYPE_RSVD). On a ring pass where new_pcs == 0 (or
if the cleared range wraps past last_trb into a pcs == 0 pass), a cycle
bit of 0 marks the cleared slots as hardware-owned rather than unowned.
If udc_req_ptr was the last queued request (or rebuilt requests consume
fewer TRBs than were wiped), the TRB at the rewound udc_ep_ptr->enq_pt will
have cycle bit 0 == udc_ep_ptr->pcs and type TRB_TYPE_RSVD, allowing the
controller to fetch a zeroed TRB instead of stopping at enq_pt.

Second, crg_udc_setup_link_trb() always writes dw3 with cycle bit 0 and
chain bit 0. Calling it unconditionally on udc_ep_ptr->last_trb will
overwrite last_trb's cycle bit and chain bit even if an earlier,
still-active request ahead of udc_req_ptr in udc_ep_ptr->queue wrapped
across last_trb during a pcs == 1 pass, invalidating the link TRB before
hardware consumes it.

> +	if (list_is_last(&udc_req_ptr->queue, &udc_ep_ptr->queue))
> +		return;
> +
> +	next_req = udc_req_ptr;
> +	list_for_each_entry_safe_continue(next_req, tmp, &udc_ep_ptr->queue,
> +					  queue) {
> +		int ret;
> +
> +		next_req->usb_req.status = -EINPROGRESS;
> +		next_req->usb_req.actual = 0;
> +		crg_udc_clear_req_container(next_req);
> +		ret = crg_udc_build_td(udc_ep_ptr, next_req);
> +		if (ret == -ENOSPC) {
> +			/* Ring is full; a later TRB completion will retry
> +			 * this and any remaining requests via
> +			 * crg_udc_queue_pending_trbs().
> +			 */
> +			udc_ep_ptr->tran_ring_full = true;
> +			break;
> +		} else if (ret) {
> +			/*
> +			 * A non-capacity failure is not transient: fail this
> +			 * request instead of leaving it stuck forever, and
> +			 * keep rebuilding the rest of the queue.
> +			 */
> +			dev_err(udc_ep_ptr->crg_udc->dev,
> +				"failed to rebuild queued request: %d\n", ret);
> +			crg_udc_req_done(udc_ep_ptr, next_req, ret);
> +			continue;
> +		}
> +		if (udc_ep_ptr->tran_ring_full)
> +			break;
> +	}
> +}

[Severity: High]
What happens to the remaining requests in udc_ep_ptr->queue if this loop
breaks out early when the ring fills up?

The while (trb != udc_ep_ptr->enq_pt) loop above already wiped the hardware
TRBs for all requests after udc_req_ptr, but crg_udc_clear_req_container()
is only called inside list_for_each_entry_safe_continue() right before each
crg_udc_build_td() call.

If an earlier request fills the ring and hits either break, any subsequent
requests in udc_ep_ptr->queue that previously had TRBs queued are left with
all_trbs_queued == true, buff_len_left == 0, and stale first_trb/last_trb
pointers. When a later TRB completion runs crg_udc_queue_pending_trbs(), it
checks:

	if (udc_req_ptr->all_trbs_queued)
		continue;

and skips those requests permanently.

Also, for isochronous endpoints, crg_udc_build_td() -> crg_udc_queue_trbs()
returns -EBUSY when the ring lacks space. Unlike crg_udc_queue_pending_trbs()
which checks for both -ENOSPC and -EBUSY, this check only tests for
ret == -ENOSPC, so -EBUSY falls into the error branch and fails the request
via crg_udc_req_done().

[ ... ]

完美

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-axiado-ax3000-usb-device-controller-v3-0-3012ecfc0a93@axiado.com?part=3