[U-Boot,3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor
diff mbox series

Message ID 1563414056-20331-4-git-send-email-sherry.sun@nxp.com
State Superseded
Delegated to: Stefano Babic
Headers show
Series
  • Make some changes to SDP
Related show

Commit Message

Sherry Sun July 18, 2019, 1:40 a.m. UTC
From: Sherry Sun <sherry.sun@nxp.com>

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@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
---
 drivers/usb/gadget/f_sdp.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

Comments

Lukasz Majewski July 18, 2019, 7:32 a.m. UTC | #1
Hi Sherry,

> From: Sherry Sun <sherry.sun@nxp.com>
> 
> Because the buffer length of sdp usb request is 65, 

Is the value 65 mentioned somewhere (in any doc/spec)?

As fair as I remember other protocols - like DFU use 64B. Do we need
here to store extra \0 or \n ?

The 64 bytes are default packet size on EP0.


If I may ask - what user space program do you use? imx_loader (imx_usb)
or uuu ?

> 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@nxp.com>
> Signed-off-by: Ye Li <ye.li@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 b1601575e0..532041063a 100644
> --- a/drivers/usb/gadget/f_sdp.c
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -157,6 +157,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,

Which endpoint do we use in SDP? EP0 or EP1...n ?

> +};
> +
>  static struct usb_descriptor_header *sdp_runtime_descs[] = {
>  	(struct usb_descriptor_header *)&sdp_intf_runtime,
>  	(struct usb_descriptor_header *)&sdp_hid_desc,
> @@ -164,6 +174,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 = {
> @@ -489,6 +506,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;
> @@ -526,7 +548,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);
>  	debug("%s: ep:%p req:%p\n", __func__, ep, req);
>  
>  	if (!req)
> @@ -541,11 +563,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);
> @@ -591,7 +617,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;
> @@ -724,6 +750,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);
> +
>  			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@denx.de
Sherry Sun July 18, 2019, 8:56 a.m. UTC | #2
Hi, Lukasz

> 
> Hi Sherry,
> 
> > From: Sherry Sun <sherry.sun@nxp.com>
> >
> > Because the buffer length of sdp usb request is 65,
> 
> Is the value 65 mentioned somewhere (in any doc/spec)?
> 
> As fair as I remember other protocols - like DFU use 64B. Do we need here to
> store extra \0 or \n ?
> 

You can see case SDP_STATE_TX_STATUS and case SDP_STATE_TX_REGISTER in
static void sdp_handle_in_ep(struct spl_image_info *spl_image)
Here sdp_func->in_req->length = 65; 

Data[0] is the report id, and the next 64 bytes are the date to host.
So totally we need 65 bytes.

> The 64 bytes are default packet size on EP0.
> 
> 
> If I may ask - what user space program do you use? imx_loader (imx_usb) or
> uuu ?
> 

Yes, I use uuu.


> > 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@nxp.com>
> > Signed-off-by: Ye Li <ye.li@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 b1601575e0..532041063a 100644
> > --- a/drivers/usb/gadget/f_sdp.c
> > +++ b/drivers/usb/gadget/f_sdp.c
> > @@ -157,6 +157,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,
> 
> Which endpoint do we use in SDP? EP0 or EP1...n ?

We use EP0 to transfer datas from host to device in report 1 and report 2.
And use EP1 to transfer datas from device to host in report3 and report 4.


> 
> > +};
> > +
> >  static struct usb_descriptor_header *sdp_runtime_descs[] = {
> >  	(struct usb_descriptor_header *)&sdp_intf_runtime,
> >  	(struct usb_descriptor_header *)&sdp_hid_desc, @@ -164,6 +174,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 = {
> > @@ -489,6 +506,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;
> > @@ -526,7 +548,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);
> >  	debug("%s: ep:%p req:%p\n", __func__, ep, req);
> >
> >  	if (!req)
> > @@ -541,11 +563,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); @@ -591,7 +617,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; @@ -724,6 +750,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);
> > +
> >  			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@denx.de



Best regards
Sherry sun

Patch
diff mbox series

diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
index b1601575e0..532041063a 100644
--- a/drivers/usb/gadget/f_sdp.c
+++ b/drivers/usb/gadget/f_sdp.c
@@ -157,6 +157,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,
@@ -164,6 +174,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 = {
@@ -489,6 +506,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;
@@ -526,7 +548,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);
 	debug("%s: ep:%p req:%p\n", __func__, ep, req);
 
 	if (!req)
@@ -541,11 +563,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);
@@ -591,7 +617,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;
@@ -724,6 +750,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);
+
 			jump_to_image_no_args(&spl_image);
 #else
 			/* In U-Boot, allow jumps to scripts */