Re: [PATCH v3 5/6] usb: gadget: udc: crg: Add gadget and endpoint operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] usb: gadget: crg: TRB interleaving, premature ZLP, and stall on multi-TD rings
- [Critical] usb: gadget: crg: dangling ring pointers and unlocked free in crg_udc_ep_enable()
- [Critical] usb: gadget: crg: race and missing async_callbacks check in crg_udc_reset_work()
- [High] usb: gadget: crg: broken last_trb calculation in crg_udc_queue_trbs()
- [High] usb: gadget: crg: broken CMPL_CODE_SHORT_PKT handling in crg_udc_handle_xfer_event()
- [High] usb: gadget: crg: wrong dequeue cycle state and stalled ring on dequeue/unhalt
- [High] usb: gadget: crg: missing hardware disable wait and lock-drop race in crg_udc_ep_disable()
- [High] usb: gadget: crg: stale state updates after lock drop in crg_udc_req_done()
- [High] usb: gadget: crg: High-Speed maxburst always 0 in crg_udc_epcx_setup()
- [High] usb: gadget: crg: broken PLC resume, state, and disconnect in crg_udc_handle_port_status()
- [High] usb: gadget: crg: missing state revalidation after lock drop in crg_udc_ep_queue()
- [Medium] usb: gadget: crg: lockless state updates in crg_gadget_stop() and start()
--

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

usb: gadget: udc: crg: Add gadget and endpoint operations

This commit implements the USB gadget and endpoint operations, TRB ring
queueing, and event handling for the Axiado AX3000/AX3005 UDC driver.

> diff --git a/drivers/usb/gadget/udc/crg/crg-udc-gadget.c b/drivers/usb/gadget/udc/crg/crg-udc-gadget.c
> new file mode 100644
> index 0000000000000..a330e857a9509
> --- /dev/null
> +++ b/drivers/usb/gadget/udc/crg/crg-udc-gadget.c
> @@ -0,0 +1,2299 @@

[ ... ]

> +static int crg_gadget_start(struct usb_gadget *g,
> +			    struct usb_gadget_driver *driver)
> +{
> +	struct crg_gadget_dev *crg_udc = gadget_to_udc(g);
> +	int ret;
> +
> +	/* The UDC core calls udc_set_speed() before udc_start(). */
> +	ret = crg_udc_program_speed(crg_udc);
> +	if (ret)
> +		return ret;
> +
> +	crg_udc->gadget_driver = driver;
> +	crg_udc_set_vbus_detect(crg_udc, true);
> +	return 0;
> +}
> +
> +static int crg_gadget_stop(struct usb_gadget *g)
> +{
> +	struct crg_gadget_dev *crg_udc;
> +	unsigned long flags = 0;
> +
> +	crg_udc = gadget_to_udc(g);
> +
> +	/*
> +	 * Clear gadget_driver first so crg_udc_handle_port_status() can no
> +	 * longer schedule udc_reset_work (it only does so while
> +	 * gadget_driver is set), then cancel_work_sync() is guaranteed to
> +	 * be the last word: no new reset notification can be queued after
> +	 * it returns.
> +	 */
> +	spin_lock_irqsave(&crg_udc->udc_lock, flags);
> +	crg_udc->gadget_driver = NULL;
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +
> +	cancel_work_sync(&crg_udc->udc_reset_work);
> +
> +	crg_udc->device_state = USB_STATE_ATTACHED;
> +	crg_udc_set_vbus_detect(crg_udc, false);
> +
> +	spin_lock_irqsave(&crg_udc->udc_lock, flags);

[Severity: Medium]
Can updating crg_udc->device_state and calling crg_udc_set_vbus_detect()
without holding crg_udc->udc_lock race with the threaded interrupt handler?

Before crg_gadget_stop() runs crg_udc_reset() and
crg_udc_reset_data_struct(), hardware interrupts remain enabled in
uccr->control (crg_gadget_pullup() only clears CRG_U3DC_CTRL_RUN, and sysfs
soft_connect_store() can invoke udc_stop() without synchronize_irq()).

If crg_gadget_handle_interrupt() is concurrently running under udc_lock in
crg_udc_handle_port_status() or crg_udc_handle_event(), setting
crg_udc->device_state = USB_STATE_ATTACHED outside udc_lock can race with the
state checks (such as crg_udc->device_state < USB_STATE_RECONNECTING),
causing crg_udc_handle_port_status() to prematurely call
crg_udc_enable_setup() or crg_udc_handle_event() to drop events. Similarly,
crg_gadget_start() updates config0, crg_udc->gadget_driver, and
CRG_UDC_VENDOR_REG without acquiring crg_udc->udc_lock.

Should these state and register updates in crg_gadget_stop() and
crg_gadget_start() be performed under crg_udc->udc_lock?

[ ... ]

> +static void crg_gadget_async_callbacks(struct usb_gadget *g, bool enable)
> +{
> +	struct crg_gadget_dev *crg_udc = gadget_to_udc(g);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&crg_udc->udc_lock, flags);
> +	crg_udc->async_callbacks = enable;
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +}

[ ... ]

> +static int crg_udc_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> +{

[ ... ]

> +	if (crg_udc_is_pointer_less_than(pause_pt, udc_req->first_trb,
> +					 udc_ep_ptr)) {
> +		crg_udc_squeeze_xfer_ring(udc_ep_ptr, udc_req);
> +		crg_udc_req_done(udc_ep_ptr, udc_req, -ECONNRESET);
> +	} else if (udc_req->last_trb &&
> +		   crg_udc_is_pointer_less_than(udc_req->last_trb, pause_pt,
> +						udc_ep_ptr)) {
> +		crg_udc_req_done(udc_ep_ptr, udc_req, -ECONNRESET);
> +	} else {
> +		struct crg_udc_request *next_req = NULL;
> +
> +		udc_req->usb_req.actual =
> +			crg_udc_actual_before(udc_ep_ptr, udc_req, pause_pt);
> +
> +		if (!list_is_last(&udc_req->queue, &udc_ep_ptr->queue))
> +			next_req = list_entry(udc_req->queue.next,
> +					      struct crg_udc_request, queue);
> +
> +		if (next_req && next_req->first_trb)
> +			udc_ep_ptr->deq_pt = next_req->first_trb;
> +		else
> +			udc_ep_ptr->deq_pt = udc_ep_ptr->enq_pt;
> +
> +		if (crg_udc_epcx_update_dqptr(udc_ep_ptr)) {
> +			spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +			return -EIO;
> +		}
> +		crg_udc_req_done(udc_ep_ptr, udc_req, -ECONNRESET);
> +	}
> +
> +	if (old_ep_state == EP_STATE_RUNNING) {
> +		udc_ep_ptr->ep_state = EP_STATE_RUNNING;
> +		crg_udc_knock_doorbell(crg_udc, dci);
> +	}
> +
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +	return 0;
> +}

[Severity: High]
When the active request is dequeued in the else branch above, advancing
udc_ep_ptr->deq_pt frees space on the transfer ring, but tran_ring_full is
never cleared and crg_udc_queue_pending_trbs() is not called. If
tran_ring_full was true and next_req has first_trb == NULL, won't setting
deq_pt = enq_pt while leaving tran_ring_full true permanently stall the
endpoint queue?

In addition, crg_udc_req_done() drops crg_udc->udc_lock while
udc_ep_ptr->ep_state is EP_STATE_STOPPED. When it returns, checking the saved
old_ep_state == EP_STATE_RUNNING unconditionally sets udc_ep_ptr->ep_state =
EP_STATE_RUNNING and rings the doorbell. If the endpoint was disabled or
halted while udc_lock was dropped during the completion callback, won't this
clobber the new endpoint state?

> +
> +/* endpoint operations */
> +
> +static int crg_udc_ep_disable(struct usb_ep *ep)
> +{

[ ... ]

> +	/*
> +	 * ep_enable is a write-1-to-disable register: endpoints are enabled
> +	 * via the CONFIG_EP command, but writing a set bit here disables the
> +	 * corresponding endpoint.
> +	 */
> +	writel(BIT(udc_ep->dci), &uccr->ep_enable);
> +
> +	udc_ep->desc = NULL;
> +	udc_ep->comp_desc = NULL;
> +
> +	/* clean up the request queue */
> +	crg_udc_ep_nuke(udc_ep, -ESHUTDOWN);
> +
> +	/* decrement ep counters */
> +	crg_udc->num_enabled_eps--;
> +	/* clean up the endpoint context */
> +	memset(p_ep_cx, 0, sizeof(struct crg_ep_cx));
> +
> +	/* If device state was changed to default by port
> +	 * reset, should not overwrite it again
> +	 */
> +	if (crg_udc->num_enabled_eps == 0 &&
> +	    crg_udc->device_state == USB_STATE_CONFIGURED) {
> +		crg_udc->device_state = USB_STATE_ADDRESS;
> +	}
> +
> +	udc_ep->ep_state = EP_STATE_DISABLED;
> +
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +
> +	return 0;
> +}

[Severity: High]
Does crg_udc_ep_disable() need to wait for the hardware to clear
BIT(udc_ep->dci) in uccr->ep_enable (similar to the readl_poll_timeout_atomic
in crg_udc_reinit()) before unmapping DMA buffers in crg_udc_ep_nuke() and
zeroing p_ep_cx?

Also, crg_udc_ep_nuke() calls crg_udc_req_done(), which drops and re-acquires
crg_udc->udc_lock around each request's completion callback. Because
udc_ep->ep_state, crg_udc->num_enabled_eps--, and memset(p_ep_cx, 0, ...) are
only updated after crg_udc_ep_nuke() returns:
- If a bus reset runs crg_udc_reinit() while udc_lock is dropped,
  crg_udc->num_enabled_eps is reset to 0, and crg_udc_ep_disable() will then
  underflow num_enabled_eps to 0xffffffff.
- Because udc_ep->ep_state is still EP_STATE_RUNNING during the callback, a
  concurrent re-enable or disable can race with the post-nuke memset(p_ep_cx)
  and udc_ep->ep_state = EP_STATE_DISABLED assignment.

Should the hardware disable poll, endpoint context cleanup, num_enabled_eps
decrement, and EP_STATE_DISABLED transition happen before calling
crg_udc_ep_nuke()?

> +
> +static int crg_udc_ep_enable(struct usb_ep *ep,
> +			     const struct usb_endpoint_descriptor *desc)
> +{

[ ... ]

> +	if (udc_ep->dci) {
> +		u32 ring_size = 0;
> +		size_t len;
> +
> +		if (usb_endpoint_xfer_bulk(desc))
> +			ring_size = CRG_UDC_BULK_EP_TD_RING_SIZE;
> +		else if (usb_endpoint_xfer_isoc(desc))
> +			ring_size = CRG_UDC_ISOC_EP_TD_RING_SIZE;
> +		else if (usb_endpoint_xfer_int(desc))
> +			ring_size = CRG_UDC_INT_EP_TD_RING_SIZE;
> +
> +		if (!ring_size) {
> +			spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +			return -EINVAL;
> +		}
> +		len = ring_size * sizeof(struct crg_transfer_trb);
> +
> +		if (udc_ep->tran_ring_info.cpu_addr &&
> +		    udc_ep->tran_ring_info.len != len) {
> +			void *old_vaddr = udc_ep->tran_ring_info.cpu_addr;
> +			dma_addr_t old_dma = udc_ep->tran_ring_info.dma_addr;
> +			size_t old_len = udc_ep->tran_ring_info.len;
> +
> +			udc_ep->tran_ring_info.cpu_addr = NULL;
> +			udc_ep->enq_pt = NULL;
> +			udc_ep->deq_pt = NULL;
> +
> +			spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +			dma_free_coherent(crg_udc->dev, old_len, old_vaddr,
> +					  old_dma);
> +			spin_lock_irqsave(&crg_udc->udc_lock, flags);
> +
> +			if (udc_ep->desc != desc ||
> +			    udc_ep->ep_state != EP_STATE_DISABLED) {
> +				spin_unlock_irqrestore(&crg_udc->udc_lock,
> +						       flags);
> +				return -EAGAIN;
> +			}
> +		}
> +
> +		/* setup transfer ring */
> +		if (!udc_ep->tran_ring_info.cpu_addr) {
> +			dma_addr_t dma;
> +			void *vaddr;
> +
> +			spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +			vaddr = dma_alloc_coherent(crg_udc->dev, len, &dma,
> +						   GFP_KERNEL);
> +			if (!vaddr)
> +				return -ENOMEM;
> +			spin_lock_irqsave(&crg_udc->udc_lock, flags);
> +			if (udc_ep->desc != desc ||
> +			    udc_ep->ep_state != EP_STATE_DISABLED) {
> +				spin_unlock_irqrestore(&crg_udc->udc_lock,
> +						       flags);
> +				dma_free_coherent(crg_udc->dev, len, vaddr,
> +						  dma);
> +				return -EAGAIN;
> +			}
> +			udc_ep->tran_ring_info.cpu_addr = vaddr;
> +			udc_ep->tran_ring_info.dma_addr = dma;
> +			udc_ep->tran_ring_info.len = len;
> +			udc_ep->first_trb = vaddr;
> +			udc_ep->last_trb = udc_ep->first_trb + ring_size - 1;
> +		}
> +		memset(udc_ep->first_trb, 0, udc_ep->tran_ring_info.len);
> +		/* Ensure that transfer ring is updated */
> +		dma_wmb();
> +		crg_udc_setup_link_trb(udc_ep->last_trb, true,
> +				       udc_ep->tran_ring_info.dma_addr);
> +		udc_ep->enq_pt = udc_ep->first_trb;
> +		udc_ep->deq_pt = udc_ep->first_trb;
> +		udc_ep->pcs = 1;
> +		udc_ep->tran_ring_full = false;
> +		crg_udc->num_enabled_eps++;
> +		status = crg_udc_epcx_setup(udc_ep);
> +		if (status)
> +			goto err_ep_enable;
> +	}
> +	param0 = BIT(udc_ep->dci);
> +	status = crg_udc_issue_command(crg_udc, CRG_CMD_CONFIG_EP, param0, 0);
> +	if (status)
> +		goto err_ep_enable;
> +	if (crg_udc->device_state == USB_STATE_ADDRESS)
> +		crg_udc->device_state = USB_STATE_CONFIGURED;
> +	udc_ep->wedge = false;
> +	udc_ep->usb_ep.enabled = 1;
> +	udc_ep->ep_state = EP_STATE_RUNNING;
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +	return 0;
> +
> +err_ep_enable:
> +	if (udc_ep->dci && crg_udc->num_enabled_eps)
> +		crg_udc->num_enabled_eps--;
> +	udc_ep->desc = NULL;
> +	udc_ep->comp_desc = NULL;
> +	udc_ep->ep_state = EP_STATE_DISABLED;
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +
> +	if (udc_ep->tran_ring_info.cpu_addr) {
> +		dma_free_coherent(crg_udc->dev, udc_ep->tran_ring_info.len,
> +				  udc_ep->tran_ring_info.cpu_addr,
> +				  udc_ep->tran_ring_info.dma_addr);
> +		udc_ep->tran_ring_info.cpu_addr = NULL;
> +		udc_ep->tran_ring_info.dma_addr = DMA_MAPPING_ERROR;
> +		udc_ep->tran_ring_info.len = 0;
> +		udc_ep->first_trb = NULL;
> +		udc_ep->last_trb = NULL;
> +	}
> +	return status;
> +}

[Severity: Critical]
Can the transfer ring resize and error paths in crg_udc_ep_enable() leave
dangling ring pointers or race on udc_ep->tran_ring_info?

1. When resizing an existing ring (udc_ep->tran_ring_info.len != len),
   udc_ep->tran_ring_info.cpu_addr, udc_ep->enq_pt, and udc_ep->deq_pt are
   set to NULL and old_vaddr is freed, but udc_ep->first_trb and
   udc_ep->last_trb still point to old_vaddr. If the subsequent
   dma_alloc_coherent() returns -ENOMEM (or the state re-check returns
   -EAGAIN, or !ring_size returns -EINVAL), crg_udc_ep_enable() returns
   without clearing udc_ep->desc, udc_ep->comp_desc, udc_ep->first_trb, or
   udc_ep->last_trb. A subsequent crg_udc_ep_queue() will see non-NULL
   udc_ep_ptr->first_trb and udc_ep_ptr->desc and dereference NULL
   udc_ep_ptr->enq_pt (or freed first_trb).

2. Under err_ep_enable, spin_unlock_irqrestore(&crg_udc->udc_lock, flags) is
   called before checking, freeing, and clearing udc_ep->tran_ring_info and
   first_trb/last_trb (and enq_pt/deq_pt are left non-NULL). Once udc_lock is
   dropped with udc_ep->ep_state == EP_STATE_DISABLED, can a concurrent
   crg_udc_ep_enable() or teardown path race on udc_ep->tran_ring_info.cpu_addr
   and double-free or use-after-free the coherent ring buffer?

[ ... ]

> +static int crg_udc_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
> +			    gfp_t gfp_flags)
> +{

[ ... ]

> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +
> +	crg_udc_clear_req_container(udc_req_ptr);
> +	udc_req_ptr->mapped = false;
> +
> +	if (_req->length) {
> +		udc_req_ptr->map_dir = dma_data_dir;
> +		status = usb_gadget_map_request_by_dev(
> +			crg_udc->dev, _req, dma_data_dir == DMA_TO_DEVICE);
> +		if (status)
> +			goto err_unmap;
> +
> +		if (is_sg) {
> +			if (!_req->num_mapped_sgs ||
> +			    crg_udc_sg_init(udc_req_ptr)) {
> +				status = -EIO;
> +				goto err_unmap;
> +			}
> +		} else if (_req->dma_mapped) {
> +			udc_req_ptr->mapped = true;
> +		}
> +	}
> +
> +	spin_lock_irqsave(&crg_udc->udc_lock, flags);
> +	if (udc_ep_ptr->desc != desc || !udc_req_ptr->usb_req.complete ||
> +	    !list_empty(&udc_req_ptr->queue) || crg_udc->resetting) {
> +		status = -ESHUTDOWN;
> +		spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +		crg_udc_unmap_request(udc_ep_ptr, udc_req_ptr);
> +		return status;
> +	}
> +
> +	_req->status = -EINPROGRESS;
> +	_req->actual = 0;
> +
> +	if (udc_ep_ptr->tran_ring_full)
> +		status = 0;
> +	else
> +		status = crg_udc_build_td(udc_ep_ptr, udc_req_ptr);
> +
> +	if (!status)
> +		list_add_tail(&udc_req_ptr->queue, &udc_ep_ptr->queue);
> +	spin_unlock_irqrestore(&crg_udc->udc_lock, flags);

[Severity: High]
When crg_udc_ep_queue() drops crg_udc->udc_lock to map the DMA buffer and then
re-acquires it, is checking udc_ep_ptr->desc != desc sufficient?

For EP0 (dci == 0), udc_ep_ptr->desc always points to &crg_udc->ep0_desc. If
a new SETUP packet arrives while udc_lock is dropped during DMA mapping,
crg_udc_abort_ep0_for_setup() and crg_udc_handle_setup_pkt() can complete and
start a new control transfer, yet udc_ep_ptr->desc != desc will still be
false. Without revalidating ep0_state, setup_status, and setup_tag after
re-acquiring udc_lock, can a stale EP0 request (potentially mapped with the
wrong DMA direction) be queued onto the new control transfer?

Similarly, for non-EP0 endpoints, crg_udc_ep_queue() never checks that
udc_ep_ptr->ep_state == EP_STATE_RUNNING (either before or after dropping
udc_lock), which allows requests to be queued while an endpoint is still
mid-enable in crg_udc_ep_enable() or after it was disabled and re-enabled with
the same descriptor pointer.

[ ... ]

> +static int crg_udc_epcx_setup(struct crg_udc_ep *udc_ep)
> +{

[ ... ]

> +	} else if ((crg_udc->gadget.speed == USB_SPEED_HIGH ||
> +		    crg_udc->gadget.speed == USB_SPEED_FULL) &&
> +		   (usb_endpoint_xfer_int(udc_ep->desc) ||
> +		    usb_endpoint_xfer_isoc(udc_ep->desc))) {
> +		if (crg_udc->gadget.speed == USB_SPEED_HIGH)
> +			maxburst = (usb_endpoint_maxp(desc) >> 11) & 0x3;
> +		if (maxburst == 0x3) {
> +			dev_err(crg_udc->dev, "invalid maxburst\n");
> +			maxburst = 0x2;
> +		}
> +	}

[Severity: High]
In crg_udc_epcx_setup(), usb_endpoint_maxp(desc) masks wMaxPacketSize with
USB_ENDPOINT_MAXP_MASK (0x07ff), stripping bits [15:11]. Doesn't shifting the
result right by 11 bits always evaluate to 0, leaving High-Speed
high-bandwidth isochronous and interrupt endpoints configured with
maxburst = 0 instead of usb_endpoint_maxp_mult(desc) - 1?

[ ... ]

> +static int crg_udc_epcx_update_dqptr(struct crg_udc_ep *udc_ep)
> +{
> +	struct crg_gadget_dev *crg_udc = udc_ep->crg_udc;
> +	u8 dci = udc_ep->dci;
> +	struct crg_ep_cx *epcx;
> +	u32 dw;
> +	dma_addr_t dqptaddr;
> +	u32 cmd_param0;
> +
> +	if (dci < 2 || dci >= CRG_NUM_EP_CX) {
> +		dev_err(crg_udc->dev, "invalid DCI %u for dequeue pointer\n", dci);
> +		return -EINVAL;
> +	}
> +
> +	epcx = &crg_udc->p_epcx[dci - 2];
> +	dqptaddr = crg_udc_tran_trb_virt_to_dma(udc_ep, udc_ep->deq_pt);
> +
> +	if (dqptaddr == DMA_MAPPING_ERROR)
> +		return -EINVAL;
> +
> +	/* fill ep_dw2 */
> +	dw = lower_32_bits(dqptaddr) & EP_CX_TR_DQPT_LO_MASK;
> +	FIELD_MODIFY(EP_CX_DEQ_CYC_STATE_MASK, &dw, udc_ep->pcs);
> +	epcx->dw2 = cpu_to_le32(dw);

[Severity: High]
Does crg_udc_epcx_update_dqptr() program the wrong Dequeue Cycle State (DCS)
when udc_ep->enq_pt has wrapped past the Link TRB?

udc_ep->pcs tracks the producer cycle state at udc_ep->enq_pt and is toggled
whenever enq_pt wraps to udc_ep->first_trb in crg_udc_queue_trbs(). When
udc_ep->deq_pt != udc_ep->enq_pt and enq_pt has wrapped while deq_pt has not
yet wrapped (deq_pt > enq_pt), the TRBs at udc_ep->deq_pt have cycle bit
udc_ep->pcs ^ 1, not udc_ep->pcs. Programming EP_CX_DEQ_CYC_STATE_MASK with
udc_ep->pcs in crg_udc_ep_dequeue() or crg_udc_ep_halt() will cause the
controller to see a cycle bit mismatch at deq_pt and stall the endpoint ring.

[ ... ]

> +static int crg_udc_queue_trbs(struct crg_udc_ep *udc_ep_ptr,
> +			      struct crg_udc_request *udc_req_ptr, bool b_isoc,
> +			      u32 xfer_ring_size, u32 num_trbs_needed,
> +			      u64 buffer_length)
> +{
> +	struct crg_gadget_dev *crg_udc = udc_ep_ptr->crg_udc;
> +	struct crg_transfer_trb *p_xfer_ring = udc_ep_ptr->first_trb;
> +	struct crg_transfer_trb *enq_pt = udc_ep_ptr->enq_pt;
> +	struct usb_request *usb_req = &udc_req_ptr->usb_req;
> +	u32 num_trbs_ava, count, i;
> +	u32 td_trbs;
> +	u32 td_size;
> +	u32 j = 1;
> +	u32 intr_rate = b_isoc ? CRG_UDC_ISOC_EP_INTERRUPT_RATE :
> +				 CRG_UDC_BULK_EP_INTERRUPT_RATE;
> +	bool full_td = true;
> +	bool need_zlp = false;
> +	bool is_sg = usb_req->num_mapped_sgs;
> +
> +	if (!b_isoc && usb_req->zero && usb_req->length &&
> +	    !(usb_req->length % udc_ep_ptr->usb_ep.maxpacket))
> +		need_zlp = true;
> +
> +	if (!num_trbs_needed)
> +		return -EINVAL;
> +
> +	/* TD_SIZE is five bits and encodes the number of TRBs remaining
> +	 * in this TD minus one. A request may contain multiple TDs.
> +	 */
> +	td_trbs = min_t(u32, num_trbs_needed, 32);
> +
> +	num_trbs_ava = crg_udc_room_on_ring(crg_udc, xfer_ring_size,
> +					    p_xfer_ring, udc_ep_ptr->enq_pt,
> +					    udc_ep_ptr->deq_pt);
> +
> +	if (num_trbs_ava < td_trbs) {
> +		if (b_isoc)
> +			return -EBUSY;
> +		trace_crg_udc_no_room_on_ring(
> +			"deferring TRBs to next doorbell");
> +		count = num_trbs_ava;
> +		full_td = false;
> +		udc_ep_ptr->tran_ring_full = true;
> +		need_zlp = false;
> +	} else {
> +		count = td_trbs;
> +	}

[ ... ]

> +		if (b_isoc) {
> +			crg_udc_setup_trb(
> +				enq_pt, trb_len, trb_buf_addr, td_size,
> +				i == 0 ? (original_pcs ^ 1) : udc_ep_ptr->pcs,
> +				TRB_TYPE_XFER_DATA_ISOCH, short_pkt, chain_bit,
> +				ioc, false, 0, true, 0, 1, 0);
> +		} else {
> +			bool azp = ioc && !chain_bit && need_zlp;
> +
> +			crg_udc_setup_trb(
> +				enq_pt, trb_len, trb_buf_addr, td_size,
> +				i == 0 ? (original_pcs ^ 1) : udc_ep_ptr->pcs,
> +				TRB_TYPE_XFER_NORMAL, short_pkt, chain_bit, ioc,
> +				false, 0, false, 0, 0, azp);
> +		}

[Severity: Critical]
How are requests requiring more than 32 TRBs (or queued behind a full ring)
handled across crg_udc_queue_trbs(), crg_udc_ep_queue(), and
crg_udc_handle_xfer_event()?

1. When num_trbs_needed > 32 and num_trbs_ava >= 32, td_trbs is capped at 32,
   so buffer_length is still non-zero at the 32nd TRB (i == count - 1), but
   need_zlp is only cleared when num_trbs_ava < td_trbs. Won't
   azp = ioc && !chain_bit && need_zlp set TRB_APPEND_ZLP_MASK on the 32nd TRB
   in the middle of the transfer, sending a premature zero-length packet
   before the remaining TRBs of the request are queued?

2. In that same case (num_trbs_needed > 32 and num_trbs_ava > 32),
   crg_udc_queue_trbs() queues the first 32 TRBs and leaves
   udc_req_ptr->all_trbs_queued = false and udc_ep_ptr->tran_ring_full =
   false. If a second request is then submitted via crg_udc_ep_queue(), it
   checks only if (udc_ep_ptr->tran_ring_full) and immediately calls
   crg_udc_build_td() for the second request, interleaving the second
   request's TRBs onto the hardware ring ahead of the remaining TRBs of the
   first request.

3. In crg_udc_handle_xfer_event() at queue_more_trbs, udc_ep_ptr->tran_ring_full
   is cleared to false before calling crg_udc_queue_pending_trbs(). Because
   crg_udc_queue_pending_trbs() breaks out of its loop and returns 0 after
   building a single TD for the first pending request, if that TD does not
   fill the ring, tran_ring_full stays false even when later requests in
   udc_ep_ptr->queue still have !all_trbs_queued (first_trb == NULL), leaving
   them unscheduled on subsequent completions and allowing newly queued
   requests in crg_udc_ep_queue() to bypass them.

> +
> +		trace_crg_udc_queue_trb(udc_ep_ptr, enq_pt);
> +
> +		if (is_sg && !crg_udc_sg_advance(udc_req_ptr, trb_len))
> +			return -EINVAL;
> +
> +		td_size--;
> +		enq_pt++;
> +		j++;
> +
> +		if (enq_pt == udc_ep_ptr->last_trb) {
> +			dma_wmb();
> +			le32p_replace_bits(&enq_pt->dw3, udc_ep_ptr->pcs,
> +					   TRB_CYCLE_BIT_MASK);
> +			udc_ep_ptr->pcs ^= 1;
> +			enq_pt = udc_ep_ptr->first_trb;
> +		}
> +	}
> +
> +	dma_wmb();
> +	le32p_replace_bits(&udc_ep_ptr->enq_pt->dw3, original_pcs,
> +			   TRB_CYCLE_BIT_MASK);
> +
> +	if (!udc_req_ptr->trbs_needed)
> +		udc_req_ptr->first_trb = udc_ep_ptr->enq_pt;
> +
> +	udc_ep_ptr->enq_pt = enq_pt;
> +	if (count == num_trbs_ava)
> +		udc_ep_ptr->tran_ring_full = true;
> +	udc_req_ptr->buff_len_left = buffer_length;
> +	if (buffer_length)
> +		udc_req_ptr->trbs_needed = num_trbs_needed - count;
> +	else
> +		udc_req_ptr->trbs_needed = 0;
> +
> +	if (!udc_req_ptr->buff_len_left) {
> +		if (udc_ep_ptr->enq_pt == udc_req_ptr->first_trb)
> +			udc_req_ptr->last_trb = udc_ep_ptr->last_trb - 1;
> +		else
> +			udc_req_ptr->last_trb = udc_ep_ptr->enq_pt - 1;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
There appear to be two issues with how udc_req_ptr->last_trb is computed here:

1. When the final TRB of a request is placed at udc_ep_ptr->last_trb - 1, the
   loop increments enq_pt to udc_ep_ptr->last_trb and wraps it to
   udc_ep_ptr->first_trb. However, the check at the end compares:

	if (udc_ep_ptr->enq_pt == udc_req_ptr->first_trb)

   instead of udc_ep_ptr->first_trb (unlike crg_udc_queue_ctrl() in
   crg-udc-ep0.c). For any request that did not start at udc_ep_ptr->first_trb,
   this condition is false on wrap and udc_req_ptr->last_trb is set to
   udc_ep_ptr->first_trb - 1 (pointing out-of-bounds before the start of the
   ring buffer).

2. udc_req_ptr->last_trb is only assigned when !udc_req_ptr->buff_len_left.
   When a request is split across multiple TDs (because num_trbs_needed > 32
   or num_trbs_ava < td_trbs), buff_len_left is non-zero and
   udc_req_ptr->last_trb remains NULL while the first TD is in flight. When
   the completion event for the last TRB of that TD (at count - 1 > 0, so
   trb_pt != udc_req->first_trb) arrives in crg_udc_handle_xfer_event(),
   is_request_dequeued() sees udc_req->last_trb == NULL,
   crg_udc_is_pointer_less_than(trb_pt, NULL, udc_ep) returns false, and
   is_request_dequeued() returns true. crg_udc_handle_xfer_event() then jumps
   to queue_more_trbs and skips the !udc_req_ptr->all_trbs_queued continuation
   path, leaving the transfer hung.

[ ... ]

> +void crg_udc_reset_work(struct work_struct *work)
> +{
> +	struct crg_gadget_dev *crg_udc =
> +		container_of(work, struct crg_gadget_dev, udc_reset_work);
> +
> +	if (crg_udc->gadget_driver)
> +		usb_gadget_udc_reset(&crg_udc->gadget, crg_udc->gadget_driver);
> +}

[Severity: Critical]
Can crg_udc_reset_work() dereference a NULL crg_udc->gadget_driver or call
driver->reset() after usb_gadget_disable_async_callbacks()?

1. crg_udc_reset_work() reads crg_udc->gadget_driver twice without holding
   crg_udc->udc_lock. If crg_gadget_stop() sets crg_udc->gadget_driver = NULL
   on another CPU right after the if (crg_udc->gadget_driver) check,
   usb_gadget_udc_reset() can be passed NULL and dereference driver->reset().

2. During unbind, the UDC core calls usb_gadget_disable_async_callbacks()
   (which calls crg_gadget_async_callbacks(g, false)), synchronize_irq(), and
   driver->unbind() before calling usb_gadget_udc_stop_locked()
   (crg_gadget_stop()). Because crg_udc_handle_port_status() and
   crg_udc_reset_work() do not check crg_udc->async_callbacks and
   crg_gadget_async_callbacks() does not cancel udc_reset_work, can
   udc_reset_work run concurrently with or after driver->unbind()?

> +
> +int crg_udc_handle_port_status(struct crg_gadget_dev *crg_udc,
> +			       unsigned long *flags)
> +{

[ ... ]

> +	/* handle Port Connection Change */
> +	tmp = readl(&uccr->portsc);
> +	if ((tmp & (CRG_U3DC_PORTSC_CCS | CRG_U3DC_PORTSC_PP)) ==
> +	    (CRG_U3DC_PORTSC_CCS | CRG_U3DC_PORTSC_PP)) {
> +		if (FIELD_GET(CRG_U3DC_PORTSC_PLS_MASK, tmp) != 0 ||
> +		    (!(tmp & CRG_U3DC_PORTSC_PED)))
> +			return 0;

[ ... ]

> +	} else if (!(tmp & CRG_U3DC_PORTSC_CCS)) {
> +		bool cable_connected;
> +		int ccs_drop_ignore = 0;
> +
> +		if ((FIELD_GET(CRG_U3DC_PORTSC_PLS_MASK, tmp) == 0x0) &&
> +		    (FIELD_GET(CRG_U3DC_PORTSC_SPEED_MASK, tmp) <
> +		     CRG_U3DC_PORTSC_SPEED_SS)) {
> +			ccs_drop_ignore = 1;
> +			dev_err(crg_udc->dev, "ccs glitch detect on HS/FS!\n");
> +		}
> +
> +		if (!ccs_drop_ignore)
> +			crg_udc->gadget.speed = USB_SPEED_UNKNOWN;
> +
> +		cable_connected = crg_udc_vbus_connected(crg_udc);
> +		if (cable_connected && !ccs_drop_ignore) {
> +			crg_udc->device_state = USB_STATE_POWERED;
> +			crg_udc_reinit(crg_udc);
> +			crg_udc->connected = 0;
> +		} else if (!cable_connected) {
> +			dev_dbg(crg_udc->dev,
> +				"cable disconnected, rst controller\n");
> +			if (was_connected)
> +				crg_udc_notify_disconnect(crg_udc, flags);
> +			if (crg_udc_reset(crg_udc, flags))
> +				dev_err(crg_udc->dev,
> +					"controller reset failed on disconnect\n");
> +			crg_udc_reset_data_struct(crg_udc);
> +			crg_udc->connected = 0;
> +			crg_udc_init_ep0(crg_udc);
> +
> +			crg_udc->device_state = USB_STATE_ATTACHED;
> +			return -ECONNRESET;
> +		}
> +	}
> +	/* handle Port Connection Change end*/
> +
> +	if (portsc_val & CRG_U3DC_PORTSC_PLC) {
> +		tmp = readl(&uccr->portsc);
> +		if (FIELD_GET(CRG_U3DC_PORTSC_PLS_MASK, tmp) == 0xf) {
> +			FIELD_MODIFY(CRG_U3DC_PORTSC_PLS_MASK, &tmp, 0x0);
> +			writel(tmp, &uccr->portsc);
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
There seem to be several issues in crg_udc_handle_port_status():

1. The Port Connection Change block runs unconditionally even when
   CRG_U3DC_PORTSC_CSC is not set in portsc_val, and returns 0 whenever
   (tmp & (CRG_U3DC_PORTSC_CCS | CRG_U3DC_PORTSC_PP)) ==
   (CRG_U3DC_PORTSC_CCS | CRG_U3DC_PORTSC_PP) and PLS != 0. When a Port Link
   State Change (CRG_U3DC_PORTSC_PLC) event arrives with PLS == 0xf (Resume)
   on a connected port, PLS != 0 is true, so the function returns 0 early and
   never reaches the if (portsc_val & CRG_U3DC_PORTSC_PLC) block below.

2. In the CRG_U3DC_PORTSC_PLC block, writing PLS = 0x0 to &uccr->portsc does
   not set CRG_U3DC_PORTSC_LWS (Link Write Strobe) and does not mask out
   CRG_U3DC_PORTSC_W1C_MASK (unlike crg_gadget_wakeup()). Without LWS, will
   the hardware ignore the PLS write while clearing any newly asserted W1C
   bits in portsc?

3. In the !(tmp & CRG_U3DC_PORTSC_CCS) branch when cable_connected &&
   !ccs_drop_ignore is true:
   - crg_udc->device_state is set to USB_STATE_POWERED, but the immediate call
     to crg_udc_reinit(crg_udc) overwrites crg_udc->device_state with
     USB_STATE_RECONNECTING.
   - crg_udc->connected is set to 0 without calling
     crg_udc_notify_disconnect(). If a cable unplug drops CCS before VBUS
     (CRG_U3DC_PORTSC_PP) decays, crg_udc->connected becomes 0 on the first
     event, so when VBUS subsequently drops and enters else if
     (!cable_connected), was_connected is false and
     crg_udc_notify_disconnect() is never called.

> +
> +int crg_udc_handle_xfer_event(struct crg_gadget_dev *crg_udc,
> +			      struct crg_event_trb *event, unsigned long *flags)
> +{

[ ... ]

> +			if (udc_req_ptr->all_trbs_queued) {
> +				udc_req_ptr->usb_req.actual =
> +					udc_req_ptr->usb_req.length -
> +					udc_req_ptr->buff_len_left;
> +				residual = FIELD_GET(EVE_TRB_TRAN_LEN_MASK,
> +						     le32_to_cpu(event->dw2));
> +				if (residual > udc_req_ptr->usb_req.actual)
> +					residual = 0;
> +				udc_req_ptr->usb_req.actual -= residual;
> +				crg_udc_req_done(udc_ep_ptr, udc_req_ptr, 0);
> +
> +				if (dci == 0 && udc_ep_ptr->desc &&
> +				    usb_endpoint_xfer_control(udc_ep_ptr->desc))
> +					crg_udc_ep0_req_complete(udc_ep_ptr,
> +								 flags);

[Severity: High]
For a standard USB_REQ_GET_STATUS request on EP0, getstatusrequest() in
crg-udc-ep0.c sets udc_req_ptr->usb_req.complete = get_status_cmpl and
crg_udc->setup_status = DATA_STAGE_XFER.

When the data stage completes here, crg_udc_req_done() runs get_status_cmpl(),
which sets crg_udc->setup_status = STATUS_STAGE_RECV and calls
crg_udc_build_ep0_status() to queue the status stage TRB. Immediately after
crg_udc_req_done() returns, crg_udc_ep0_req_complete() is called; because
setup_status is already STATUS_STAGE_RECV instead of DATA_STAGE_XFER,
crg_udc_ep0_req_complete() hits its default case and resets
crg_udc->setup_status = WAIT_FOR_SETUP while the status stage TRB is still in
flight.

> +			} else {
> +				int ret;
> +
> +				ret = crg_udc_build_td(udc_ep_ptr, udc_req_ptr);
> +				if (ret) {
> +					udc_req_ptr->usb_req.actual =
> +						udc_req_ptr->usb_req.length -
> +						udc_req_ptr->buff_len_left;
> +					crg_udc_req_done(udc_ep_ptr, udc_req_ptr,
> +							 -EIO);
> +				} else {
> +					crg_udc_knock_doorbell(crg_udc, dci);
> +				}
> +			}
> +		}
> +		break;
> +
> +	case CMPL_CODE_SHORT_PKT:
> +		if (list_empty(&udc_ep_ptr->queue))
> +			break;
> +
> +		udc_req_ptr = list_first_entry(&udc_ep_ptr->queue,
> +					       struct crg_udc_request, queue);
> +		udc_req_ptr->usb_req.actual = udc_req_ptr->usb_req.length -
> +					      udc_req_ptr->buff_len_left;
> +		residual = FIELD_GET(EVE_TRB_TRAN_LEN_MASK,
> +				     le32_to_cpu(event->dw2));
> +		if (residual > udc_req_ptr->usb_req.actual)
> +			residual = 0;
> +		udc_req_ptr->usb_req.actual -= residual;
> +
> +		if (udc_req_ptr->usb_req.short_not_ok &&
> +		    usb_endpoint_dir_out(udc_ep_ptr->desc))
> +			crg_udc_req_done(udc_ep_ptr, udc_req_ptr, -EREMOTEIO);
> +		else
> +			crg_udc_req_done(udc_ep_ptr, udc_req_ptr, 0);
> +
> +		trbs_dequeued = true;
> +		crg_udc_advance_dequeue_pt(udc_ep_ptr);
> +		break;

[Severity: High]
Can CMPL_CODE_SHORT_PKT cause EP0 control OUT transfers to hang and miscalculate
usb_req.actual on multi-TRB transfers?

1. crg_udc_setup_datastage_trb() sets TRB_INTR_ON_SHORT_PKT_MASK on EP0 data
   stage TRBs. If a host sends a short packet during a control OUT data stage
   (DATA_STAGE_RECV), completion arrives with CMPL_CODE_SHORT_PKT, which
   completes the request via crg_udc_req_done() but never calls
   crg_udc_ep0_req_complete(udc_ep_ptr, flags) when dci == 0, so the status
   stage TRB is never queued.

2. EVE_TRB_TRAN_LEN_MASK in event->dw2 is the residual byte count of the
   single TRB that short-terminated, whereas
   udc_req_ptr->usb_req.length - udc_req_ptr->buff_len_left is the total byte
   count of all TRBs queued in the TD. If an OUT request spans multiple TRBs
   (for example, a scatter-gather list or a buffer crossing a 64 KB boundary)
   and short-terminates on an earlier TRB, subtracting only that TRB's
   residual counts all subsequent unexecuted TRBs in the TD as received.

3. Calling crg_udc_req_done() before crg_udc_advance_dequeue_pt(udc_ep_ptr)
   means udc_lock is dropped for the completion callback while udc_ep_ptr->deq_pt
   is still stale; if the callback queues a new request, advancing deq_pt
   afterwards can corrupt the dequeue pointer. (A similar ordering issue
   exists in crg_udc_ep_halt() and set_ep_halt(), where crg_udc_ep_nuke()
   drops udc_lock before udc_ep_ptr->deq_pt = udc_ep_ptr->enq_pt and
   udc_ep_ptr->ep_state = EP_STATE_HALTED are set.)

[ ... ]

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