Re: [PATCH v4 3/3] usb: cdns3: cdnsp: Enable eUSB2v2 1KB bulk packet capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: cdns3: TRB logic uses incorrect 512-byte maxp for 1024-byte eUSB2v2 endpoints
- [High] usb: cdns3: Missing hardware capability check for eUSB2v2 MPS update
--

commit 5efa1eec93f4abee65d86f70cca3c1177b04dfda
Author: Pawel Laszczak <pawell@cadence.com>

usb: cdns3: cdnsp: Enable eUSB2v2 1KB bulk packet capability

This commit implements peripheral-side changes in the Cadence (cdnsp) driver
to support eUSB2v2 1024-byte Bulk MaxPacketSize. It checks for eUSB2v2
capability in the HCCPARAMS2 register, delegates bulk max packet update
requests to the composite layer, and dynamically configures the hardware
endpoint for a 1024-byte packet size when the feature is active.

> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c
> index 5cd9b898ce971..090d20b307ee2 100644
> --- a/drivers/usb/cdns3/cdnsp-ep0.c
> +++ b/drivers/usb/cdns3/cdnsp-ep0.c
> @@ -253,6 +253,8 @@ static int cdnsp_ep0_handle_feature_device(struct cdnsp_device *pdev,
>  		 */
>  		cdnsp_enter_test_mode(pdev);
>  		break;
> +	case USB_DEVICE_BULK_MAX_PACKET_UPDATE:
> +		return cdnsp_ep0_delegate_req(pdev, ctrl);

[Severity: High]
Does this code need to verify that the controller is actually eUSB2v2 capable
(pdev->gadget.is_eusb2v2) before delegating the request?

The composite gadget framework unconditionally sets gadget->eusb2v2_mps_active = 1
when it receives USB_DEVICE_BULK_MAX_PACKET_UPDATE without checking
gadget->is_eusb2v2:

drivers/usb/gadget/composite.c:composite_setup() {
    ...
        case USB_DEVICE_BULK_MAX_PACKET_UPDATE:
            if (ctrl->bRequestType != (USB_DIR_OUT | USB_RECIP_DEVICE))
                goto unknown;
            if (ctrl->bRequest != USB_REQ_SET_FEATURE)
                goto unknown;
            gadget->eusb2v2_mps_active = 1;
    ...
}

If a malicious or improperly configured USB host sends this request to a
non-eUSB2v2 capable device, this delegation allows the flag to be set, leading
to cdnsp_endpoint_init() configuring a 1024-byte maximum packet size on
unsupported hardware.

>  	default:
>  		return -EINVAL;
>  	}

[ ... ]

> diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c
> index 419309c8439e3..a499887ed5618 100644
> --- a/drivers/usb/cdns3/cdnsp-mem.c
> +++ b/drivers/usb/cdns3/cdnsp-mem.c
> @@ -976,8 +976,13 @@ int cdnsp_endpoint_init(struct cdnsp_device *pdev,
>  	if (!usb_endpoint_xfer_isoc(pep->endpoint.desc))
>  		err_count = 3;
>  	if (usb_endpoint_xfer_bulk(pep->endpoint.desc) &&
> -	    pdev->gadget.speed == USB_SPEED_HIGH)
> -		max_packet = 512;
> +	    pdev->gadget.speed == USB_SPEED_HIGH) {
> +		if (pdev->gadget.eusb2v2_mps_active)
> +			max_packet = 1024;

[Severity: High]
Does the TRB generation logic in drivers/usb/cdns3/cdnsp-ring.c need to be
updated to handle the 1024-byte max packet size?

While the hardware endpoint context is correctly programmed to 1024 bytes here,
functions in cdnsp-ring.c still rely on usb_endpoint_maxp(ep->desc), which
evaluates to 512 bytes.

For example, calculating ZLP boundaries:

drivers/usb/cdns3/cdnsp-ring.c:cdnsp_queue_bulk_tx() {
    ...
    if (preq->request.zero && preq->request.length &&
        IS_ALIGNED(full_len, usb_endpoint_maxp(pep->endpoint.desc))) {
        need_zero_pkt = true;
        num_trbs++;
    }
    ...
}

Calculating bounce buffer alignment:

drivers/usb/cdns3/cdnsp-ring.c:cdnsp_align_td() {
    ...
    max_pkt = usb_endpoint_maxp(preq->pep->endpoint.desc);
    unalign = (enqd_len + *trb_buff_len) % max_pkt;
    ...
}

Calculating TRB TD size:

drivers/usb/cdns3/cdnsp-ring.c:cdnsp_td_remainder() {
    ...
    maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
    total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
    ...
}

This causes the driver to incorrectly calculate ZLP boundaries on 512-byte
boundaries instead of 1024, fragment TRBs into 512-byte bounce buffers, and
provide wrong TRB_TD_SIZE metrics to the hardware scheduler.

> +		else
> +			max_packet = 512;
> +	}
> +
>  	/* Controller spec indicates that ctrl ep avg TRB Length should be 8. */
>  	if (usb_endpoint_xfer_control(pep->endpoint.desc))
>  		avg_trb_len = 8;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-eusb2v2-packet-size-v4-0-a505ee17b460@cadence.com?part=3