| Message ID | 20240912045150.915573-1-xu.yang_2@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] usb: chipidea: udc: limit usb request length to max 16KB | expand |
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
On Fri, Sep 13, 2024 at 09:20:45AM +0800, Peter Chen wrote: > 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? No. It's decided by hw IP design. This old design may not suit current requirements. > > > 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. This 1st patch is mainly used to serve the 2nd patch which may impact both IN and OUT eps. Because it's hard to judge whether a request is suit for transfer if it spans more dTDs. So it's needed for both eps. I've looked through all function drivers, all of them use 16KB as max request size. If one future function driver does use a larger request size, they can adjust it according to this error log too. > > > > > 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. Okay, I can limit the impact to only imx platform. Thanks, Xu Yang
On 24-09-13 15:11:33, Xu Yang wrote: > On Fri, Sep 13, 2024 at 09:20:45AM +0800, Peter Chen wrote: > > 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? > > No. It's decided by hw IP design. This old design may not suit current > requirements. > > > > > > 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. > > This 1st patch is mainly used to serve the 2nd patch which may impact > both IN and OUT eps. ... > Because it's hard to judge whether a request is > suit for transfer if it spans more dTDs. So it's needed for both eps. Sorry, I do not understand you above words. First, you may know this request is for IN or OUT, second, according to TD size and data buffer address, you may know you use one or more dTDs. Peter > I've looked through all function drivers, all of them use 16KB as max > request size. If one future function driver does use a larger request > size, they can adjust it according to this error log too. > > > > > > > > > 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. > > Okay, I can limit the impact to only imx platform. > > Thanks, > Xu Yang
On Fri, Sep 13, 2024 at 05:53:44PM +0800, Peter Chen wrote: > On 24-09-13 15:11:33, Xu Yang wrote: > > On Fri, Sep 13, 2024 at 09:20:45AM +0800, Peter Chen wrote: > > > 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? > > > > No. It's decided by hw IP design. This old design may not suit current > > requirements. > > > > > > > > > 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. > > > > This 1st patch is mainly used to serve the 2nd patch which may impact > > both IN and OUT eps. > ... > > Because it's hard to judge whether a request is > > suit for transfer if it spans more dTDs. So it's needed for both eps. > > Sorry, I do not understand you above words. First, you may know this > request is for IN or OUT, second, according to TD size and data buffer > address, you may know you use one or more dTDs. If req.num_sgs = 0, then we can know how many TDs need to transfer data. For example: req.buf = 0xA0001800 req.length = 40KB - TD1 addr:0xA0001800 size:18KB - TD2 addr:0xA0017000 size:20KB - TD3 addr:0xA002D000 size:2KB We basically won't meet issue for non-sg case. The only expection is that received short packet on TD1 (or TD2). Then the next data packet will go to TD2. But it should go to TD1 of next request. But if num_sgs > 0, we need to check validity of each sg entry due to above limitations. For example: req.num_sgs = 3 req.length = 40KB - sg1.addr = 0xA0001800 length = 18KB -> TD1 - sg2.addr = 0xA0016000 length = 20KB -> TD2 - sg3.addr = 0xA0028800 length = 2KB -> TD3 This request can be safty used to transfer data. But we can also meet previous short packet issue. req.num_sgs = 5 req.length = 10B + 20KB - sg1.addr = 0xA0001800 length = 10B -> TD1 - sg2.addr = 0xA0016000 length = 6KB -> TD2 - sg3.addr = 0xA0028800 length = 6KB -> TD3 - sg4.addr = 0xA003A000 length = 4KB -> TD3 - sg5.addr = 0xA004C000 length = 4KB -> TD3 This request can't be used to transfer data since sg1 + sg2 can't form a data packet. The host will see a short packet (100 bytes). req.num_sgs = 5 req.length = 20KB + 10B - sg1.addr = 0xA0001800 length = 2KB -> TD1 - sg2.addr = 0xA0016400 length = 5KB -> TD2 - sg3.addr = 0xA0028800 length = 8KB -> TD3 - sg4.addr = 0xA003A800 length = 5KB -> TD4 - sg5.addr = 0xA004C200 length = 10B -> TD5 Compared to previous request, it need 5 TDs even though req.length are same. Most of the sg entries can't share same TD since their address is not page aligned. For high-speed isoc eps, sg1 + sg2 can't form a 3KB DATA2 + DATA1 + DATA0 data sequence too. Therefore, it's a bit complicated to validate request if num_sgs > 0, especially when req.length is larger than 16KB (1 TD size). When add such condition, each of the sg entry must follow below requirements: 1. the end address of the first sg buffer must be 4KB aligned. 2. the start and end address of the middle sg buffer must be 4KB aligned. 3. the start address of the last sg buffer must be 4KB aligned. So it will be more easy to validate the request. Hope this will help you understand the motivation of 1st patch. Thanks, Xu Yang
On 24-09-13 23:25:13, Xu Yang wrote: > On Fri, Sep 13, 2024 at 05:53:44PM +0800, Peter Chen wrote: > > On 24-09-13 15:11:33, Xu Yang wrote: > > > On Fri, Sep 13, 2024 at 09:20:45AM +0800, Peter Chen wrote: > > > > 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? > > > > > > No. It's decided by hw IP design. This old design may not suit current > > > requirements. > > > > > > > > > > > > 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. > > > > > > This 1st patch is mainly used to serve the 2nd patch which may impact > > > both IN and OUT eps. > > ... > > > Because it's hard to judge whether a request is > > > suit for transfer if it spans more dTDs. So it's needed for both eps. > > > > Sorry, I do not understand you above words. First, you may know this > > request is for IN or OUT, second, according to TD size and data buffer > > address, you may know you use one or more dTDs. > > If req.num_sgs = 0, then we can know how many TDs need to transfer data. > > For example: > req.buf = 0xA0001800 req.length = 40KB > > - TD1 addr:0xA0001800 size:18KB > - TD2 addr:0xA0017000 size:20KB > - TD3 addr:0xA002D000 size:2KB > > We basically won't meet issue for non-sg case. The only expection is that > received short packet on TD1 (or TD2). Then the next data packet will go > to TD2. But it should go to TD1 of next request. > > But if num_sgs > 0, we need to check validity of each sg entry due to above > limitations. > > For example: > req.num_sgs = 3 req.length = 40KB > > - sg1.addr = 0xA0001800 length = 18KB -> TD1 > - sg2.addr = 0xA0016000 length = 20KB -> TD2 > - sg3.addr = 0xA0028800 length = 2KB -> TD3 > > This request can be safty used to transfer data. But we can also meet > previous short packet issue. > > req.num_sgs = 5 req.length = 10B + 20KB > > - sg1.addr = 0xA0001800 length = 10B -> TD1 > - sg2.addr = 0xA0016000 length = 6KB -> TD2 > - sg3.addr = 0xA0028800 length = 6KB -> TD3 > - sg4.addr = 0xA003A000 length = 4KB -> TD3 > - sg5.addr = 0xA004C000 length = 4KB -> TD3 > With your the 2nd patch, you could make end of sg1.addr is PAGE aligned, in that case, the sg1 and sg2 could be at the one TD. sg1 is at the first dTD, and sg2 at the 2nd & 3rd dTD. If that could be done, the host may not see short packet, anyway, you could confirm through analyser. Peter > This request can't be used to transfer data since sg1 + sg2 can't > form a data packet. The host will see a short packet (100 bytes). > > req.num_sgs = 5 req.length = 20KB + 10B > > - sg1.addr = 0xA0001800 length = 2KB -> TD1 > - sg2.addr = 0xA0016400 length = 5KB -> TD2 > - sg3.addr = 0xA0028800 length = 8KB -> TD3 > - sg4.addr = 0xA003A800 length = 5KB -> TD4 > - sg5.addr = 0xA004C200 length = 10B -> TD5 > > Compared to previous request, it need 5 TDs even though req.length > are same. Most of the sg entries can't share same TD since their > address is not page aligned. For high-speed isoc eps, sg1 + sg2 can't > form a 3KB DATA2 + DATA1 + DATA0 data sequence too. > > Therefore, it's a bit complicated to validate request if num_sgs > 0, > especially when req.length is larger than 16KB (1 TD size). > > When add such condition, each of the sg entry must follow below > requirements: > 1. the end address of the first sg buffer must be 4KB aligned. > 2. the start and end address of the middle sg buffer must be 4KB aligned. > 3. the start address of the last sg buffer must be 4KB aligned. > > So it will be more easy to validate the request. > > Hope this will help you understand the motivation of 1st patch. > > Thanks, > Xu Yang
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; + } + /* first nuke then test link, e.g. previous status has not sent */ if (!list_empty(&hwreq->queue)) { dev_err(hwep->ci->dev, "request already in queue\n");
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. 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. 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(+)