Message-ID: <e56d75116d36eee749439f8d0a1b8ca4f7ce1445.camel@hotmail.com>
Date:   Tue, 7 Nov 2023 22:02:28 +0000
From:   Victor Fragoso <victorffs@...mail.com>
To:     "larsm17@...il.com" <larsm17@...il.com>,
        "johan@...nel.org" <johan@...nel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] USB: serial: option: add Fibocom L7xx modules

On Sat, 2023-10-28 at 02:59 +0700, Lars Melin wrote:
> On 10/28/2023 0:55, Victor Fragoso wrote:
> > On Thu, 2023-10-26 at 20:13 +0700, Lars Melin wrote:
> > > On 10/26/2023 8:24, Victor Fragoso wrote:
> > > > Add support for Fibocom L7xx module series and variants.
> > > > 
> > > > L716-EU-60 (ECM):
> > > > T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 17 Spd=480  MxCh= 0
> > > > D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> > > > P:  Vendor=19d2 ProdID=0579 Rev= 1.00
> > > > S:  Manufacturer=Fibocom,Incorporated
> > > > S:  Product=Fibocom Mobile Boardband
> > > > S:  SerialNumber=1234567890ABCDEF
> > > > C:* #Ifs= 7 Cfg#= 1 Atr=e0 MxPwr=500mA
> > > > A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> > > > I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> > > > E:  Ad=87(I) Atr=03(Int.) MxPS=  16 Ivl=32ms
> > > > I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > > > I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > > > E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> > > > E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > 
> > > > L716-EU-60 (RNDIS):
> > > > T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 21 Spd=480  MxCh= 0
> > > > D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> > > > P:  Vendor=2cb7 ProdID=0001 Rev= 1.00
> > > > S:  Manufacturer=Fibocom,Incorporated
> > > > S:  Product=Fibocom Mobile Boardband
> > > > S:  SerialNumber=1234567890ABCDEF
> > > > C:* #Ifs= 7 Cfg#= 1 Atr=e0 MxPwr=500mA
> > > > A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> > > > I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> > > > E:  Ad=87(I) Atr=03(Int.) MxPS=  16 Ivl=32ms
> > > > I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > > > I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > > > E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> > > > E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > 
> > > > L716-EU-10 (ECM):
> > > > T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 21 Spd=480  MxCh= 0
> > > > D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> > > > P:  Vendor=2cb7 ProdID=0001 Rev= 1.00
> > > > S:  Manufacturer=Fibocom,Incorporated
> > > > S:  Product=Fibocom Mobile Boardband
> > > > S:  SerialNumber=1234567890ABCDEF
> > > > C:* #Ifs= 7 Cfg#= 1 Atr=e0 MxPwr=500mA
> > > > A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> > > > I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> > > > E:  Ad=87(I) Atr=03(Int.) MxPS=  16 Ivl=32ms
> > > > I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > > > I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > > > E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > > > E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > I:* If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> > > > E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > > > 
> > > > Signed-off-by: Victor Fragoso <victorffs@...mail.com>
> > > > ---
> > > >    drivers/usb/serial/option.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> > > > index 45dcfaadaf98..4ba3dc352d65 100644
> > > > --- a/drivers/usb/serial/option.c
> > > > +++ b/drivers/usb/serial/option.c
> > > > @@ -2262,6 +2262,11 @@ static const struct usb_device_id option_ids[] =
> > > > {
> > > >    	{ USB_DEVICE_INTERFACE_CLASS(0x2cb7, 0x01a2, 0xff)
> > > > },			/* Fibocom FM101-GL (laptop MBIM) */
> > > >    	{ USB_DEVICE_INTERFACE_CLASS(0x2cb7, 0x01a4,
> > > > 0xff),			/* Fibocom FM101-GL (laptop MBIM) */
> > > >    	  .driver_info = RSVD(4) },
> > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x2cb7, 0x0001, 0xff, 0xff,
> > > > 0xff) },	/* Fibocom L71x */
> > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x2cb7, 0x0001, 0x0a, 0x00,
> > > > 0xff) },	/* Fibocom L71x */
> > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x2cb7, 0x0100, 0xff, 0xff,
> > > > 0xff) },	/* Fibocom L71x */
> > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x19d2, 0x0256, 0xff, 0xff,
> > > > 0xff) },	/* Fibocom L71x */
> > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x19d2, 0x0579, 0xff, 0xff,
> > > > 0xff) },	/* Fibocom L71x */
> > > >    	{ USB_DEVICE_INTERFACE_CLASS(0x2df3, 0x9d03, 0xff)
> > > > },			/* LongSung M5710 */
> > > >    	{ USB_DEVICE_INTERFACE_CLASS(0x305a, 0x1404, 0xff)
> > > > },			/* GosunCn GM500 RNDIS */
> > > >    	{ USB_DEVICE_INTERFACE_CLASS(0x305a, 0x1405, 0xff)
> > > > },			/* GosunCn GM500 MBIM */
> > > 
> > > 
> > > Hi Victor, thanks for the patch, there is unfortunately the following
> > > errors in it:
> > > The device list is sorted in ascending order based on vid:pid, you have
> > > inserted all of your added Id's in the wrong place.
> > > 
> > > 19d2:0579 is a ZTE device Id and should be placed among the other 19d2
> > > devices.
> > > 
> > > You have not included usb-devices output for 19d2:0256 and 2cb7:0100,
> > > and I have strong reasons to believe that they should not be included in
> > > the option driver.
> > > If you are of another opinion then please show the usb-devices output
> > > for them, otherwise remove them from the patch.
> > > 
> > > You have added support for an interface with the attributes 0a/00/ff ,
> > > there is no such interface in your provided usb-devices listing,
> > > interface Class 0a does not even belong to the option driver.
> > > 
> > > Thanks
> > > Lars
> > 
> > Hi Lars, sorry about the wrong order, I will correct it.
> > 
> > But about the ZTE device ID, I belive that we should insert among
> > Fibocom drivers because we are talking about a Fibocom module that is
> > using an ZTE Chipset.
> > So, this is exactly the same situation from Fibocom L610 IDs (0x1782,
> > 0x4d10 / 0x1782, 0x4d11) that is using the Unisoc Chipset but were
> > inserted among Fibocom drivers.
> > 
> > And about the usb-devices output, let me explain better:
> > I am a Field Application Enginner at Fibocom Brazil and I am using the
> > IDs from our internal and official documentation.
> > On this documents its suggested to add all this IDs because it will
> > guarantee that can be used on all the variants devices from L71x series
> > (that can change according to different part number, region support or
> > network protocol).
> > Unfortunately, I don't have all the modules variations available with
> > me right now to test and share all the outputs.
> > 
> > Can we continue with all the IDs that I have inserted?
> > Or do you prefer to keep just the devices that I tested by myself until
> > now?
> > 
> > Victor Fragoso
> 
> Johan may have a different opinion from mine and he is the one to 
> decide, my take is that there is no value in having a minor part of the 
> list grouped by mfgr and the major part of the list sorted by USB Id.
> We have also seen in the past that when a mfgr1 buys a chipset from 
> mfgr2 and then sells his product with mfgr2 Id instead of using one of 
> his own Id's then there is also not uncommon that mfgr3 is also buying 
> the same chipset without changing Id. Hence "Manufacturer name" is not 
> unique but the vid is..
> 
> The list should not be seen or used as a cross reference between 
> mfgr:product name and vid:pid, anyone who needs to search the driver 
> source to see if a device is supported ought to know its vid:pid so can 
> search on those and that works much better if the list is sorted by 
> vid:pid in ascending order. Personally I'd rather see a source without 
> all the unnecessary defines, only vid:pid based and with the 
> mfgr/product as an optional comment.
> 
> When adding Id's to the driver it is you who should know the devices you 
> are adding, don't add Id's if you have not personally confirmed their 
> interface composition and usage.
> Those who told you add these Id's apparently also told you that there 
> are two versions of 2cb7:0001, one with ECM interfaces and one with 
> RNDIS interfaces but your usb-devices output show both to be identical.. 
> There is no RNDIS interface in your listings, both of them will load
> the cdc_ether driver.
> 
> br
> Lars
> 

Lars, understood. Your point makes sense and I could accept it with no
problems.

Johan, can you please share your opinion to decide?

1- Should I create a new commit inserting the PID grouped among to
Fibocom's VID? Or separately on Fibocom's VID section and another one
on ZTE's VID section? 
2- Should I remove the generic Fibocom product comment and add just the
product/mode that I could test it? e.g. Fibocom L716-EU (ECM)

Victor Fragoso

Powered by blists - more mailing lists