Under dummy_hcd, every available endpoint is *either* IN or OUT capable.
But with some real hardware, there are endpoints that support both IN and
OUT. In particular, the PLX 2380 has four available endpoints that each
support both IN and OUT.
raw-gadget currently gets confused and thinks that any endpoint that is
usable as an IN endpoint can never be used as an OUT endpoint.
Fix it by looking at the direction in the configured endpoint descriptor
instead of looking at the hardware capabilities.
With this change, I can use the PLX 2380 with raw-gadget.
Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
Signed-off-by: Jann Horn <[email protected]>
---
drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index c5a2c734234a..d86c3a36441e 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -1004,7 +1004,7 @@ static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
ret = -EBUSY;
goto out_unlock;
}
- if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
+ if (in != usb_endpoint_dir_in(ep->ep->desc)) {
dev_dbg(&dev->gadget->dev, "fail, wrong direction\n");
ret = -EINVAL;
goto out_unlock;
base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
--
2.35.0.rc0.227.g00780c9af4-goog
On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <[email protected]> wrote:
> On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <[email protected]> wrote:
> >
> > Under dummy_hcd, every available endpoint is *either* IN or OUT capable.
> > But with some real hardware, there are endpoints that support both IN and
> > OUT. In particular, the PLX 2380 has four available endpoints that each
> > support both IN and OUT.
> >
> > raw-gadget currently gets confused and thinks that any endpoint that is
> > usable as an IN endpoint can never be used as an OUT endpoint.
> >
> > Fix it by looking at the direction in the configured endpoint descriptor
> > instead of looking at the hardware capabilities.
> >
> > With this change, I can use the PLX 2380 with raw-gadget.
> >
> > Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> > Signed-off-by: Jann Horn <[email protected]>
> > ---
> > drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> > index c5a2c734234a..d86c3a36441e 100644
> > --- a/drivers/usb/gadget/legacy/raw_gadget.c
> > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > @@ -1004,7 +1004,7 @@ static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> > ret = -EBUSY;
> > goto out_unlock;
> > }
> > - if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
> > + if (in != usb_endpoint_dir_in(ep->ep->desc)) {
> > dev_dbg(&dev->gadget->dev, "fail, wrong direction\n");
> > ret = -EINVAL;
> > goto out_unlock;
> >
> > base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> > --
> > 2.35.0.rc0.227.g00780c9af4-goog
> >
>
> Awesome! Thanks for finding this!
>
> What do you think about using
>
> if ((in && !ep->ep->caps.dir_in) || (!in && !ep->ep->caps.dir_out))
>
> instead?
>
> It looks less cryptic: if (in and no in caps) or (out and no out caps) => fail.
That's also semantically different, right?
As I understand it, what we should be checking here is whether the
direction of the request matches the direction previously specified in
USB_RAW_IOCTL_EP_ENABLE, not whether the hardware would be capable of
using the endpoint in the requested direction if it had been
configured for that direction?
But I might also be misunderstanding what's going on - it's not like
I've looked at a spec for this or anything like that, I'm just kinda
guessing...
On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <[email protected]> wrote:
>
> Under dummy_hcd, every available endpoint is *either* IN or OUT capable.
> But with some real hardware, there are endpoints that support both IN and
> OUT. In particular, the PLX 2380 has four available endpoints that each
> support both IN and OUT.
>
> raw-gadget currently gets confused and thinks that any endpoint that is
> usable as an IN endpoint can never be used as an OUT endpoint.
>
> Fix it by looking at the direction in the configured endpoint descriptor
> instead of looking at the hardware capabilities.
>
> With this change, I can use the PLX 2380 with raw-gadget.
>
> Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> Signed-off-by: Jann Horn <[email protected]>
> ---
> drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index c5a2c734234a..d86c3a36441e 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -1004,7 +1004,7 @@ static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> ret = -EBUSY;
> goto out_unlock;
> }
> - if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
> + if (in != usb_endpoint_dir_in(ep->ep->desc)) {
> dev_dbg(&dev->gadget->dev, "fail, wrong direction\n");
> ret = -EINVAL;
> goto out_unlock;
>
> base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>
Awesome! Thanks for finding this!
What do you think about using
if ((in && !ep->ep->caps.dir_in) || (!in && !ep->ep->caps.dir_out))
instead?
It looks less cryptic: if (in and no in caps) or (out and no out caps) => fail.
FTR, I tested both changes with the net2280 hardware that I have, and
they work. At least gadget.c from the test suite enumerates properly.
Most of the tests still fail. But with g_zero, even more of them fail.
P.S. I wonder why everyone suddenly got interested in Raw Gadget lately? :)
On Wed, Jan 26, 2022 at 11:31 PM Jann Horn <[email protected]> wrote:
>
> On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <[email protected]> wrote:
> > On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <[email protected]> wrote:
> > >
> > > Under dummy_hcd, every available endpoint is *either* IN or OUT capable.
> > > But with some real hardware, there are endpoints that support both IN and
> > > OUT. In particular, the PLX 2380 has four available endpoints that each
> > > support both IN and OUT.
> > >
> > > raw-gadget currently gets confused and thinks that any endpoint that is
> > > usable as an IN endpoint can never be used as an OUT endpoint.
> > >
> > > Fix it by looking at the direction in the configured endpoint descriptor
> > > instead of looking at the hardware capabilities.
> > >
> > > With this change, I can use the PLX 2380 with raw-gadget.
> > >
> > > Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> > > Signed-off-by: Jann Horn <[email protected]>
> > > ---
> > > drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> > > index c5a2c734234a..d86c3a36441e 100644
> > > --- a/drivers/usb/gadget/legacy/raw_gadget.c
> > > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > > @@ -1004,7 +1004,7 @@ static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> > > ret = -EBUSY;
> > > goto out_unlock;
> > > }
> > > - if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
> > > + if (in != usb_endpoint_dir_in(ep->ep->desc)) {
> > > dev_dbg(&dev->gadget->dev, "fail, wrong direction\n");
> > > ret = -EINVAL;
> > > goto out_unlock;
> > >
> > > base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> > > --
> > > 2.35.0.rc0.227.g00780c9af4-goog
> > >
> >
> > Awesome! Thanks for finding this!
> >
> > What do you think about using
> >
> > if ((in && !ep->ep->caps.dir_in) || (!in && !ep->ep->caps.dir_out))
> >
> > instead?
> >
> > It looks less cryptic: if (in and no in caps) or (out and no out caps) => fail.
>
> That's also semantically different, right?
> As I understand it, what we should be checking here is whether the
> direction of the request matches the direction previously specified in
> USB_RAW_IOCTL_EP_ENABLE, not whether the hardware would be capable of
> using the endpoint in the requested direction if it had been
> configured for that direction?
> But I might also be misunderstanding what's going on - it's not like
> I've looked at a spec for this or anything like that, I'm just kinda
> guessing...
Ah, yes, you are right. We already checked caps via
usb_gadget_ep_match_desc() in EP_ENABLE. So here it makes sense to
check that the request direction matches the one in the descriptor.
Your original patch makes sense.
Reviewed-by: Andrey Konovalov <[email protected]>
Tested-by: Andrey Konovalov <[email protected]>
Thanks!
On Wed, Jan 26, 2022 at 11:37 PM Andrey Konovalov <[email protected]> wrote:
>
> On Wed, Jan 26, 2022 at 11:31 PM Jann Horn <[email protected]> wrote:
> >
> > On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <[email protected]> wrote:
> > > On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <[email protected]> wrote:
> > > >
> > > > Under dummy_hcd, every available endpoint is *either* IN or OUT capable.
> > > > But with some real hardware, there are endpoints that support both IN and
> > > > OUT. In particular, the PLX 2380 has four available endpoints that each
> > > > support both IN and OUT.
> > > >
> > > > raw-gadget currently gets confused and thinks that any endpoint that is
> > > > usable as an IN endpoint can never be used as an OUT endpoint.
> > > >
> > > > Fix it by looking at the direction in the configured endpoint descriptor
> > > > instead of looking at the hardware capabilities.
> > > >
> > > > With this change, I can use the PLX 2380 with raw-gadget.
> > > >
> > > > Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> > > > Signed-off-by: Jann Horn <[email protected]>
> > > > ---
> > > > drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> > > > index c5a2c734234a..d86c3a36441e 100644
> > > > --- a/drivers/usb/gadget/legacy/raw_gadget.c
> > > > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > > > @@ -1004,7 +1004,7 @@ static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> > > > ret = -EBUSY;
> > > > goto out_unlock;
> > > > }
> > > > - if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
> > > > + if (in != usb_endpoint_dir_in(ep->ep->desc)) {
> > > > dev_dbg(&dev->gadget->dev, "fail, wrong direction\n");
> > > > ret = -EINVAL;
> > > > goto out_unlock;
> > > >
> > > > base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> > > > --
> > > > 2.35.0.rc0.227.g00780c9af4-goog
> > > >
>
> Reviewed-by: Andrey Konovalov <[email protected]>
> Tested-by: Andrey Konovalov <[email protected]>
Greg, could you PTAL and pick this up?
It also makes sense to include this fix into stable kernels.
Cc: <[email protected]>
Thanks!
On Sun, Jan 30, 2022 at 10:58:42PM +0100, Andrey Konovalov wrote:
> On Wed, Jan 26, 2022 at 11:37 PM Andrey Konovalov <[email protected]> wrote:
> >
> > On Wed, Jan 26, 2022 at 11:31 PM Jann Horn <[email protected]> wrote:
> > >
> > > On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <[email protected]> wrote:
> > > > On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <[email protected]> wrote:
> > > > >
> > > > > Under dummy_hcd, every available endpoint is *either* IN or OUT capable.
> > > > > But with some real hardware, there are endpoints that support both IN and
> > > > > OUT. In particular, the PLX 2380 has four available endpoints that each
> > > > > support both IN and OUT.
> > > > >
> > > > > raw-gadget currently gets confused and thinks that any endpoint that is
> > > > > usable as an IN endpoint can never be used as an OUT endpoint.
> > > > >
> > > > > Fix it by looking at the direction in the configured endpoint descriptor
> > > > > instead of looking at the hardware capabilities.
> > > > >
> > > > > With this change, I can use the PLX 2380 with raw-gadget.
> > > > >
> > > > > Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> > > > > Signed-off-by: Jann Horn <[email protected]>
> > > > > ---
> > > > > drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> > > > > index c5a2c734234a..d86c3a36441e 100644
> > > > > --- a/drivers/usb/gadget/legacy/raw_gadget.c
> > > > > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > > > > @@ -1004,7 +1004,7 @@ static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> > > > > ret = -EBUSY;
> > > > > goto out_unlock;
> > > > > }
> > > > > - if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
> > > > > + if (in != usb_endpoint_dir_in(ep->ep->desc)) {
> > > > > dev_dbg(&dev->gadget->dev, "fail, wrong direction\n");
> > > > > ret = -EINVAL;
> > > > > goto out_unlock;
> > > > >
> > > > > base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> > > > > --
> > > > > 2.35.0.rc0.227.g00780c9af4-goog
> > > > >
> >
> > Reviewed-by: Andrey Konovalov <[email protected]>
> > Tested-by: Andrey Konovalov <[email protected]>
>
> Greg, could you PTAL and pick this up?
>
> It also makes sense to include this fix into stable kernels.
>
> Cc: <[email protected]>
Now picked up, thanks.
greg k-h