Re: [PATCH RFT] can: mcba_usb: properly check endpoint type

[Pavel Skripkin]

Hi Vincent,

On 3/8/22 03:23, Vincent MAILHOL wrote:
> Hi Pavel,
>
> > [PATCH RFT] can: mcba_usb: properly check endpoint type
> It is RFC, not RFT :)
> I guess you went on some manual editing. Next time, you can just let
> git add the tag for you by doing:
> | git format-patch --rfc ...

I marked it as RFT, because I wanted someone to test it. But indeed with 
my lack of usb knowledge it should have been RFC :)

> On Tue. 8 Mar 2022, 03:53, Pavel Skripkin <paskripkin@xxxxxxxxx> wrote:

[snip]

> > /* MCBA endpoint numbers */
> > #define MCBA_USB_EP_IN 1
> > #define MCBA_USB_EP_OUT 1
> >
> > That's why check only for in endpoint is added
>
> MCBA_USB_EP_{IN,OUT} are respectively used in usb_rcvbulkpipe()
> and usb_sndbulkpipe().  I invite you to have a look at what those
> macros do and you will understand that these returns two different
> pipes:
>
> https://elixir.bootlin.com/linux/latest/source/include/linux/usb.h#L1964
>
> In other words, ep_in and ep_out are some indexes of a different
> entity and do not conflict with each other.

Got it! Thank you for pointing out!

> > ---
> >  drivers/net/can/usb/mcba_usb.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > index 77bddff86252..646aac1a8684 100644
> > --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -807,6 +807,13 @@ static int mcba_usb_probe(struct usb_interface *intf,
> >         struct mcba_priv *priv;
> >         int err;
> >         struct usb_device *usbdev = interface_to_usbdev(intf);
> > +       struct usb_endpoint_descriptor *in;
> > +
> > +       err = usb_find_common_endpoints(intf->cur_altsetting, &in, NULL, NULL, NULL);
>
> If you go this direction, then please use
> usb_find_common_endpoint() to retrieve the value of both ep_in
> and ep_out and use them instead of MCBA_USB_EP_{IN,OUT}
>
> > +       if (err) {
> > +               dev_err(&intf->dev, "Can't find endpoints\n");
> > +               return -ENODEV;
>
> return ret;
>
> Please keep the error code of usb_find_common_endpoint().

Will redo in v2. Thank you for reviewing




With regards,
Pavel Skripkin