| Message ID | 20220311082944.4881-1-jj251510319013@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: raw-gadget: return -EINVAL if no proper ep address available | expand |
On Fri, Mar 11, 2022 at 9:30 AM Wei Ming Chen <jj251510319013@gmail.com> wrote: > > If we try to use raw_ioctl_ep_enable() for ep5in on a hardware that > only support from ep1-ep4 for both in and out direction, it will return > -EBUSY originally. > > I think it will be more intuitive if we return -EINVAL, because -EBUSY > sounds like ep5in is not available now, but might be available in the > future. > > Signed-off-by: Wei Ming Chen <jj251510319013@gmail.com> > --- > > Changes in v2: > - Rename variable from ep_num_matched to ep_props_matched > - Incorporate the patch from Andrey Konovalov that cover > the foloowing cases: > 1. If there are no endpoints that match the provided descriptor, return > EINVAL. > 2. If there are matching endpoints, but they are all already enabled, > return EBUSY. > > drivers/usb/gadget/legacy/raw_gadget.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c > index d86c3a36441e..e5707626c4d4 100644 > --- a/drivers/usb/gadget/legacy/raw_gadget.c > +++ b/drivers/usb/gadget/legacy/raw_gadget.c > @@ -758,6 +758,7 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value) > unsigned long flags; > struct usb_endpoint_descriptor *desc; > struct raw_ep *ep; > + bool ep_props_matched = false; > > desc = memdup_user((void __user *)value, sizeof(*desc)); > if (IS_ERR(desc)) > @@ -787,13 +788,14 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value) > > for (i = 0; i < dev->eps_num; i++) { > ep = &dev->eps[i]; > - if (ep->state != STATE_EP_DISABLED) > - continue; > if (ep->addr != usb_endpoint_num(desc) && > ep->addr != USB_RAW_EP_ADDR_ANY) > continue; > if (!usb_gadget_ep_match_desc(dev->gadget, ep->ep, desc, NULL)) > continue; > + ep_props_matched = true; > + if (ep->state != STATE_EP_DISABLED) > + continue; > ep->ep->desc = desc; > ret = usb_ep_enable(ep->ep); > if (ret < 0) { > @@ -815,8 +817,13 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value) > goto out_unlock; > } > > - dev_dbg(&dev->gadget->dev, "fail, no gadget endpoints available\n"); > - ret = -EBUSY; > + if (!ep_props_matched) { > + dev_dbg(&dev->gadget->dev, "fail, bad endpoint descriptor\n"); > + ret = -EINVAL; > + } else { > + dev_dbg(&dev->gadget->dev, "fail, no endpoints available\n"); > + ret = -EBUSY; > + } > > out_free: > kfree(desc); > -- > 2.25.1 > Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> Greg, could you consider picking this up? Thanks!
diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c index d86c3a36441e..e5707626c4d4 100644 --- a/drivers/usb/gadget/legacy/raw_gadget.c +++ b/drivers/usb/gadget/legacy/raw_gadget.c @@ -758,6 +758,7 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value) unsigned long flags; struct usb_endpoint_descriptor *desc; struct raw_ep *ep; + bool ep_props_matched = false; desc = memdup_user((void __user *)value, sizeof(*desc)); if (IS_ERR(desc)) @@ -787,13 +788,14 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value) for (i = 0; i < dev->eps_num; i++) { ep = &dev->eps[i]; - if (ep->state != STATE_EP_DISABLED) - continue; if (ep->addr != usb_endpoint_num(desc) && ep->addr != USB_RAW_EP_ADDR_ANY) continue; if (!usb_gadget_ep_match_desc(dev->gadget, ep->ep, desc, NULL)) continue; + ep_props_matched = true; + if (ep->state != STATE_EP_DISABLED) + continue; ep->ep->desc = desc; ret = usb_ep_enable(ep->ep); if (ret < 0) { @@ -815,8 +817,13 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value) goto out_unlock; } - dev_dbg(&dev->gadget->dev, "fail, no gadget endpoints available\n"); - ret = -EBUSY; + if (!ep_props_matched) { + dev_dbg(&dev->gadget->dev, "fail, bad endpoint descriptor\n"); + ret = -EINVAL; + } else { + dev_dbg(&dev->gadget->dev, "fail, no endpoints available\n"); + ret = -EBUSY; + } out_free: kfree(desc);
If we try to use raw_ioctl_ep_enable() for ep5in on a hardware that only support from ep1-ep4 for both in and out direction, it will return -EBUSY originally. I think it will be more intuitive if we return -EINVAL, because -EBUSY sounds like ep5in is not available now, but might be available in the future. Signed-off-by: Wei Ming Chen <jj251510319013@gmail.com> --- Changes in v2: - Rename variable from ep_num_matched to ep_props_matched - Incorporate the patch from Andrey Konovalov that cover the foloowing cases: 1. If there are no endpoints that match the provided descriptor, return EINVAL. 2. If there are matching endpoints, but they are all already enabled, return EBUSY. drivers/usb/gadget/legacy/raw_gadget.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)