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 <[email protected]>
---
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);
--
2.14.1
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> Signed-off-by: Kristian Evensen <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> 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 <[email protected]> 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
Allow matching on interfaces having two endpoints by adding a new
device-id flag.
This allows for the handling of devices whose interface numbers can
change (e.g. Quectel EP06) to be contained in the device-id table.
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/option.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 382feafbd127..241a73e172a1 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -561,6 +561,9 @@ static void option_instat_callback(struct urb *urb);
/* Interface is reserved */
#define RSVD(ifnum) ((BIT(ifnum) & 0xff) << 0)
+/* Interface must have two endpoints */
+#define NUMEP2 BIT(16)
+
static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(OPTION_VENDOR_ID, OPTION_PRODUCT_COLT) },
@@ -1082,7 +1085,7 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_BG96),
.driver_info = RSVD(4) },
{ USB_DEVICE_AND_INTERFACE_INFO(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EP06, 0xff, 0xff, 0xff),
- .driver_info = RSVD(1) | RSVD(2) | RSVD(3) | RSVD(4) },
+ .driver_info = RSVD(1) | RSVD(2) | RSVD(3) | RSVD(4) | NUMEP2 },
{ 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) },
@@ -1986,7 +1989,6 @@ 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 */
@@ -2002,16 +2004,11 @@ static int option_probe(struct usb_serial *serial,
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).
+ * Allow matching on bNumEndpoints for devices whose interface numbers
+ * change (e.g. Quectel EP06).
*/
- 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) {
+ if (device_flags & NUMEP2 && iface_desc->bNumEndpoints != 2)
return -ENODEV;
- }
/* Store the device flags so we can use them during attach. */
usb_set_serial_data(serial, (void *)device_flags);
--
2.19.0
From: Kristian Evensen <[email protected]>
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 <[email protected]>
Acked-by: Dan Williams <[email protected]>
[ johan: drop uneeded RSVD(5) for ADB ]
Cc: stable <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/option.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 0215b70c4efc..382feafbd127 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) },
+ { 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,18 @@ 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);
--
2.19.0
Hi Johan,
On Thu, Sep 13, 2018 at 11:17 AM Johan Hovold <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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