Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <52da93cd-6a78-1b77-6a86-c338c7cb11e9@gmail.com>
Date:   Tue, 8 Mar 2022 11:06:18 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH RFT] can: mcba_usb: properly check endpoint type
Content-Language: en-US
To:     Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc:     Yasushi SHOJI <yashi@spacecubics.com>,
        Wolfgang Grandegger <wg@grandegger.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        David Miller <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        linux-can <linux-can@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        syzbot+3bc1dce0cc0052d60fde@syzkaller.appspotmail.com
References: <20220307185314.11228-1-paskripkin@gmail.com>
 <CAMZ6RqKEALqGSh-tr_jTbQWca0wHK7t96yR3N-r625pbM4cUSw@mail.gmail.com>
From:   Pavel Skripkin <paskripkin@gmail.com>
In-Reply-To: <CAMZ6RqKEALqGSh-tr_jTbQWca0wHK7t96yR3N-r625pbM4cUSw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@gmail.com> 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