Re: [PATCH v2 2/2] usb: gadget: udc: add Axiado AX3000 and AX3005 UDC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 22 potential issue(s) to consider:
- [Critical] 500ms busy-wait with local interrupts disabled during hardware reset.
- [High] UDC driver automatically queues STATUS TRBs, conflicting with Gadget API and causing double STATUS TRBs.
- [High] Hardware dequeue pointer is not advanced past a canceled request's TRBs in `crg_udc_ep_dequeue`.
- [High] Multi-TRB Transfer Descriptors are exposed to the hardware prematurely due to immediate Cycle bit updates.
- [High] Large requests requiring more than 32 TRBs will silently hang and never complete.
- [High] TRBs from multiple USB requests can be interleaved on the transfer ring, corrupting data streams.
- [High] Transfer ring size mismatch leading to DMA ring corruption.
- [High] Failure to detect error returns from `*_virt_to_dma` functions results in hardware being programmed with DMA address 0.
- [High] Truncation of 64-bit DMA addresses on 32-bit architectures leads to out-of-bounds TRB accesses.
- [High] `crg_udc_ep_nuke()` will infinite loop if a gadget driver resubmits requests from its completion callback.
- [High] Undeclared identifier `kzalloc_obj` will cause build failures.
- [High] UAF in interrupt handler due to devm_request_threaded_irq and missing disable_irq.
- [High] Ring buffer capacity calculation allows the ring to become completely full, making it indistinguishable from empty.
- [High] Endpoint stall due to incorrect cycle state (pcs) after squeezing transfer ring.
- [High] Lock drop in `crg_udc_ep_dequeue` allows concurrent TRB queueing, causing a fatal hardware state mismatch.
- [High] Read-modify-write on hardware registers in `crg_gadget_pullup` lacks lock protection.
- [High] Failure to clear endpoint descriptors and queues during controller reset allows stale requests to persist across sessions.
- [Medium] Incorrect `actual` byte count calculation for short packets on split requests.
- [Medium] VBUS presence detection reads software-controlled enable bit.
- [Medium] Stuck endpoint and leaked request on deferred queueing error.
- [Medium] Event Ring Dequeue Pointer (ERDP) is not updated before yielding the CPU, risking Event Ring Full errors.
- [Low] Unused loop variable in `ep_halt` causes redundant hardware commands.
--

--- Patch [2]: [PATCH v2 2/2] usb: gadget: udc: add Axiado AX3000 and AX3005 UDC driver ---
commit 3443453e5e9e5d73494d04cff64d7d014eadf93b
Author: Naveen Kumar Rajgiri Bassappa <rbnaveenkumar@axiado.com>

usb: gadget: udc: add Axiado AX3000 and AX3005 UDC driver

This commit introduces a new USB device controller driver for the Axiado
AX3000 and AX3005 SoCs. It utilizes a Corigine USB 3.1 IP core and an
xHCI-like programming model. The driver supports High-Speed and SuperSpeed
operations, various transfer types, and standard endpoint operations.

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

[ ... ]

> +static struct transfer_trb_s *tran_trb_dma_to_virt(struct crg_udc_ep *udc_ep,
> +						   dma_addr_t address)
> +{
> +	unsigned long offset;
> +	struct transfer_trb_s *trb_virt;
> +
> +	if (lower_32_bits(address) & 0xf)
> +		return NULL;
> +
> +	offset = address - udc_ep->tran_ring_info.dma_addr;

[Severity: High]
Does this cause truncation on 32-bit architectures? The offset variable is
typed unsigned long instead of u64 or dma_addr_t, which could lead to
out-of-bounds TRB accesses when computing the address difference.

[ ... ]

> +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 ep_cx_s *epcx = &crg_udc->p_epcx[dci - 2];
> +	u32 dw;
> +	dma_addr_t dqptaddr;
> +	u32 cmd_param0;
> +
> +	if (dci == 0) {
> +		dev_err(crg_udc->dev, "cannot update dqptr for ep0\n");
> +		return -EINVAL;
> +	}
> +
> +	dqptaddr = tran_trb_virt_to_dma(udc_ep, udc_ep->deq_pt);
> +
> +	if (dma_mapping_error(crg_udc->dev, dqptaddr))

[Severity: High]
Is dma_mapping_error() the correct check here? The tran_trb_virt_to_dma()
routine returns 0 on error, which dma_mapping_error() might not catch,
potentially resulting in the hardware being programmed with DMA address 0.

[ ... ]

> +static void crg_udc_ep_nuke(struct crg_udc_ep *udc_ep, int status,
> +			    unsigned long *flags)
> +{
> +	struct crg_udc_request *req = NULL;
> +
> +	while (!list_empty(&udc_ep->queue)) {
> +		req = list_entry(udc_ep->queue.next, struct crg_udc_request,
> +				 queue);
> +
> +		req_done(udc_ep, req, status, flags);
> +	}
> +}

[Severity: High]
Could this loop indefinitely if a gadget driver resubmits requests from its
completion callback? Since req_done() drops the lock and invokes the
completion routine, new requests might be added to the queue if
crg_udc_ep_queue() doesn't reject them during teardown.

[ ... ]

> +static u32 room_on_ring(struct crg_gadget_dev *crg_udc, u32 num_trbs,
> +			struct transfer_trb_s *p_ring,
> +			struct transfer_trb_s *enq_pt,
> +			struct transfer_trb_s *dq_pt)
> +{
> +	u32 i = 0;
> +
> +	if (enq_pt == dq_pt) {
> +		/* ring is empty */
> +		return num_trbs - 1;
> +	}

[Severity: High]
Is it intentional to return num_trbs - 1 for an empty ring? This capacity
calculation allows the ring to become completely full, making it
indistinguishable from being empty.

[ ... ]

> +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 transfer_trb_s *p_xfer_ring = udc_ep_ptr->first_trb;
> +	struct transfer_trb_s *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 ? ISOC_EP_INTERRUPT_RATE :
> +		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);

[Severity: High]
Will large requests requiring more than 32 TRBs complete successfully? The
function clamps td_trbs to 32 and returns success without enqueuing the
rest of the required TRBs.

[ ... ]

> +		if (b_isoc) {
> +			setup_trb(enq_pt, trb_len, trb_buf_addr, td_size,
> +				  udc_ep_ptr->pcs, TRB_TYPE_XFER_DATA_ISOCH,
> +				  short_pkt, chain_bit, ioc, false, 0, true, 0, 1, 0);
> +		} else {
> +			u8 azp = ioc && !chain_bit && need_zlp;
> +
> +			setup_trb(enq_pt, trb_len, trb_buf_addr, td_size,
> +				  udc_ep_ptr->pcs, TRB_TYPE_XFER_NORMAL, short_pkt,
> +				  chain_bit, ioc, false, 0, false, 0, 0, azp);
> +		}

[Severity: High]
Are multi-TRB transfer descriptors exposed to the hardware prematurely? The
cycle bit is set immediately on every TRB via setup_trb() inside this loop,
which might allow the controller to execute incomplete descriptors.

[ ... ]

> +static int crg_udc_queue_ctrl(struct crg_udc_ep *udc_ep_ptr,
> +			      struct crg_udc_request *udc_req_ptr,
> +			      u32 num_of_trbs_needed)
> +{
> +	struct crg_gadget_dev *crg_udc = udc_ep_ptr->crg_udc;
> +	enum ep_state ep_state;
> +	struct transfer_trb_s *enq_pt = udc_ep_ptr->enq_pt;
> +	struct transfer_trb_s *dq_pt = udc_ep_ptr->deq_pt;

[ ... ]

> +			for (i = 0; i < num_of_trbs_needed; i++) {
> +				p_trb = enq_pt;
> +				transfer_length = crg_trb_chunk_len(trb_dma, remaining);
> +				ioc = (i == num_of_trbs_needed - 1);
> +				azp = ioc && need_zlp;
> +				setup_datastage_trb(p_trb, trb_dma,
> +						    udc_ep_ptr->pcs, transfer_length,
> +						    num_of_trbs_needed - i - 1,
> +						    ioc, azp, dir, setup_tag);

[Severity: High]
Similarly, is the cycle bit set prematurely on these control TRBs before
the entire chain is ready?

[ ... ]

> +static void ep0_req_complete(struct crg_udc_ep *udc_ep_ptr,
> +			     unsigned long *flags)
> +{
> +	struct crg_gadget_dev *crg_udc = udc_ep_ptr->crg_udc;
> +
> +	switch (crg_udc->setup_status) {
> +	case DATA_STAGE_XFER:
> +		crg_udc->setup_status = STATUS_STAGE_RECV;
> +		build_ep0_status(udc_ep_ptr, true, -EINPROGRESS, NULL, 0, 0);
> +		break;
> +	case DATA_STAGE_RECV:
> +		crg_udc->setup_status = STATUS_STAGE_XFER;
> +		build_ep0_status(udc_ep_ptr, true, -EINPROGRESS, NULL, 0, 0);
> +		break;

[Severity: High]
Does this conflict with the Gadget API? The driver appears to automatically
queue status TRBs for control transfers, which might cause double status
TRBs if the gadget driver also queues its own status response.

[ ... ]

> +static int queue_pending_trbs(struct crg_udc_ep *udc_ep_ptr)
> +{
> +	struct crg_udc_request *udc_req_ptr;
> +	int ret;
> +
> +	list_for_each_entry(udc_req_ptr, &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) {
> +			dev_err(udc_ep_ptr->crg_udc->dev,
> +				"failed to build pending transfer: %d\n", ret);
> +			return ret;
> +		}

[Severity: Medium]
What happens to udc_req_ptr if crg_udc_build_td() fails here? Returning
directly propagates the error without removing the failed request from the
queue or completing it, potentially leaving the endpoint stuck.

[ ... ]

> +static void squeeze_xfer_ring(struct crg_udc_ep *udc_ep_ptr,
> +			      struct crg_udc_request *udc_req_ptr)
> +{
> +	struct transfer_trb_s *trb = udc_req_ptr->first_trb;
> +	struct crg_udc_request *next_req;
> +
> +	if (!trb)
> +		return;
> +
> +	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;
> +	}

[Severity: High]
Does wrapping trb from last_trb to first_trb require updating the endpoint's
pcs cycle state? Failing to toggle the cycle state could cause the endpoint
to stall due to a mismatch between software and hardware.

[ ... ]

> +static int ep_halt(struct crg_udc_ep *udc_ep_ptr, int halt, int ignore_wedge,
> +		   unsigned long *flags)
> +{

[ ... ]

> +		if (!list_empty(&udc_ep_ptr->queue)) {
> +			tmp = udc_ep_ptr->dci;
> +			tmp = FIELD_PREP(CRG_U3DC_DB_TARGET_MASK, tmp);
> +
> +			list_for_each_entry(udc_req_ptr, &udc_ep_ptr->queue,
> +					    queue) {
> +				struct transfer_trb_s *tmp_ptr =
> +					udc_ep_ptr->deq_pt + 1;

[Severity: Low]
Is tmp_ptr evaluated correctly? The loop body ignores udc_req_ptr and
redundantly evaluates deq_pt + 1 over and over, potentially causing
unnecessary hardware commands.

[ ... ]

> +static int crg_udc_ep_enable(struct usb_ep *ep,
> +			     const struct usb_endpoint_descriptor *desc)
> +{
> +	struct crg_udc_ep *udc_ep;
> +	struct crg_gadget_dev *crg_udc;
> +	u32 param0;
> +	int status;
> +	unsigned long flags = 0;

[ ... ]

> +	if (udc_ep->dci) {
> +		/* setup transfer ring */
> +		if (!udc_ep->tran_ring_info.cpu_addr) {
> +			dma_addr_t dma;
> +			u32 ring_size = 0;

[Severity: High]
If cpu_addr is non-NULL, does this skip reallocation without verifying that
the existing allocation size matches the new descriptor's requirements? This
might lead to DMA ring corruption if the new configuration requires a larger
ring.

[ ... ]

> +static struct usb_request *crg_udc_alloc_request(struct usb_ep *_ep,
> +						 gfp_t gfp_flags)
> +{
> +	struct crg_udc_request *udc_req_ptr;
> +
> +	udc_req_ptr = kzalloc_obj(*udc_req_ptr, gfp_flags);

[Severity: High]
Is kzalloc_obj defined anywhere? Using this undeclared identifier will cause
build failures.

[ ... ]

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

[ ... ]

> +	if (usb_endpoint_xfer_control(udc_ep_ptr->desc) && !_req->length) {
> +		crg_udc->setup_status = STATUS_STAGE_XFER;
> +		build_ep0_status(&crg_udc->udc_ep[0], false, -EINPROGRESS,
> +				 udc_req_ptr, 0, 0);
> +		spin_unlock_irqrestore(&crg_udc->udc_lock, flags);
> +		return 0;
> +	}

[Severity: High]
Is this hardcoded interception of 0-length requests overriding the direction
to TX? This may conflict with standard gadget zero-length responses.

[ ... ]

> +	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);

[Severity: High]
Could TRBs from multiple USB requests be interleaved on the transfer ring?
If the endpoint queue has partially queued requests and tran_ring_full is
false, new TRBs might be appended to the ring before older requests finish
queueing, corrupting data streams.

[ ... ]

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

[ ... ]

> +	} else {
> +		udc_req->usb_req.actual =
> +			crg_udc_actual_before(udc_ep_ptr, udc_req, pause_pt);
> +		req_done(udc_ep_ptr, udc_req, -ECONNRESET, &flags);
> +		udc_ep_ptr->deq_pt = pause_pt;
> +		if (crg_udc_epcx_update_dqptr(udc_ep_ptr)) {

[Severity: High]
req_done() drops the spinlock. Does this create a race condition where
concurrent TRB queueing modifies the state before crg_udc_epcx_update_dqptr()
is called?

[Severity: High]
Additionally, is setting the dequeue pointer to pause_pt correct? This does
not advance the hardware dequeue pointer past the canceled request's TRBs,
causing the hardware to resume executing canceled TRBs.

[ ... ]

> +static int crg_udc_reset(struct crg_gadget_dev *crg_udc, unsigned long *flags)
> +{
> +	struct crg_uccr __iomem *uccr = crg_udc->uccr;
> +	u32 i, tmp;
> +	struct crg_udc_ep *udc_ep_ptr;
> +	u32 val;
> +
> +	val = readl(&uccr->control);
> +	val |= CRG_U3DC_CTRL_SWRST;
> +	writel(val, &uccr->control);
> +	/* Poll until SWRST clears; hardware reset takes up to 500 ms */
> +	if (readl_poll_timeout_atomic(&uccr->control, tmp,
> +				      !(tmp & CRG_U3DC_CTRL_SWRST), 1000,
> +				      500000)) {
> +		dev_err(crg_udc->dev, "reset error\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: Critical]
Is a 500ms busy-wait safe here? The atomic poll macro is invoked with a
500000us timeout while the udc_lock is held and local interrupts are
disabled in crg_gadget_handle_interrupt(), which could cause CPU starvation
and hard lockups.

[ ... ]

> +	/* Complete any reqs on EP1-EP15 queue */
> +	for (i = 2; i < CRG_NUM_EP_CX; i++) {
> +		udc_ep_ptr = &crg_udc->udc_ep[i];
> +
> +		if (udc_ep_ptr->desc)
> +			crg_udc_ep_nuke(udc_ep_ptr, -ESHUTDOWN, flags);
> +		udc_ep_ptr->tran_ring_full = false;
> +		udc_ep_ptr->ep_state = EP_STATE_DISABLED;
> +	}

[Severity: High]
Should udc_ep_ptr->desc be set to NULL here? Failing to clear the descriptor
leaves the endpoint logically open, which may allow stale requests to persist
across sessions since crg_udc_ep_enable() doesn't reinitialize the list.

[ ... ]

> +static bool crg_udc_vbus_connected(struct crg_gadget_dev *crg_udc)
> +{
> +	return !!(readl(crg_udc->mmio_virt_base + CRG_UDC_VENDOR_REG) &
> +		  CRG_UDC_VBUS_DETECT);
> +}

[Severity: Medium]
Is CRG_UDC_VBUS_DETECT a software-controlled enable bit? In
crg_vbus_detect(), this same bit is written to enable detection, meaning
this function might just be reading the software override rather than the
actual hardware status.

[ ... ]

> +static int crg_gadget_pullup(struct usb_gadget *g, int is_on)
> +{
> +	struct crg_gadget_dev *crg_udc = gadget_to_udc(g);
> +	struct crg_uccr __iomem *uccr = crg_udc->uccr;
> +	u32 val;
> +
> +	if (is_on) {
> +		/* Enable event reporting and interrupts, then assert RUN */
> +		val = readl(&uccr->config1);
> +		val |= (CRG_U3DC_CFG1_CSC_EVENT_EN |
> +			CRG_U3DC_CFG1_PEC_EVENT_EN |
> +			CRG_U3DC_CFG1_PPC_EVENT_EN |
> +			CRG_U3DC_CFG1_PRC_EVENT_EN |
> +			CRG_U3DC_CFG1_PLC_EVENT_EN |
> +			CRG_U3DC_CFG1_CEC_EVENT_EN);
> +		writel(val, &uccr->config1);

[Severity: High]
Are these read-modify-write operations on shared hardware control registers
safe without lock protection?

[ ... ]

> +static int crg_handle_xfer_event(struct crg_gadget_dev *crg_udc,
> +				 struct event_trb_s *event,
> +				 unsigned long *flags)
> +{

[ ... ]

> +	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);
> +		residual = FIELD_GET(EVE_TRB_TRAN_LEN_MASK,
> +				     le32_to_cpu(event->dw2));
> +		if (residual > udc_req_ptr->usb_req.length)
> +			residual = 0;
> +		udc_req_ptr->usb_req.actual =
> +			udc_req_ptr->usb_req.length - residual;

[Severity: Medium]
Does this correctly compute the actual byte count for short packets on
split requests? Using length - residual without considering earlier completed
segments or TRBs will yield an incorrect actual count.

[ ... ]

> +static int process_event_ring(struct crg_gadget_dev *crg_udc, int index,
> +			      unsigned long *flags)
> +{

[ ... ]

> +		if (++processed % CRG_EVENT_RING_BATCH == 0) {
> +			spin_unlock_irqrestore(&crg_udc->udc_lock, *flags);
> +			cond_resched();
> +			spin_lock_irqsave(&crg_udc->udc_lock, *flags);
> +		}
> +	}
> +
> +	/* update dequeue pointer */
> +	erdp = event_trb_virt_to_dma(udc_event, udc_event->evt_dq_pt);
> +	if (dma_mapping_error(crg_udc->dev, erdp))
> +		return -EINVAL;
> +	writel(upper_32_bits(erdp), &uicr->erdphi);
> +	writel(lower_32_bits(erdp) | CRG_U3DC_ERDPLO_EHB, &uicr->erdplo);

[Severity: Medium]
If cond_resched() is called, the lock is dropped and the CPU yielded without
updating the ERDP register. Could this cause Event Ring Full errors if new
events arrive while yielded?

[ ... ]

> +static void crg_udc_free_resources(struct crg_gadget_dev *crg_udc)
> +{
> +	int i;
> +
> +	if (crg_udc->uccr) {
> +		u32 val = readl(&crg_udc->uccr->control);
> +
> +		val &= ~(CRG_U3DC_CTRL_INT_EN | CRG_U3DC_CTRL_RUN);
> +		writel(val, &crg_udc->uccr->control);
> +	}
> +	if (crg_udc->irq >= 0)
> +		synchronize_irq(crg_udc->irq);
> +
> +	for (i = 0; i < CRG_RING_NUM; i++) {
> +		struct crg_udc_event *udc_event = &crg_udc->udc_event[i];
> +
> +		if (udc_event->event_ring.cpu_addr) {
> +			dma_free_coherent(crg_udc->dev,

[Severity: High]
Could this cause a use-after-free in the interrupt handler? This frees DMA
memory while the devm-managed shared IRQ is still active and capable of
scheduling the thread, as synchronize_irq() does not disable the IRQ.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-axiado-ax3000-usb-device-controller-v2-0-0ef033dd0a68@axiado.com?part=2