| Message ID | 20190822134455.32366-4-sherry.sun@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | Make some changes to SDP | expand |
On Thu, 22 Aug 2019 01:46:20 +0000 Sherry Sun <sherry.sun@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@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 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. > 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. > 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
Hi Lukasz, > > On Thu, 22 Aug 2019 01:46:20 +0000 > Sherry Sun <sherry.sun@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@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 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@denx.de
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); 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); + jump_to_image_no_args(&spl_image); #else /* In U-Boot, allow jumps to scripts */