diff mbox series

usb: raw-gadget: fix handling of dual-direction-capable endpoints

Message ID 20220126205214.2149936-1-jannh@google.com (mailing list archive)
State Accepted
Commit 292d2c82b105d92082c2120a44a58de9767e44f1
Headers show
Series usb: raw-gadget: fix handling of dual-direction-capable endpoints | expand

Commit Message

Jann Horn Jan. 26, 2022, 8:52 p.m. UTC 
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 <jannh@google.com>
---
 drivers/usb/gadget/legacy/raw_gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8

Comments

Andrey Konovalov Jan. 26, 2022, 10:12 p.m. UTC | #1 
On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <jannh@google.com> 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 <jannh@google.com>
> ---
>  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? :)
Jann Horn Jan. 26, 2022, 10:30 p.m. UTC | #2 
On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <jannh@google.com> 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 <jannh@google.com>
> > ---
> >  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...
Andrey Konovalov Jan. 26, 2022, 10:37 p.m. UTC | #3 
On Wed, Jan 26, 2022 at 11:31 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> > On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <jannh@google.com> 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 <jannh@google.com>
> > > ---
> > >  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 <andreyknvl@gmail.com>
Tested-by: Andrey Konovalov <andreyknvl@gmail.com>

Thanks!
Andrey Konovalov Jan. 30, 2022, 9:58 p.m. UTC | #4 
On Wed, Jan 26, 2022 at 11:37 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 11:31 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> > > On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <jannh@google.com> 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 <jannh@google.com>
> > > > ---
> > > >  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 <andreyknvl@gmail.com>
> Tested-by: Andrey Konovalov <andreyknvl@gmail.com>

Greg, could you PTAL and pick this up?

It also makes sense to include this fix into stable kernels.

Cc: <stable@vger.kernel.org>

Thanks!
Greg Kroah-Hartman Jan. 31, 2022, 1:20 p.m. UTC | #5 
On Sun, Jan 30, 2022 at 10:58:42PM +0100, Andrey Konovalov wrote:
> On Wed, Jan 26, 2022 at 11:37 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 11:31 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > On Wed, Jan 26, 2022 at 11:12 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> > > > On Wed, Jan 26, 2022 at 9:52 PM Jann Horn <jannh@google.com> 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 <jannh@google.com>
> > > > > ---
> > > > >  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 <andreyknvl@gmail.com>
> > Tested-by: Andrey Konovalov <andreyknvl@gmail.com>
> 
> Greg, could you PTAL and pick this up?
> 
> It also makes sense to include this fix into stable kernels.
> 
> Cc: <stable@vger.kernel.org>

Now picked up, thanks.

greg k-h
diff mbox series

Patch

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;