Date:   Thu, 23 Nov 2017 19:15:37 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     netdev@...r.kernel.org
Subject: Re: [PATCH net] net: qmi_wwan: add support for Cinterion PLS8

Oliver Graute <oliver.graute@...il.com> writes:
> On 23/11/17, Bjørn Mork wrote:
>> Oliver Graute <oliver.graute@...il.com> writes:
>> 
>> >     When the PLS8 devices show up with PID 0x0061 they will expose both a
>> >     QMI port and a WWAN interface.
>> 
>> 
>> Please remove the indentation.
>
> will do after we clarifed if this patch is really needed
>
>> Are you sure this is correct? The Windows drivers I found for this
>> device appear to think all functions use even interface numbers only,
>> presumably because of a control+data interface layout?
>
> honestly not. I have this change for some time in use with older kernel
> to get this PLS8 module working. Its pop-ups as usb0 network interface and
> with 5 /dev/ttyACM interfaces. Its gets an IP address from the provider.
>
> back then I also added this two lines:
>
> +++ V/drivers/usb/serial/option.c	2015-01-26 15:28:09.676671843 +0100
> @@ -338,6 +338,7 @@ static void option_instat_callback(struc
>  #define CINTERION_PRODUCT_PH8			0x0053
>  #define CINTERION_PRODUCT_AHXX			0x0055
>  #define CINTERION_PRODUCT_PLXX			0x0060
> +#define CINTERION_PRODUCT_PLS8			0x0061
>  
> @@ -1251,6 +1252,7 @@ static const struct usb_device_id option
>  	{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_PLXX),
> +	{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_PLS8),

This should not be necessary and is wrong given the descriptors in your
lsusb outout.  All the serial functions look like proper CDC ACM
functions which should Just Work with the cdc-acm class driver.  No need
for any device specific entry for those.

>> But I don't know anything about this device.  Maybe the layout is
>> configurable without changing the device ID?  Do you have any more
>> information you can provide, like for example a verbose lsusb output or
>> the relevant part of /sys/kernel/debug/usb/devices?
>
> here the output of lsusb -vvv
>
> Bus 002 Device 002: ID 1e2d:0061  
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass            0 (Defined at Interface level)
>   bDeviceSubClass         0 
>   bDeviceProtocol         0 
>   bMaxPacketSize0        64
>   idVendor           0x1e2d 
>   idProduct          0x0061 
>   bcdDevice            2.32
>   iManufacturer           1 Cinterion
>   iProduct                2 LTE Modem
>   iSerial                 0 
>   bNumConfigurations      1
>   Configuration Descriptor:
>     bLength                 9
>     bDescriptorType         2
>     wTotalLength          497
>     bNumInterfaces         14
>     bConfigurationValue     1
>     iConfiguration          0 
>     bmAttributes         0xe0
>       Self Powered
>       Remote Wakeup
>     MaxPower               20mA
>     Interface Association:
>       bLength                 8
>       bDescriptorType        11
>       bFirstInterface         0
>       bInterfaceCount         2
>       bFunctionClass          2 Communications
>       bFunctionSubClass       2 Abstract (modem)
>       bFunctionProtocol       1 AT-commands (v.25ter)
>       iFunction               5 CDC Serial
>     Interface Descriptor:
>       bLength                 9
>       bDescriptorType         4
>       bInterfaceNumber        0
>       bAlternateSetting       0
>       bNumEndpoints           1
>       bInterfaceClass         2 Communications
>       bInterfaceSubClass      2 Abstract (modem)
>       bInterfaceProtocol      1 AT-commands (v.25ter)
>       iInterface              3 CDC Abstract Control Model (ACM)
>       CDC Header:
>         bcdCDC               1.20
>       CDC Call Management:
>         bmCapabilities       0x03
>           call management
>           use DataInterface
>         bDataInterface          1
>       CDC ACM:
>         bmCapabilities       0x02
>           line coding and serial state
>       CDC Union:
>         bMasterInterface        0
>         bSlaveInterface         1 



OK, this is consistent with the Windows driver files, so I believe your
patch could work.  The USB interface it identified as QMI is in fact a
CDC ACM data interface.


>     Interface Association:
>       bLength                 8
>       bDescriptorType        11
>       bFirstInterface        10
>       bInterfaceCount         2
>       bFunctionClass          2 Communications
>       bFunctionSubClass       6 Ethernet Networking
>       bFunctionProtocol       0 
>       iFunction               9 CDC Ethernet (RmNet)
>     Interface Descriptor:
>       bLength                 9
>       bDescriptorType         4
>       bInterfaceNumber       10
>       bAlternateSetting       0
>       bNumEndpoints           1
>       bInterfaceClass         2 Communications
>       bInterfaceSubClass      6 Ethernet Networking
>       bInterfaceProtocol      0 
>       iInterface             10 CDC Ethernet Control Model (ECM) for RmNet
>       CDC Header:
>         bcdCDC               1.20
>       CDC Ethernet:
>         iMacAddress                     12 DEADBEEF0000

Nice mac address :-)

This is also consistent with the Windows drivers.  And being a proper
CDC ECM class function, it should Just Work with the cdc_ether driver.
Except for the "RmNet" part, which I guess is the reason you want to
add this device to qmi_wwan.  Which is fine, *if* we can be reasonably
certain that it does support QMI.  The description string is a strong
indication, but it would be even better to know this was tested.

But adding this to qmi_wwan is not enough.  You also need to add a
blacklist entry to cdc_ether.  Both should use a device+class match,
similar to the Novatel entries.  This will make the interface numbering
irrelevant, and will allow a single entry to match both QMI/rmnet
functions.




Bjørn

Powered by blists - more mailing lists