[U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor

[Sherry Sun]

Hi Lukasz,

> 
> On Thu, 22 Aug 2019 01:46:20 +0000
> Sherry Sun <sherry.sun at nxp.com> wrote:
> 
> > Because the buffer length of sdp usb request is 65, we have to
> > allocate 65 bytes not 64 bytes. Otherwise there is potential buffer
> > overflow.
> >
> > So the wMaxPacketSize of fullspeed can't meet the needs. Add HS
> > endpoint descriptor for SDP. Then we can use high speed endpoint, and
> > the SDP device can send packet with 512 byte size.
> >
> > Signed-off-by: Sherry Sun <sherry.sun at nxp.com>
> > Signed-off-by: Ye Li <ye.li at nxp.com>
> > ---
> >  drivers/usb/gadget/f_sdp.c | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> > index 841814bc07..8aaed92e9b 100644
> > --- a/drivers/usb/gadget/f_sdp.c
> > +++ b/drivers/usb/gadget/f_sdp.c
> > @@ -158,6 +158,16 @@ static struct usb_endpoint_descriptor in_desc = {
> >  	.bInterval =		1,
> >  };
> >
> > +static struct usb_endpoint_descriptor in_hs_desc = {
> > +	.bLength =		USB_DT_ENDPOINT_SIZE,
> > +	.bDescriptorType =	USB_DT_ENDPOINT,
> > /*USB_DT_CS_ENDPOINT*/ +
> > +	.bEndpointAddress =	1 | USB_DIR_IN,
> > +	.bmAttributes =	USB_ENDPOINT_XFER_INT,
> > +	.wMaxPacketSize =	512,
> > +	.bInterval =		1,
> > +};
> > +
> >  static struct usb_descriptor_header *sdp_runtime_descs[] = {
> >  	(struct usb_descriptor_header *)&sdp_intf_runtime,
> >  	(struct usb_descriptor_header *)&sdp_hid_desc, @@ -165,6 +175,13
> @@
> > static struct usb_descriptor_header *sdp_runtime_descs[] = { NULL,  };
> >
> > +static struct usb_descriptor_header *sdp_runtime_hs_descs[] = {
> > +	(struct usb_descriptor_header *)&sdp_intf_runtime,
> > +	(struct usb_descriptor_header *)&sdp_hid_desc,
> > +	(struct usb_descriptor_header *)&in_hs_desc,
> > +	NULL,
> > +};
> > +
> >  /* This is synchronized with what the SoC implementation reports */
> > static struct hid_report sdp_hid_report = {
> >  	.usage_page = {
> > @@ -490,6 +507,11 @@ static int sdp_bind(struct usb_configuration *c,
> > struct usb_function *f) goto error;
> >  	}
> >
> > +	if (gadget_is_dualspeed(gadget)) {
> > +		/* Assume endpoint addresses are the same for both
> > speeds */
> > +		in_hs_desc.bEndpointAddress =
> > in_desc.bEndpointAddress;
> > +	}
> > +
> >  	sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
> >
> >  	cdev->req->context = sdp;
> > @@ -527,7 +549,7 @@ static struct usb_request *sdp_start_ep(struct
> > usb_ep *ep) {
> >  	struct usb_request *req;
> >
> > -	req = alloc_ep_req(ep, 64);
> > +	req = alloc_ep_req(ep, 65);
> 
> Maybe it would be good to have the #define for this magic number (65)?
> 
> If normally we have 64, then it is pretty clear. With 65 we may need #define
> with some extra comment in the code.

Okay, I will add this.

> 
> >  	debug("%s: ep:%p req:%p\n", __func__, ep, req);
> >
> >  	if (!req)
> > @@ -542,11 +564,15 @@ static int sdp_set_alt(struct usb_function *f,
> > unsigned intf, unsigned alt) {
> >  	struct f_sdp *sdp = func_to_sdp(f);
> >  	struct usb_composite_dev *cdev = f->config->cdev;
> > +	struct usb_gadget *gadget = cdev->gadget;
> >  	int result;
> >
> >  	debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
> >
> > -	result = usb_ep_enable(sdp->in_ep, &in_desc);
> > +	if (gadget_is_dualspeed(gadget) && gadget->speed ==
> > USB_SPEED_HIGH)
> > +		result = usb_ep_enable(sdp->in_ep, &in_hs_desc);
> > +	else
> > +		result = usb_ep_enable(sdp->in_ep, &in_desc);
> >  	if (result)
> >  		return result;
> >  	sdp->in_req = sdp_start_ep(sdp->in_ep); @@ -592,7 +618,7 @@
> static
> > int sdp_bind_config(struct usb_configuration *c) memset(sdp_func, 0,
> > sizeof(*sdp_func));
> >
> >  	sdp_func->usb_function.name = "sdp";
> > -	sdp_func->usb_function.hs_descriptors = sdp_runtime_descs;
> > +	sdp_func->usb_function.hs_descriptors = sdp_runtime_hs_descs;
> >  	sdp_func->usb_function.descriptors = sdp_runtime_descs;
> >  	sdp_func->usb_function.bind = sdp_bind;
> >  	sdp_func->usb_function.unbind = sdp_unbind; @@ -725,6 +751,7
> @@
> > static void sdp_handle_in_ep(struct spl_image_info *spl_image) /* In
> > SPL, allow jumps to U-Boot images */
> >  			struct spl_image_info spl_image = {};
> >  			spl_parse_image_header(&spl_image, header);
> > +
> 
> Minor:
> 
> Please remove this white space.

Thanks for your reminder, I will do it.

Best regards
Sherry sun

> 
> >  			jump_to_image_no_args(&spl_image);
> >  #else
> >  			/* In U-Boot, allow jumps to scripts */
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de