Re: [PATCH 1/2] usb: chipidea: udc: limit usb request length to max 16KB

Peter Chen
2024-09-13T01:20:53Z

On 24-09-12 12:51:49, Xu Yang wrote:
> Currently, the deivice controller has below limitations:
> 1. can't generate short packet interrupt if IOC not set in dTD. So if one
> request span more than one dTDs and only the last dTD set IOC, the usb
> request will pending there if no more data comes.
> 2. the controller can't accurately deliver data to differtent usb requests
> in some cases due to short packet. For example: one usb request span 3
> dTDs, then if the controller received a short packet the next packet
> will go to 2nd dTD of current request rather than the first dTD of next
> request.
>

Are there any IP errata for it?

> To let the device controller work properly, one usb request should only
> correspond to one dTD. Then every dTD will set IOC. In theory, each dTD
> support up to 20KB data transfer if the offset is 0. Due to we cannot
> predetermine the offset, this will limit the usb request length to max
> 16KB. This should be fine since most of the user transfer data based on
> this size policy.
>
> Although these limitations found on OUT eps, we can put the request to IN
> eps too, this will benefit the following patches.

Since IN endpoints have not found the problem, please limit the changes
only for OUT endpoints.

>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
> drivers/usb/chipidea/ci.h | 1 +
> drivers/usb/chipidea/udc.c | 5 +++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 2a38e1eb6546..f8deccfc8713 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -25,6 +25,7 @@
> #define TD_PAGE_COUNT 5
> #define CI_HDRC_PAGE_SIZE 4096ul /* page size for TD's */
> #define ENDPT_MAX 32
> +#define CI_MAX_REQ_SIZE (4 * CI_HDRC_PAGE_SIZE)
> #define CI_MAX_BUF_SIZE (TD_PAGE_COUNT * CI_HDRC_PAGE_SIZE)
>
> /******************************************************************************
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index b1a1be6439b6..5d9369d01065 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -969,6 +969,11 @@ static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
> return -EMSGSIZE;
> }
>
> + if (hwreq->req.length > CI_MAX_REQ_SIZE) {
> + dev_err(hwep->ci->dev, "request length too big (max 16KB)\n");
> + return -EMSGSIZE;
> + }
> +

Since this IP is used by many vendors, it may fix by others.
I prefer add flag like CI_HDRC_SHORT_PACKET_NOT_SUPPORT,
and set in imx platform file.

Peter