casaroli commented on code in PR #21513:
URL: https://github.com/apache/nuttx/pull/21513#discussion_r4496251160
##########
drivers/usbdev/usbdev_fs.c:
##########
@@ -236,11 +238,25 @@ static int usbdev_fs_submit_wrreq(FAR struct usbdev_ep_s
*ep,
****************************************************************************/
static int usbdev_fs_submit_rdreq(FAR struct usbdev_ep_s *ep,
- FAR struct usbdev_fs_req_s *container)
+ FAR struct usbdev_fs_req_s *container,
+ size_t want)
{
+ FAR struct usbdev_fs_ep_s *fs_ep = (FAR struct usbdev_fs_ep_s *)ep->fs;
FAR struct usbdev_req_s *req = container->req;
+ size_t pending = 0;
+ uint16_t i;
+
+ for (i = 0; i < fs_ep->reqnum; i++)
+ {
+ if (&fs_ep->reqbuffer[i] != container)
+ {
+ pending += fs_ep->reqbuffer[i].req->len;
+ }
+ }
- req->len = ep->maxpacket;
+ want = ALIGN_UP(want, ep->maxpacket);
+ req->len = want > pending ? MIN(want - pending, fs_ep->reqsize) :
Review Comment:
With USBDEV_FS_BULK_REQLEN=0 the request size still changes when `reqsize`
is larger than `maxpacket`. A USBDEV_DUALSPEED build that enumerates at full
speed has `reqsize` = hssize (512) and `maxpacket` = 64, so an OUT request
grows from 1 packet to up to 8 packets (the write at line 817 too). A
USBDEV_SUPERSPEED build gets sssize times the burst. The PR body says the
default keeps one maxpacket per request.
Fix: limit the request to `ep->maxpacket` instead of `fs_ep->reqsize` when
CONFIG_USBDEV_FS_BULK_REQLEN is 0, here and at line 817.
##########
drivers/usbdev/Kconfig:
##########
@@ -1833,6 +1833,15 @@ config USBDEV_FS_NEP0REQS
---help---
The number of ep0 requests that can be in flight
+config USBDEV_FS_BULK_REQLEN
Review Comment:
The option has no upper limit. With CONFIG_USBDEV_DMA, usbdev_allocreq()
gets the buffer from the DCD `allocbuffer(ep, uint16_t nbytes)`, so a value of
65536 or more allocates a truncated buffer while `req->len` keeps the full
size, and the DCD writes past the buffer.
Fix: add `range 0 32768`.
##########
drivers/usbdev/usbdev_fs.c:
##########
@@ -1044,29 +1060,35 @@ static int usbdev_fs_ep_bind(FAR struct usbdev_s *dev,
uint8_t epno,
{
if (epinfo->compdesc.mxburst >= USB_SS_BULK_EP_MAXBURST)
{
- reqsize = reqsize * USB_SS_BULK_EP_MAXBURST;
+ fs_ep->reqsize *= USB_SS_BULK_EP_MAXBURST;
}
else
{
- reqsize = reqsize * (epinfo->compdesc.mxburst + 1);
+ fs_ep->reqsize *= (epinfo->compdesc.mxburst + 1);
}
}
else if (transtpye == USB_EP_ATTR_XFER_INT)
{
if (epinfo->compdesc.mxburst >= USB_SS_INT_EP_MAXBURST)
{
- reqsize = reqsize * USB_SS_INT_EP_MAXBURST;
+ fs_ep->reqsize *= USB_SS_INT_EP_MAXBURST;
}
else
{
- reqsize = reqsize * (epinfo->compdesc.mxburst + 1);
+ fs_ep->reqsize *= (epinfo->compdesc.mxburst + 1);
}
}
}
#endif
fs_ep->ep->fs = fs_ep;
+#if CONFIG_USBDEV_FS_BULK_REQLEN > 0
+ fs_ep->reqsize = ALIGN_UP(CONFIG_USBDEV_FS_BULK_REQLEN, fs_ep->reqsize);
Review Comment:
This also sizes the ep0 requests and the interrupt endpoint requests (for
example the MTP event endpoint), not only bulk. With 16384, ep0 gets
CONFIG_USBDEV_FS_NEP0REQS (default 4) buffers of more than 16 KiB each for
control data of a few bytes.
Fix: round up only bulk endpoints: `if (epno != 0 && (epinfo->desc.attr &
USB_EP_ATTR_XFERTYPE_MASK) == USB_EP_ATTR_XFER_BULK)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
