| Message ID | 20180908125754.1947-1-kristian.evensen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | option: Improve Quectel EP06 detection | expand |
On Sat, Sep 08, 2018 at 02:57:54PM +0200, Kristian Evensen wrote: > The Quectel EP06 (and EM06/EG06) LTE modem supports updating the USB > configuration, without the VID/PID or configuration number changing. > When the configuration is updated and interfaces are added/removed, the > interface numbers are updated. This causes our current code for matching > EP06 not to work as intended, as the assumption about reserved > interfaces no longer holds. If for example the diagnostic (first) > interface is removed, option will (try to) bind to the QMI interface. > > This patch improves EP06 detection by replacing the current match with > two matches, and those matches check class, subclass and protocol as > well as VID and PID. The diag interface exports class, subclass and > protocol as 0xff. For the other serial interfaces, class is 0xff and > subclass and protocol are both 0x0. > > The modem can export the following devices and always in this order: > diag, nmea, at, ppp. qmi and adb. This means that diag can only ever be > interface 0, and interface numbers 1-5 should be marked as reserved. The > three other serial devices can have interface numbers 0-3, but I have > not marked any interfaces as reserved. The reason is that the serial > devices are the only interfaces exported by the device where subclass > and protocol is 0x0. > > QMI exports the same class, subclass and protocol values as the diag > interface. However, the two interfaces have different number of > endpoints, QMI has three and diag two. I have added a check for number > of interfaces if VID/PID matches the EP06, and we ignore the device if > number of interfaces equals three (and subclass is set). > > Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com> What a mess. Please provide the output of usb-devices (or lsusb -v) for both "configurations". How do you update the configuration by the way? Thanks, Johan
Hi, On Mon, Sep 10, 2018 at 12:30 PM Johan Hovold <johan@kernel.org> wrote: > Please provide the output of usb-devices (or lsusb -v) for both > "configurations". How do you update the configuration by the way? The configuration is updated using a proprietary AT-command (AT+QCFG="usbcfg"). The format of the command is as follows: AT+QCFG="usbcfg",<vid>,<pid>,<diag>,<nmea>,<at_port>,<modem>,<rmnet>,<adb>. In other words, you set which interfaces to enable/disable. Based on my testing, it is only possible to enable/disable diag, rmnet (QMI) and adb, as well as nmea, at_port and modem together. I.e., it is not possible to only disable for example nmea. With all interfaces are enabled, the output of lsusb -v looks as follows: Bus 002 Device 008: ID 2c7c:0306 Quectel Wireless Solutions Co., Ltd. EG06/EP06/EM06 LTE-A modem Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 3.00 bDeviceClass 239 Miscellaneous Device bDeviceSubClass 2 ? bDeviceProtocol 1 Interface Association bMaxPacketSize0 9 idVendor 0x2c7c Quectel Wireless Solutions Co., Ltd. idProduct 0x0306 EG06/EP06/EM06 LTE-A modem bcdDevice 3.10 iManufacturer 1 Quectel iProduct 2 LTE-A Module iSerial 3 c1494706 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 328 bNumInterfaces 6 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 126mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 255 Vendor Specific Subclass bInterfaceProtocol 255 Vendor Specific Protocol iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x01 EP 1 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 ** UNRECOGNIZED: 05 24 00 10 01 ** UNRECOGNIZED: 05 24 01 00 00 ** UNRECOGNIZED: 04 24 02 02 ** UNRECOGNIZED: 05 24 06 00 00 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x000a 1x 10 bytes bInterval 9 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x82 EP 2 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x02 EP 2 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 2 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 ** UNRECOGNIZED: 05 24 00 10 01 ** UNRECOGNIZED: 05 24 01 00 00 ** UNRECOGNIZED: 04 24 02 02 ** UNRECOGNIZED: 05 24 06 00 00 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x85 EP 5 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x000a 1x 10 bytes bInterval 9 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x84 EP 4 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x03 EP 3 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 3 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 ** UNRECOGNIZED: 05 24 00 10 01 ** UNRECOGNIZED: 05 24 01 00 00 ** UNRECOGNIZED: 04 24 02 02 ** UNRECOGNIZED: 05 24 06 00 00 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x87 EP 7 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x000a 1x 10 bytes bInterval 9 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x86 EP 6 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x04 EP 4 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 4 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 255 Vendor Specific Subclass bInterfaceProtocol 255 Vendor Specific Protocol iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x89 EP 9 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0008 1x 8 bytes bInterval 9 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x88 EP 8 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x05 EP 5 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 5 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 66 bInterfaceProtocol 1 iInterface 4 ADB Interface Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x06 EP 6 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x8a EP 10 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Binary Object Store Descriptor: bLength 5 bDescriptorType 15 wTotalLength 22 bNumDeviceCaps 2 USB 2.0 Extension Device Capability: bLength 7 bDescriptorType 16 bDevCapabilityType 2 bmAttributes 0x00000002 Link Power Management (LPM) Supported SuperSpeed USB Device Capability: bLength 10 bDescriptorType 16 bDevCapabilityType 3 bmAttributes 0x00 wSpeedsSupported 0x000f Device can operate at Low Speed (1Mbps) Device can operate at Full Speed (12Mbps) Device can operate at High Speed (480Mbps) Device can operate at SuperSpeed (5Gbps) bFunctionalitySupport 1 Lowest fully-functional device speed is Full Speed (12Mbps) bU1DevExitLat 1 micro seconds bU2DevExitLat 500 micro seconds Device Status: 0x0000 (Bus Powered) If I for example disable diag, then the bInterfaceNumber of nmea changes from 1 to 0, at from 2 to 1, etc., etc. BR, Kristian
On Sat, 2018-09-08 at 14:57 +0200, Kristian Evensen wrote: > The Quectel EP06 (and EM06/EG06) LTE modem supports updating the USB > configuration, without the VID/PID or configuration number changing. > When the configuration is updated and interfaces are added/removed, > the > interface numbers are updated. This causes our current code for > matching > EP06 not to work as intended, as the assumption about reserved > interfaces no longer holds. If for example the diagnostic (first) > interface is removed, option will (try to) bind to the QMI interface. > > This patch improves EP06 detection by replacing the current match > with > two matches, and those matches check class, subclass and protocol as > well as VID and PID. The diag interface exports class, subclass and > protocol as 0xff. For the other serial interfaces, class is 0xff and > subclass and protocol are both 0x0. > > The modem can export the following devices and always in this order: > diag, nmea, at, ppp. qmi and adb. This means that diag can only ever > be > interface 0, and interface numbers 1-5 should be marked as reserved. > The > three other serial devices can have interface numbers 0-3, but I have > not marked any interfaces as reserved. The reason is that the serial > devices are the only interfaces exported by the device where subclass > and protocol is 0x0. > > QMI exports the same class, subclass and protocol values as the diag > interface. However, the two interfaces have different number of > endpoints, QMI has three and diag two. I have added a check for > number > of interfaces if VID/PID matches the EP06, and we ignore the device > if > number of interfaces equals three (and subclass is set). I double-checked the permutations & logic and it makes sense to me. Acked-by: Dan Williams <dcbw@redhat.com> > Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com> > --- > drivers/usb/serial/option.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/serial/option.c > b/drivers/usb/serial/option.c > index 0215b70c4efc..835dcd2875a7 100644 > --- a/drivers/usb/serial/option.c > +++ b/drivers/usb/serial/option.c > @@ -1081,8 +1081,9 @@ static const struct usb_device_id option_ids[] > = { > .driver_info = RSVD(4) }, > { USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_BG96), > .driver_info = RSVD(4) }, > - { USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EP06), > - .driver_info = RSVD(4) | RSVD(5) }, > + { USB_DEVICE_AND_INTERFACE_INFO(QUECTEL_VENDOR_ID, > QUECTEL_PRODUCT_EP06, 0xff, 0xff, 0xff), > + .driver_info = RSVD(1) | RSVD(2) | RSVD(3) | RSVD(4) | > RSVD(5) }, > + { USB_DEVICE_AND_INTERFACE_INFO(QUECTEL_VENDOR_ID, > QUECTEL_PRODUCT_EP06, 0xff, 0, 0) }, > { USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_6001) }, > { USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_CMU_300) }, > { USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_6003), > @@ -1985,6 +1986,7 @@ static int option_probe(struct usb_serial > *serial, > { > struct usb_interface_descriptor *iface_desc = > &serial->interface->cur_altsetting- > >desc; > + struct usb_device_descriptor *dev_desc = &serial->dev- > >descriptor; > unsigned long device_flags = id->driver_info; > > /* Never bind to the CD-Rom emulation interface */ > @@ -1999,6 +2001,17 @@ static int option_probe(struct usb_serial > *serial, > if (device_flags & RSVD(iface_desc->bInterfaceNumber)) > return -ENODEV; > > + /* > + * Don't bind to the QMI device of the Quectel > EP06/EG06/EM06. Class, > + * subclass and protocol is 0xff for both the diagnostic > port and the > + * QMI interface, but the diagnostic port only has two > endpoints (QMI > + * has three). > + */ > + if (dev_desc->idVendor == cpu_to_le16(QUECTEL_VENDOR_ID) && > + dev_desc->idProduct == cpu_to_le16(QUECTEL_PRODUCT_EP06) > && > + iface_desc->bInterfaceSubClass && iface_desc- > >bNumEndpoints == 3) > + return -ENODEV; > + > /* Store the device flags so we can use them during attach. > */ > usb_set_serial_data(serial, (void *)device_flags); >
On 9/10/2018 18:39, Kristian Evensen wrote: > Hi, > > On Mon, Sep 10, 2018 at 12:30 PM Johan Hovold <johan@kernel.org> wrote: >> Please provide the output of usb-devices (or lsusb -v) for both >> "configurations". How do you update the configuration by the way? > > The configuration is updated using a proprietary AT-command > (AT+QCFG="usbcfg"). The format of the command is as follows: > AT+QCFG="usbcfg",<vid>,<pid>,<diag>,<nmea>,<at_port>,<modem>,<rmnet>,<adb>. > In other words, you set which interfaces to enable/disable. Based on > my testing, it is only possible to enable/disable diag, rmnet (QMI) > and adb, as well as nmea, at_port and modem together. I.e., it is not > possible to only disable for example nmea. > > If I for example disable diag, then the bInterfaceNumber of nmea > changes from 1 to 0, at from 2 to 1, etc., etc. > > BR, > Kristian > This also becomes a mess for the qmi-wwan driver which has the rmnet/qmi interface hardcoded to 4 so that driver will also need a workaround. Quectel seems to have completely missed the reason why usb id's should be unique and not reused for a different product or a different interface layout, there is already a workaround in qmi-wwan for their previous EC-20 card... My opinion is that the option and qmi-wwan drivers should support EP06 in the factory delivery configuration and not in a configuration the user has selected with a Quectel proprietary AT cmd. Can you give some good reason for disabling an interface instead of letting it stay but not use it if you don't need it? Thanks /Lars
On Tue, Sep 11, 2018 at 4:00 PM Lars Melin <larsm17@gmail.com> wrote: > This also becomes a mess for the qmi-wwan driver which has the rmnet/qmi > interface hardcoded to 4 so that driver will also need a workaround. > Quectel seems to have completely missed the reason why usb id's should > be unique and not reused for a different product or a different > interface layout, there is already a workaround in qmi-wwan for their > previous EC-20 card... A patch has already been submitted to work around the issue in qmi driver. > My opinion is that the option and qmi-wwan drivers should support EP06 > in the factory delivery configuration and not in a configuration the > user has selected with a Quectel proprietary AT cmd. > Can you give some good reason for disabling an interface instead of > letting it stay but not use it if you don't need it? Yes. I am working with two boards, one based on MT7621 and the other on MT7623, and I would like to connect two EP06-modems to these boards. However, the USB-controller used on these SoCs has a (very) limited number of USB endpoints. With the default configuration, there are only enough endpoints for one EP06-modem to work properly. By disabling diag, there are enough endpoints for both modems to work correctly. BR, Kristian
On 9/11/2018 21:34, Kristian Evensen wrote: > On Tue, Sep 11, 2018 at 4:00 PM Lars Melin <larsm17@gmail.com> wrote: >> My opinion is that the option and qmi-wwan drivers should support EP06 >> in the factory delivery configuration and not in a configuration the >> user has selected with a Quectel proprietary AT cmd. >> Can you give some good reason for disabling an interface instead of >> letting it stay but not use it if you don't need it? > > Yes. I am working with two boards, one based on MT7621 and the other > on MT7623, and I would like to connect two EP06-modems to these > boards. However, the USB-controller used on these SoCs has a (very) > limited number of USB endpoints. With the default configuration, there > are only enough endpoints for one EP06-modem to work properly. By > disabling diag, there are enough endpoints for both modems to work > correctly. > > BR, > Kristian > You have chosen a platform which has limited usb resources and want to solve that problem by adjusting the device driver? Why don't you just unbind those interfaces which you are not using and which are eating up your usb resources? Thanks /Lars
On Wed, Sep 12, 2018 at 6:32 PM Lars Melin <larsm17@gmail.com> wrote: > You have chosen a platform which has limited usb resources and want to > solve that problem by adjusting the device driver? No, you asked for a good reason for why disabling and not just ignoring an interface makes sense, and I think that supporting multiple EP06 on platforms with limited endpoints qualifies as a reason. My motivation behind this patch and modifying the driver, is to make the driver work with the different options/combinations supported by the modem. The platforms I am working on merely triggered the error and inspired the change. > Why don't you just unbind those interfaces which you are not using and > which are eating up your usb resources? As far as I know, unbinding interfaces from the driver does not free up the memory allocated to the interface by/on the USB controller. I also tried, just in case, and the output from lsusb is the same regardless of bind/unbind. Btw, the patch for the QMI driver has been accepted, since you mentioned that driver earlier. So the assumption about interface four is removed from there. BR, Kristian
On 9/12/2018 23:57, Kristian Evensen wrote: > On Wed, Sep 12, 2018 at 6:32 PM Lars Melin <larsm17@gmail.com> wrote: >> You have chosen a platform which has limited usb resources and want to >> solve that problem by adjusting the device driver? > > No, you asked for a good reason for why disabling and not just > ignoring an interface makes sense, and I think that supporting > multiple EP06 on platforms with limited endpoints qualifies as a > reason. My motivation behind this patch and modifying the driver, is > to make the driver work with the different options/combinations > supported by the modem. The platforms I am working on merely triggered > the error and inspired the change. > >> Why don't you just unbind those interfaces which you are not using and >> which are eating up your usb resources? > > As far as I know, unbinding interfaces from the driver does not free > up the memory allocated to the interface by/on the USB controller. I > also tried, just in case, and the output from lsusb is the same > regardless of bind/unbind. > > Btw, the patch for the QMI driver has been accepted, since you > mentioned that driver earlier. So the assumption about interface four > is removed from there. > > BR, > Kristian > That the patch has qmi-wwan patch has been accepted does not change the fact that you are solving your problems in the wrong end. You are using the OEM re-branding AT cmd to change the interface composition without changing the vid and pid at the same time, this is a big donut and the whole reason for why you have submitted patches which shouldn't be needed. rgds /Lars
On Thu, 2018-09-13 at 01:25 +0700, Lars Melin wrote: > On 9/12/2018 23:57, Kristian Evensen wrote: > > On Wed, Sep 12, 2018 at 6:32 PM Lars Melin <larsm17@gmail.com> > > wrote: > > > You have chosen a platform which has limited usb resources and > > > want to > > > solve that problem by adjusting the device driver? > > > > No, you asked for a good reason for why disabling and not just > > ignoring an interface makes sense, and I think that supporting > > multiple EP06 on platforms with limited endpoints qualifies as a > > reason. My motivation behind this patch and modifying the driver, > > is > > to make the driver work with the different options/combinations > > supported by the modem. The platforms I am working on merely > > triggered > > the error and inspired the change. > > > > > Why don't you just unbind those interfaces which you are not > > > using and > > > which are eating up your usb resources? > > > > As far as I know, unbinding interfaces from the driver does not > > free > > up the memory allocated to the interface by/on the USB controller. > > I > > also tried, just in case, and the output from lsusb is the same > > regardless of bind/unbind. > > > > Btw, the patch for the QMI driver has been accepted, since you > > mentioned that driver earlier. So the assumption about interface > > four > > is removed from there. > > > > BR, > > Kristian > > > > That the patch has qmi-wwan patch has been accepted does not change > the > fact that you are solving your problems in the wrong end. > You are using the OEM re-branding AT cmd to change the interface > composition without changing the vid and pid at the same time, this > is a > big donut and the whole reason for why you have submitted patches > which > shouldn't be needed. The fact that the firmware implementation has the ability to change the endpoints is unrelated to Kristian's case, and that alone is justification for this to be quirked in the driver. People other than Kristian will undoubtedly use the functionality, on platforms less limited. Also most Huawei modems have the ability to change their layout and configuration just like the EP06 via the U2DIAG and SETPORT commands. Dan
Dan Williams <dcbw@redhat.com> writes: > The fact that the firmware implementation has the ability to change the > endpoints is unrelated to Kristian's case, and that alone is > justification for this to be quirked in the driver. People other than > Kristian will undoubtedly use the functionality, on platforms less > limited. FWIW, I agree with Dan and Kristian on this. It's a documented feature, and it will be used. The reasons are irrelevant. The firmware implementation is inconvenient, but we should still strive to make it Just Work in Linux. Kristian's solution does that. > Also most Huawei modems have the ability to change their layout and > configuration just like the EP06 via the U2DIAG and SETPORT commands. Yes, but they are nice enough to use unique class/subclass/protocol triplets for their functions so it's easy to support the changing layout. At least as long as they use their own VID and not some laptop vendor's.. The Sierra Wireless strategy, using fixed interface numbers leaving "holes" is another fine solution to the problem. Or they could have allocated unique VIDs per function combination, as long as the number of valid combinations are low. But they didn't. It's not like it's the first bad firmware design we've had to deal with. Let's just work around it, like we always do. No need to make life difficult for end users just because Quectel makes life difficult for us. Bjørn
On Wed, Sep 12, 2018 at 10:34:43PM +0200, Bjørn Mork wrote: > Dan Williams <dcbw@redhat.com> writes: > > > The fact that the firmware implementation has the ability to change the > > endpoints is unrelated to Kristian's case, and that alone is > > justification for this to be quirked in the driver. People other than > > Kristian will undoubtedly use the functionality, on platforms less > > limited. > > FWIW, I agree with Dan and Kristian on this. It's a documented feature, > and it will be used. The reasons are irrelevant. The firmware > implementation is inconvenient, but we should still strive to make it > Just Work in Linux. Kristian's solution does that. > > > Also most Huawei modems have the ability to change their layout and > > configuration just like the EP06 via the U2DIAG and SETPORT commands. > > Yes, but they are nice enough to use unique class/subclass/protocol > triplets for their functions so it's easy to support the changing > layout. At least as long as they use their own VID and not some laptop > vendor's.. > > The Sierra Wireless strategy, using fixed interface numbers leaving > "holes" is another fine solution to the problem. Or they could have > allocated unique VIDs per function combination, as long as the number of > valid combinations are low. But they didn't. It's not like it's the > first bad firmware design we've had to deal with. Let's just work > around it, like we always do. No need to make life difficult for end > users just because Quectel makes life difficult for us. Ok, thanks for everyone for your input. As Lars I was sceptical about this, but if this is contained to Quectel and we have a solutions which hopefully covers all permutations for their other devices, let's go along with this. I'd prefer seeing this contained in the device-id table as far as possible rather than maintaining a second table of product ids in probe() so I've cooked up a patch (on top of this one) adding a new device-id match flag. Kristian would you mind giving it a try? Oh, also note that I dropped the RSVD(5) for the ADB interface in your patch since it uses a different subclass anyway. I'll submit both patches in a series. Thanks, Johan
Hi Johan, On Thu, Sep 13, 2018 at 11:17 AM Johan Hovold <johan@kernel.org> wrote: > Kristian would you mind giving it a try? Yes, thank you very much. I will give the patches a go during the day today and report back. > Oh, also note that I dropped the RSVD(5) for the ADB interface in your > patch since it uses a different subclass anyway. I'll submit both > patches in a series. Thanks, I completely forgot about that. BR, Kristian
Hello again,
On Thu, Sep 13, 2018 at 11:17 AM Johan Hovold <johan@kernel.org> wrote:
> Kristian would you mind giving it a try?
I just finished backporting + testing your patch with our 4.14-kernel
(mine is already there) and it works great. The driver correctly
handles different EP06-configurations.
Thanks a lot!
BR,
Kristian
On Thu, Sep 13, 2018 at 05:13:02PM +0200, Kristian Evensen wrote: > I just finished backporting + testing your patch with our 4.14-kernel > (mine is already there) and it works great. The driver correctly > handles different EP06-configurations. Great, thanks for testing. Do you mind if I add your tested-by to the patch? Johan
Hi Johan, On Fri, Sep 14, 2018 at 9:51 AM Johan Hovold <johan@kernel.org> wrote: > Great, thanks for testing. Do you mind if I add your tested-by to the > patch? Not at all, go right ahead! I should probably have replied to the patch with a Tested-by. Sorry about forgetting that. BR, Kristian
On Fri, Sep 14, 2018 at 09:53:31AM +0200, Kristian Evensen wrote: > Hi Johan, > On Fri, Sep 14, 2018 at 9:51 AM Johan Hovold <johan@kernel.org> wrote: > > Great, thanks for testing. Do you mind if I add your tested-by to the > > patch? > > Not at all, go right ahead! I should probably have replied to the > patch with a Tested-by. Sorry about forgetting that. No problem at all, I've applied these for 4.19-rc now. Thanks, Johan
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c index 0215b70c4efc..835dcd2875a7 100644 --- a/drivers/usb/serial/option.c +++ b/drivers/usb/serial/option.c @@ -1081,8 +1081,9 @@ static const struct usb_device_id option_ids[] = { .driver_info = RSVD(4) }, { USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_BG96), .driver_info = RSVD(4) }, - { USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EP06), - .driver_info = RSVD(4) | RSVD(5) }, + { USB_DEVICE_AND_INTERFACE_INFO(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EP06, 0xff, 0xff, 0xff), + .driver_info = RSVD(1) | RSVD(2) | RSVD(3) | RSVD(4) | RSVD(5) }, + { USB_DEVICE_AND_INTERFACE_INFO(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EP06, 0xff, 0, 0) }, { USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_6001) }, { USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_CMU_300) }, { USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_6003), @@ -1985,6 +1986,7 @@ static int option_probe(struct usb_serial *serial, { struct usb_interface_descriptor *iface_desc = &serial->interface->cur_altsetting->desc; + struct usb_device_descriptor *dev_desc = &serial->dev->descriptor; unsigned long device_flags = id->driver_info; /* Never bind to the CD-Rom emulation interface */ @@ -1999,6 +2001,17 @@ static int option_probe(struct usb_serial *serial, if (device_flags & RSVD(iface_desc->bInterfaceNumber)) return -ENODEV; + /* + * Don't bind to the QMI device of the Quectel EP06/EG06/EM06. Class, + * subclass and protocol is 0xff for both the diagnostic port and the + * QMI interface, but the diagnostic port only has two endpoints (QMI + * has three). + */ + if (dev_desc->idVendor == cpu_to_le16(QUECTEL_VENDOR_ID) && + dev_desc->idProduct == cpu_to_le16(QUECTEL_PRODUCT_EP06) && + iface_desc->bInterfaceSubClass && iface_desc->bNumEndpoints == 3) + return -ENODEV; + /* Store the device flags so we can use them during attach. */ usb_set_serial_data(serial, (void *)device_flags);
The Quectel EP06 (and EM06/EG06) LTE modem supports updating the USB configuration, without the VID/PID or configuration number changing. When the configuration is updated and interfaces are added/removed, the interface numbers are updated. This causes our current code for matching EP06 not to work as intended, as the assumption about reserved interfaces no longer holds. If for example the diagnostic (first) interface is removed, option will (try to) bind to the QMI interface. This patch improves EP06 detection by replacing the current match with two matches, and those matches check class, subclass and protocol as well as VID and PID. The diag interface exports class, subclass and protocol as 0xff. For the other serial interfaces, class is 0xff and subclass and protocol are both 0x0. The modem can export the following devices and always in this order: diag, nmea, at, ppp. qmi and adb. This means that diag can only ever be interface 0, and interface numbers 1-5 should be marked as reserved. The three other serial devices can have interface numbers 0-3, but I have not marked any interfaces as reserved. The reason is that the serial devices are the only interfaces exported by the device where subclass and protocol is 0x0. QMI exports the same class, subclass and protocol values as the diag interface. However, the two interfaces have different number of endpoints, QMI has three and diag two. I have added a check for number of interfaces if VID/PID matches the EP06, and we ignore the device if number of interfaces equals three (and subclass is set). Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com> --- drivers/usb/serial/option.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)