netdev - Re: [PATCH V3] r8152: add Linksys USB3GIGV1 id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANEJEGvK94v_SV9xWmDaiA4CZOiroKZVhfCH4vydF6_Su2_PyA@mail.gmail.com>
Date:   Wed, 27 Sep 2017 17:07:05 -0700
From:   Grant Grundler <grundler@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Grant Grundler <grundler@...omium.org>,
        Hayes Wang <hayeswang@...ltek.com>,
        Oliver Neukum <oneukum@...e.com>,
        linux-usb <linux-usb@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH V3] r8152: add Linksys USB3GIGV1 id

Hi Doug!

On Wed, Sep 27, 2017 at 4:47 PM, Doug Anderson <dianders@...omium.org> wrote:
> Hi,
>
> On Wed, Sep 27, 2017 at 10:28 AM, Grant Grundler <grundler@...omium.org> wrote:
>> This linksys dongle by default comes up in cdc_ether mode.
>> This patch allows r8152 to claim the device:
>>    Bus 002 Device 002: ID 13b1:0041 Linksys
>>
>> Signed-off-by: Grant Grundler <grundler@...omium.org>
>> ---
>>  drivers/net/usb/cdc_ether.c | 10 ++++++++++
>>  drivers/net/usb/r8152.c     |  2 ++
>>  2 files changed, 12 insertions(+)
>>
>> V3: for backwards compat, add #ifdef CONFIG_USB_RTL8152 around
>>     the cdc_ether blacklist entry so the cdc_ether driver can
>>     still claim the device if r8152 driver isn't configured.
>>
>> V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist
>>
>> diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
>> index 8ab281b478f2..446dcc0f1f70 100644
>> --- a/drivers/net/usb/cdc_ether.c
>> +++ b/drivers/net/usb/cdc_ether.c
>> @@ -546,6 +546,7 @@ static const struct driver_info wwan_info = {
>>  #define DELL_VENDOR_ID         0x413C
>>  #define REALTEK_VENDOR_ID      0x0bda
>>  #define SAMSUNG_VENDOR_ID      0x04e8
>> +#define LINKSYS_VENDOR_ID      0x13b1
>>  #define LENOVO_VENDOR_ID       0x17ef
>
> Slight nit that "LI" sorts after "LE".  You got it right in the other case...

The list isn't sorted by any rational thing I can see.  I managed to
check my OCD reaction to sort the list numerically. :)

>>  #define NVIDIA_VENDOR_ID       0x0955
>>  #define HP_VENDOR_ID           0x03f0
>> @@ -737,6 +738,15 @@ static const struct usb_device_id  products[] = {
>>         .driver_info = 0,
>>  },
>>
>> +#ifdef CONFIG_USB_RTL8152
>> +/* Linksys USB3GIGV1 Ethernet Adapter */
>> +{
>> +       USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
>> +                       USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
>> +       .driver_info = 0,
>> +},
>> +#endif
>
> I believe you want to use IS_ENABLED(), don't you?

Ah yes - I wasn't aware IS_ENABLED existed.  Will respin V4 with this
if there isn't any other feedback.


> There's still a weird esoteric side case where kernel modules don't
> all need to be included in the filesystem just because they were built
> at the same time.  ...but IMHO that seems like enough of a nit that we
> can probably ignore it unless someone has a better idea.

I think that would require a run time check. I'm perfectly willing to
ignore that case. :)

thanks!
grant

>
>
> -Doug