linux-kernel - Re: [PATCH] USB:serial:option Add Foxconn T99W265

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: <YUhVKrRJ8M2hKJil@hovoldconsulting.com>
Date:   Mon, 20 Sep 2021 11:32:26 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Slark Xiao <slark_xiao@....com>
Cc:     gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB:serial:option Add Foxconn T99W265

On Fri, Sep 17, 2021 at 07:01:06PM +0800, Slark Xiao wrote:
> Adding support for Foxconn device T99W265 for enumeration with
> PID 0xe0db.
> 
> usb-devices output for 0xe0db
> T:  Bus=04 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 19 Spd=5000 MxCh= 0
> D:  Ver= 3.20 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs=  1
> P:  Vendor=0489 ProdID=e0db Rev=05.04
> S:  Manufacturer=Microsoft
> S:  Product=Generic Mobile Broadband Adapter
> S:  SerialNumber=6c50f452
> C:  #Ifs= 5 Cfg#= 1 Atr=a0 MxPwr=896mA
> I:  If#=0x0 Alt= 0 #EPs= 1 Cls=02(commc) Sub=0e Prot=00 Driver=cdc_mbim
> I:  If#=0x1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim
> I:  If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=40 Driver=option
> I:  If#=0x3 Alt= 0 #EPs= 1 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
> I:  If#=0x4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30 Driver=option
> 
> if0/1: MBIM, if2:Diag, if3:GNSS, if4: Modem
> 
> Signed-off-by: Slark Xiao <slark_xiao@....com>

Thanks for the patch and for including all the necessary details in the
commit message.

First, a minor style nit: Please add spaces after the ':'s in the patch
Subject.

> ---
>  drivers/usb/serial/option.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index 29c765cc8495..fde599fa2d73 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -2068,6 +2068,8 @@ static const struct usb_device_id option_ids[] = {
>  	  .driver_info = RSVD(0) | RSVD(1) | RSVD(6) },
>  	{ USB_DEVICE(0x0489, 0xe0b5),						/* Foxconn T77W968 ESIM */
>  	  .driver_info = RSVD(0) | RSVD(1) | RSVD(6) },
> +	{ USB_DEVICE(0x0489, 0xe0db),						/* Foxconn T99W265 MBIM extension*/
> +	  .driver_info = RSVD(0) | RSVD(1) | RSVD(3) },

If you use USB_DEVICE_INTERFACE_CLASS() instead you don't need to
explicitly reserve the MBIM interfaces. 

Also, why are you reserving the GNSS interface (e.g. unlike T77W968)?

>  	{ USB_DEVICE(0x1508, 0x1001),						/* Fibocom NL668 (IOT version) */
>  	  .driver_info = RSVD(4) | RSVD(5) | RSVD(6) },
>  	{ USB_DEVICE(0x2cb7, 0x0104),						/* Fibocom NL678 series */

Johan