Re: [PATCH 1/1] net: usb: qmi_wwan: add Telit 0x1050 composition
Daniele Palmas writes:
> This patch adds support for Telit FN980 0x1050 composition
>
> 0x1050: tty, adb, rmnet, tty, tty, tty, tty
Great! I must admit I have been a bit curious about this since you
submitted the option patch. And I'm still curious about what to expect
from X55 modems in general. There was a lot of discussion about future
modems using PCIe instead of USB etc. I'd appreciate any info you have
on relative performance, quirks, firmware workarounds etc. If you are
allowed to share any of it..
Acked-by: Bjørn Mork"
> please find below usb-devices output
>
> T: Bus=03 Lev=01 Prnt=01 Port=06 Cnt=02 Dev#= 10 Spd=480 MxCh= 0
480 Mbps is a bit slow for this device, isn't it? :-)
I assume you've tested with higher bus speeds? Not that it matters for
this patch. Just curious again.
> D: Ver= 2.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> P: Vendor=1bc7 ProdID=1050 Rev=04.14
> S: Manufacturer=Telit Wireless Solutions
> S: Product=FN980m
> S: SerialNumber=270b8241
> C: #Ifs= 7 Cfg#= 1 Atr=80 MxPwr=500mA
> I: If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30 Driver=option
> I: If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> I: If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan
> I: If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> I: If#= 4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> I: If#= 5 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> I: If#= 6 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
>
> Thanks,
> Daniele
> ---
> drivers/net/usb/qmi_wwan.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 3d77cd402ba9..596428ec71df 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -1327,6 +1327,7 @@ static const struct usb_device_id products[] = {
> {QMI_FIXED_INTF(0x2357, 0x0201, 4)},/* TP-LINK HSUPA Modem MA180 */
> {QMI_FIXED_INTF(0x2357, 0x9000, 4)},/* TP-LINK MA260 */
> {QMI_QUIRK_SET_DTR(0x1bc7, 0x1040, 2)}, /* Telit LE922A */
> + {QMI_QUIRK_SET_DTR(0x1bc7, 0x1050, 2)}, /* Telit FN980 */
> {QMI_FIXED_INTF(0x1bc7, 0x1100, 3)},/* Telit ME910 */
> {QMI_FIXED_INTF(0x1bc7, 0x1101, 3)},/* Telit ME910 dual modem */
> {QMI_FIXED_INTF(0x1bc7, 0x1200, 5)},/* Telit LE920 */
Acked-by: Bjørn Mork
Re: Possible bug with cdc_ether, triggers NETDEV WATCHDOG
Adam Bennett writes: > I've been messing around with a Raspberry Pi Zero, in its ethernet > gadget mode. This possible bug report is not against the Pi Zero > linux kernel, but rather the host computer's linux kernel. I've been > able to reproduce the same host computer issue with my normal laptop, > and an embedded board (buildroot-based). Both run a newish version of > 4.19. The issue is reported on the host, but it's really a problem with the gadget. You'll probably have the same issues with any host, including hosts running something other than Linux. They just won't be as verbose about it. > The bug report is that most of the time I cannot ping through the > local link, and I get a kernel debug message: sometimes I can ping > the Pi Zero with no kernel message, most of the time I can't ping and > the message comes up, and occasionally I get the message right when I > plug in the Pi Zero, before I issue the ping command. > > Here is the dmesg on my normal laptop (I've included the plug-in > sequence also): > > [11728.029900] usb 1-1: new high-speed USB device number 10 using xhci_hcd > [11728.434200] usb 1-1: device descriptor read/64, error -71 First symptom of something wrong with the gadget... > [11728.669543] usb 1-1: New USB device found, idVendor=0525, > idProduct=a4a2, bcdDevice= 4.19 > [11728.669548] usb 1-1: New USB device strings: Mfr=1, Product=2, > SerialNumber=0 > [11728.669551] usb 1-1: Product: RNDIS/Ethernet Gadget > [11728.669554] usb 1-1: Manufacturer: Linux 4.19.75+ with 2098.usb > [11728.674528] cdc_ether 1-1:1.0 usb0: register 'cdc_ether' at > usb-:00:14.0-1, CDC Ethernet Device, 22:93:3a:1e:ac:5c > [11730.725278] cdc_ether 1-1:1.0 enp0s20f0u1: renamed from usb0 > [11768.174915] [ cut here ] > [11768.174921] NETDEV WATCHDOG: enp0s20f0u1 (cdc_ether): transmit > queue 0 timed out This warning means that the gadget doesn't accept the packets we send it. There isn't much the host can do about that, except dropping packets on the floor. Which is why the warning is this loud. Bjørn
Re: Moschip 7703 USB to serial free to a good home
Aaron Thompson writes: > I have a Moschip 7703 USB to single serial port adapter that I'm not > using primarily because it doesn't have an in-tree driver, so I'd be > happy to send it to anyone who would like to add support for it. It > looks like it should be easy to add to the existing mos7720 driver. > Anyone interested? Sorry, not interested. But did you try the obvious fix, even mentioned here under "Moschip MCS7720, MCS7715 driver"?: https://www.kernel.org/doc/Documentation/usb/usb-serial.txt Just add the VID/PID to the device table and see what happens. Just post any error messages or other stuff here, along with the patch you are testing, and you will get help interpreting it. Note that you'll probably have to do a minor change in the mos77xx_calc_num_ports() too, since it hardcodes 2 ports. But if you're lucky then that's it. Bjørn
Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize
Jakub Kicinski writes:
> On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote:
>> Endpoints with zero wMaxPacketSize are not usable for transferring
>> data. Ignore such endpoints when looking for valid in, out and
>> status pipes, to make the drivers more robust against invalid and
>> meaningless descriptors.
>>
>> The wMaxPacketSize of these endpoints are used for memory allocations
>> and as divisors in many usbnet minidrivers. Avoiding zero is therefore
>> critical.
>>
>> Signed-off-by: Bjørn Mork
>
> Fixes tag would be useful. I'm not sure how far into stable we should
> backport this.
That would be commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), so I don't think
a Fixes tag is very useful...
I haven't verified how deep into the code you have been able to get with
wMaxPacketSize being zero. But I don't think there ever has been much
protection since it's so obviously "insane". There was no point in
protecting against this as long as we considered the USB port a security
barrier.
I see that the v2.6.12-rc2 version of drivers/usb/net/usbnet.c (sic)
already had this in it's genelink_tx_fixup():
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1984) // add padding
byte
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1985) if ((skb->len %
dev->maxpacket) == 0)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1986) skb_put
(skb, 1);
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1987)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1988) return skb;
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1989) }
And this in usbnet_start_xmit():
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3564) /* don't assume
the hardware handles USB_ZERO_PACKET
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3565) * NOTE:
strictly conforming cdc-ether devices should expect
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3566) * the ZLP
here, but ignore the one-byte packet.
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3567) *
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3568) * FIXME zero
that byte, if it doesn't require a new skb.
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3569) */
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3570) if ((length %
dev->maxpacket) == 0)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3571)
urb->transfer_buffer_length++;
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3572)
usbnet_probe() calculated dev->maxpacket as
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3826) dev->maxpacket
= usb_maxpacket (dev->udev, dev->out, 1);
without any sanity checking. And usb_maxpacket() hasn't changed much.
It was pretty much the same then as now:
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1123)
usb_maxpacket(struct usb_device *udev, int pipe, int is_out)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1124) {
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1125) struct
usb_host_endpoint*ep;
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1126) unsigned
epnum = usb_pipeendpoint(pipe);
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1127)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1128) if (is_out) {
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1129)
WARN_ON(usb_pipein(pipe));
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1130) ep =
udev->ep_out[epnum];
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1131) } else {
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1132)
WARN_ON(usb_pipeout(pipe));
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1133) ep =
udev->ep_in[epnum];
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1134) }
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1135) if (!ep)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1136) return
0;
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1137)
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1138) /* NOTE: only
0x07ff bits are for packet size... */
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1139) return
le16_to_cpu(ep->desc.wMaxPacketSize);
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1140) }
So, to summarize: I believe the fix is valid for all stable versions.
I'll leave it up to the more competent stable maintainers to decide how
many, if any, it should be backported to. I will not cry if the answer
is none.
> Is this something that occurs on real devices or protection from
> malicious ones?
Only malicious ones AFAICS.
I don't necessarily agree, but I believe the current
Re: Failed to connect to 4G modem
Greg KH writes:
>> >> [ 44.059958] qmi_wwan 1-1:1.3 wwan0: unregister 'qmi_wwan'
>> >> usb-ci_hdrc.1-1, We
..
>> That was always my thought until I tried kernel 5.1 under the same
>> platform (nothing changed except the kernel version), the kernel 5.1
>> can connect to the 4G modem, I could not tell the hardware engineer if
>> it was hardware problem where kernel 5.1 can connect, kernel 4.19
>> could not, how would you explain it? Seems some differences between
>> kernel 5.1 and kernel 4.19, what I could be missing?
>>
>> I cannot use kernel 5, we need kernel LTS on product, too late to wait
>> for 5.4 LTS.
>
> Can you use 'git bisect' to find the commit that fixes the issue? That
> way we can backport it to the 4.19.y tree for you.
Yes, please.
But if I were to guess based on the above info, then I'd start looking
at the chipidea changes. Commit 2c4593ecc920 ("usb: chipidea: host:
override ehci->hub_control") looks particularily interesting.
Bjørn
[PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize
Endpoints with zero wMaxPacketSize are not usable for transferring
data. Ignore such endpoints when looking for valid in, out and
status pipes, to make the drivers more robust against invalid and
meaningless descriptors.
The wMaxPacketSize of these endpoints are used for memory allocations
and as divisors in many usbnet minidrivers. Avoiding zero is therefore
critical.
Signed-off-by: Bjørn Mork
---
drivers/net/usb/usbnet.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 58952a79b05f..dbea2136d901 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -100,6 +100,11 @@ int usbnet_get_endpoints(struct usbnet *dev, struct
usb_interface *intf)
int intr = 0;
e = alt->endpoint + ep;
+
+ /* ignore endpoints which cannot transfer data */
+ if (!usb_endpoint_maxp(&e->desc))
+ continue;
+
switch (e->desc.bmAttributes) {
case USB_ENDPOINT_XFER_INT:
if (!usb_endpoint_dir_in(&e->desc))
--
2.20.1
[PATCH net,stable] cdc_ncm: fix divide-by-zero caused by invalid wMaxPacketSize
Endpoints with zero wMaxPacketSize are not usable for transferring
data. Ignore such endpoints when looking for valid in, out and
status pipes, to make the driver more robust against invalid and
meaningless descriptors.
The wMaxPacketSize of the out pipe is used as divisor. So this change
fixes a divide-by-zero bug.
Reported-by: syzbot+ce366e2b8296e25d8...@syzkaller.appspotmail.com
Signed-off-by: Bjørn Mork
---
#syz test: https://github.com/google/kasan.git f0df5c1b
drivers/net/usb/cdc_ncm.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 50c05d0f44cb..00cab3f43a4c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -681,8 +681,12 @@ cdc_ncm_find_endpoints(struct usbnet *dev, struct
usb_interface *intf)
u8 ep;
for (ep = 0; ep < intf->cur_altsetting->desc.bNumEndpoints; ep++) {
-
e = intf->cur_altsetting->endpoint + ep;
+
+ /* ignore endpoints which cannot transfer data */
+ if (!usb_endpoint_maxp(&e->desc))
+ continue;
+
switch (e->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
case USB_ENDPOINT_XFER_INT:
if (usb_endpoint_dir_in(&e->desc)) {
--
2.20.1
[PATCH net,stable] cdc_ether: fix rndis support for Mediatek based smartphones
A Mediatek based smartphone owner reports problems with USB
tethering in Linux. The verbose USB listing shows a rndis_host
interface pair (e0/01/03 + 10/00/00), but the driver fails to
bind with
[ 355.960428] usb 1-4: bad CDC descriptors
The problem is a failsafe test intended to filter out ACM serial
functions using the same 02/02/ff class/subclass/protocol as RNDIS.
The serial functions are recognized by their non-zero bmCapabilities.
No RNDIS function with non-zero bmCapabilities were known at the time
this failsafe was added. But it turns out that some Wireless class
RNDIS functions are using the bmCapabilities field. These functions
are uniquely identified as RNDIS by their class/subclass/protocol, so
the failing test can safely be disabled. The same applies to the two
types of Misc class RNDIS functions.
Applying the failsafe to Communication class functions only retains
the original functionality, and fixes the problem for the Mediatek based
smartphone.
Tow examples of CDC functional descriptors with non-zero bmCapabilities
from Wireless class RNDIS functions are:
0e8d:000a Mediatek Crosscall Spider X5 3G Phone
CDC Header:
bcdCDC 1.10
CDC ACM:
bmCapabilities 0x0f
connection notifications
sends break
line coding and serial state
get/set/clear comm features
CDC Union:
bMasterInterface0
bSlaveInterface 1
CDC Call Management:
bmCapabilities 0x03
call management
use DataInterface
bDataInterface 1
and
19d2:1023 ZTE K4201-z
CDC Header:
bcdCDC 1.10
CDC ACM:
bmCapabilities 0x02
line coding and serial state
CDC Call Management:
bmCapabilities 0x03
call management
use DataInterface
bDataInterface 1
CDC Union:
bMasterInterface0
bSlaveInterface 1
The Mediatek example is believed to apply to most smartphones with
Mediatek firmware. The ZTE example is most likely also part of a larger
family of devices/firmwares.
Suggested-by: Lars Melin
Signed-off-by: Bjørn Mork
---
drivers/net/usb/cdc_ether.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 8458e88c18e9..32f53de5b1fe 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -206,7 +206,15 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
goto bad_desc;
}
skip:
- if (rndis && header.usb_cdc_acm_descriptor &&
+ /* Communcation class functions with bmCapabilities are not
+* RNDIS. But some Wireless class RNDIS functions use
+* bmCapabilities for their own purpose. The failsafe is
+* therefore applied only to Communication class RNDIS
+* functions. The rndis test is redundant, but a cheap
+* optimization.
+*/
+ if (rndis && is_rndis(&intf->cur_altsetting->desc) &&
+ header.usb_cdc_acm_descriptor &&
header.usb_cdc_acm_descriptor->bmCapabilities) {
dev_dbg(&intf->dev,
"ACM capabilities %02x, not really RNDIS?\n",
--
2.20.1
Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
writes:
>> > What better suggestion do folks have, instead of using
>> USB_REQ_SET_ADDRESS?
>>
>> The spec is clear: wIndex is supposed to be 'NCM Communications Interface'.
>> That's how you address a specific NCM function (a USB device can have more
>> than one...), and that's what you'll see in all the other interface specific
>> class
>> requests in this driver. You don't have to look hard to find examples.
>>
>>
>> Bjørn
>
>
> I have presented what works, with the v3 patch series.
Sure. It will work iff your NCM function has a control interface
numbered 5. Most NCM functions do not.
> Mind you, the code I have provided sends the exact same USB message as
> I traced with Wireshark on my Windows system.
Snooping on communcation with one specific device is not a good way to
figure out dynamic content. wIndex cannot be a constant. It depends on
the device configuration.
>If you can provide good working code that replicates what I have
>provided, I would be thrilled.
There is working control request code a few lines up in the driver. I
didn't think it was too much to ask that you looked it up. But I can
copy an example here too:
static int cdc_ncm_init(struct usbnet *dev)
{
struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
int err;
err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
USB_TYPE_CLASS | USB_DIR_IN
|USB_RECIP_INTERFACE,
0, iface_no, &ctx->ncm_parm,
sizeof(ctx->ncm_parm));
,,
You'll obviously have to replace USB_CDC_GET_NTB_PARAMETERS with
USB_CDC_GET_NET_ADDRESS, &ctx->ncm_parm with buf, and
sizeof(ctx->ncm_parm) with ETH_ALEN.
Bjørn
Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
writes: > What better suggestion do folks have, instead of using USB_REQ_SET_ADDRESS? The spec is clear: wIndex is supposed to be 'NCM Communications Interface'. That's how you address a specific NCM function (a USB device can have more than one...), and that's what you'll see in all the other interface specific class requests in this driver. You don't have to look hard to find examples. Bjørn
Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
writes:
> +static int cdc_ncm_get_ethernet_address(struct usbnet *dev,
> + struct cdc_ncm_ctx *ctx)
Is this function called anywhere? Shouldn't it replace the
usbnet_get_ethernet_addr() call in cdc_ncm_bind_common()?
But do note that cdc_ncm_bind_common() is shared with cdc_mbim and
huawei_cdc_ncm, and that NCM specific code therefore must be
conditional.
That's why the usbnet_get_ethernet_addr() call is currently wrapped in
'if (ctx->ether_desc) {}'. You should definitely not try to do
USB_CDC_GET_NET_ADDRESS or USB_CDC_SET_NET_ADDRESS on MBIM. I don't
know about the Huawei firmwares. But I believe it's best to be careful
and avoid these requests there too. Unless you have a number of
different Huawei devices with assorted firmware revisions for testing.
CDC NCM compliance is obviously not a requirement for their vendor
specific dialect.
> +{
> + int ret;
> + char *buf;
> +
> + buf = kmalloc(ETH_ALEN, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
usbnet_read_cmd() will kmalloc() yet another buffer, so this is
completely pointless. Just use the stack for the 6 byte buffer.
Or let it write directly to dev->net->dev_addr, since you will fall back
to usbnet_get_ethernet_addr() anyway if the request fails.
> + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS,
> + USB_DIR_IN | USB_TYPE_CLASS
> + | USB_RECIP_INTERFACE, 0,
> + USB_REQ_SET_ADDRESS, buf, ETH_ALEN);
Where did that USB_REQ_SET_ADDRESS come from? Did you just look up an
arbutrary macro that happened to match your device config? How do you
expect this to work with a generic NCM device? Or even your own device,
when the next firmware revision moves the function to a different
interface?
It's nice to have USB_CDC_GET_NET_ADDRESS and USB_CDC_GET_NET_ADDRESS
implemented, but there are a large number of NCM devices. You should
probably test the code with one or two other than your own.
Note that few, if any, NCM devices are spec compliant. You should
expect at least one of them to do something really stupid when the see
this optional and unexpected request. I think it would be wise to avoid
sending unsupported requests more than once to a device.
> + if (ret == ETH_ALEN) {
> + memcpy(dev->net->dev_addr, buf, ETH_ALEN);
> + ret = 0;/* success */
> + } else {
> + ret = usbnet_get_ethernet_addr(dev,
> +ctx->ether_desc->iMACAddress);
> + }
> + kfree(buf);
> + return ret;
If you passed dev->net->dev_addr above, then this could be simplified to
if (ret == ETH_ALEN)
return 0;
return usbnet_get_ethernet_addr(dev,..);
> +
> +/* Provide method to push MAC address to the USB device's ethernet
> controller.
> + * If the device does not support CDC_SET_ADDRESS, there is no harm and we
> + * proceed as before.
> + */
> +static int cdc_ncm_set_ethernet_address(struct usbnet *dev,
> + struct sockaddr *addr)
> +{
> + int ret;
> + char *buf;
> +
> + buf = kmalloc(ETH_ALEN, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + memcpy(buf, addr->sa_data, ETH_ALEN);
> + ret = usbnet_write_cmd(dev, USB_CDC_SET_NET_ADDRESS,
> +USB_DIR_OUT | USB_TYPE_CLASS
> +| USB_RECIP_INTERFACE, 0,
> +USB_REQ_SET_ADDRESS, buf, ETH_ALEN);
Same comments as above.
Bjørn
Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
writes:
> + if (strstr(dev->udev->product, "D6000")) {
Huh? Can you please test that on all USB devices ever made?
Bjørn
Re: [RFC 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
writes:
> +static int cdc_ncm_resume (struct usb_interface *intf)
> +{
> + struct usbnet *dev = usb_get_intfdata(intf);
> + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> + int ret;
> +
> + ret = usbnet_resume(intf);
> + if (ret != 0)
> + goto error2;
> +
> + if (ctx->ether_desc) {
> + struct sockaddr sa;
> +
> + if (cdc_ncm_get_ethernet_address(dev, ctx)) {
> + dev_dbg(&intf->dev, "failed to get mac address\n");
> + goto error2;
> + }
> + if (get_acpi_mac_passthru(&intf->dev, &sa) == 0) {
> + if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) !=
> 0) {
> + if (cdc_ncm_set_ethernet_address(dev, &sa) ==
> 0) {
> + memcpy(dev->net->dev_addr, sa.sa_data,
> ETH_ALEN);
> + }
> + }
> + }
> + dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
> + }
> +
> +error2:
> + return ret;
> +}
This is wrong. dev->net->dev_addr will hold the correct address, and it
does not have to match any USB descriptor or ACPI table entry. It's a
user managed address.
You should simply make sure the device is syncronized with the
dev->net->dev_addr field. But you probably want to avoid doing that if
it failed initially. There is no need to issue control requests you
know will fail.
Why the dev_info()? It logs info the user can retrieve with a simple
"ip link" or similar. Seems pretty useless even as a debug statement?
Bjørn
Re: [RFC 4/4] net: cdc_ncm: Add ACPI MAC address pass through functionality
writes:
> @@ -930,11 +931,18 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
> usb_interface *intf, u8 data_
> usb_set_intfdata(ctx->control, dev);
>
> if (ctx->ether_desc) {
> + struct sockaddr sa;
> +
> temp = usbnet_get_ethernet_addr(dev,
> ctx->ether_desc->iMACAddress);
> if (temp) {
> dev_dbg(&intf->dev, "failed to get mac address\n");
> goto error2;
> }
> + if (get_acpi_mac_passthru(&intf->dev, &sa) == 0) {
> + memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
> + if (usbnet_set_ethernet_addr(dev) < 0)
> + usbnet_get_ethernet_addr(dev,
> ctx->ether_desc->iMACAddress);
> + }
> dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
> }
So you want to run a Dell specific ACPI method every time anyone plugs
some NCM class device into a host supporing ACPI? That's going to annoy
the hell out of 99.9997% of the x86, ia64 and arm64 users of this
driver.
Call ACPI once when the driver loads, and only if running on an actual
Dell system where this method is supported. There must be some ACPI
device ID you can match on to know if this method is supported or not?
Bjørn
Re: [RFC 2/3] ACPI: move ACPI functionality out of r8152 driver
writes: > This change moves ACPI functionality out of the Realtek r8152 driver to > its own source and header file, making it available to other drivers as > needed now and into the future. At the time this ACPI snippet was > introduced in 2016, only the Realtek driver made use of it in support of > Dell's enterprise IT policy efforts. There comes now a need for this > same support in a different driver, also in support of Dell's enterprise > IT policy efforts. Why not make a standalone driver out of this, making the MAC address (and other system specifc objects?) available to userspace? Then you can just distribute updated udev rules or systemd units or whatever for the next docking product. I don't think system specific policies should be put into device drivers. Users will combine systems and devices in ways you don't foresee, and may have good reasons to want some non-default policy even for supported combinations. If you really want to have this policy in the driver(s), then please consider extending eth_platform_get_mac_address() with an x86/acpi method. This will make the device driver code support fetching the mac address from device tree and Sparc idproms too. Provided the netdev folks things this is OK, of course. This needs to be discussed there, like get_maintainer.pl would have told you. Making sure we can modify the MAC address of USB ethernet devices is obviously a good thing regardless of how/where you fetch it. Bjørn
Re: [RFC 3/4] Move ACPI functionality out of r8152 driver
writes: > This change moves ACPI functionality out of the Realtek r8152 driver to > its own source and header file, making it available to other drivers as > needed now and into the future. At the time this ACPI snippet was > introduced in 2016, only the Realtek driver made use of it in support of > Dell's enterprise IT policy efforts. There comes now a need for this > same support in a different driver, also in support of Dell's enterprise > IT policy efforts. Yes, and we told you so. Bjørn
Re: WARNING in wdm_write/usb_submit_urb
Oliver Neukum writes: > + wait_event(desc->wait, > + /* > + * needs both flags. We cannot do with one > + * because resetting it would cause a race > + * with write() yet we need to signal > + * a disconnect > + */ > + !test_bit(WDM_IN_USE, &desc->flags) && > + test_bit(WDM_DISCONNECTING, &desc->flags)); I'm confused now... Won't this condition always be false? Should be wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags) || test_bit(WDM_DISCONNECTING, &desc->flags)); instead, or? Bjørn
Re: WARNING in wdm_write/usb_submit_urb
Oliver Neukum writes:
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 1656f5155ab8..a341081a5f47 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
> {
> struct wdm_device *desc = file->private_data;
>
> - wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> + wait_event(desc->wait,
> + /*
> + * needs both flags. We cannot do with one
> + * because resetting it would cause a race
> + * with write() yet we need to signal
> + * a disconnect
> + */
> + !test_bit(WDM_IN_USE, &desc->flags) &&
> + !test_bit(WDM_DISCONNECTING, &desc->flags));
Makes sense. But isn't the WDM_DISCONNECTING test inverted?
> /* cannot dereference desc->intf if WDM_DISCONNECTING */
> if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
> dev_err(&desc->intf->dev, "Error in flush path: %d\n",
> desc->werr);
>
> - return usb_translate_errors(desc->werr);
> + return test_bit(WDM_DISCONNECTING, &desc->flags) ?
> + -ENODEV :
> + usb_translate_errors(desc->werr);
> }
Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
here. How about
if (test_bit(WDM_DISCONNECTING, &desc->flags))
return -ENODEV;
if (desc->werr < 0)
dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);
return usb_translate_errors(desc->werr);
> static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
> @@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
> spin_lock_irqsave(&desc->iuspin, flags);
> set_bit(WDM_DISCONNECTING, &desc->flags);
> set_bit(WDM_READ, &desc->flags);
> - /* to terminate pending flushes */
> - clear_bit(WDM_IN_USE, &desc->flags);
> spin_unlock_irqrestore(&desc->iuspin, flags);
> wake_up_all(&desc->wait);
> mutex_lock(&desc->rlock);
Yes, this looks much better.
Bjørn
[PATCH net,stable] qmi_wwan: Fix out-of-bounds read
The syzbot reported
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113
print_address_description+0x67/0x231 mm/kasan/report.c:188
__kasan_report.cold+0x1a/0x32 mm/kasan/report.c:317
kasan_report+0xe/0x20 mm/kasan/common.c:614
qmi_wwan_probe+0x342/0x360 drivers/net/usb/qmi_wwan.c:1417
usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
really_probe+0x281/0x660 drivers/base/dd.c:509
driver_probe_device+0x104/0x210 drivers/base/dd.c:670
__device_attach_driver+0x1c2/0x220 drivers/base/dd.c:777
bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
Caused by too many confusing indirections and casts.
id->driver_info is a pointer stored in a long. We want the
pointer here, not the address of it.
Thanks-to: Hillf Danton
Reported-by: syzbot+b68605d7fadd21510...@syzkaller.appspotmail.com
Cc: Kristian Evensen
Fixes: e4bf63482c30 ("qmi_wwan: Add quirk for Quectel dynamic config")
Signed-off-by: Bjørn Mork
---
The bug was introduced in v5.2-rc1 but has been backported to stable kernels.
So this fix also needs to go into stable.
drivers/net/usb/qmi_wwan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index d080f8048e52..8b4ad10cf940 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -1482,7 +1482,7 @@ static int qmi_wwan_probe(struct usb_interface *intf,
* different. Ignore the current interface if the number of endpoints
* equals the number for the diag interface (two).
*/
- info = (void *)&id->driver_info;
+ info = (void *)id->driver_info;
if (info->data & QMI_WWAN_QUIRK_QUECTEL_DYNCFG) {
if (desc->bNumEndpoints == 2)
--
2.11.0
[PATCH] USB: serial: option: add Olicard 600
This is a Qualcomm based device with a QMI function on interface 4.
It is mode switched from 2020:2030 using a standard eject message.
T: Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 6 Spd=480 MxCh= 0
D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
P: Vendor=2020 ProdID=2031 Rev= 2.32
S: Manufacturer=Mobile Connect
S: Product=Mobile Connect
S: SerialNumber=0123456789ABCDEF
C:* #Ifs= 6 Cfg#= 1 Atr=80 MxPwr=500mA
I:* If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
E: Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E: Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none)
E: Ad=83(I) Atr=03(Int.) MxPS= 10 Ivl=32ms
E: Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E: Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none)
E: Ad=85(I) Atr=03(Int.) MxPS= 10 Ivl=32ms
E: Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E: Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none)
E: Ad=87(I) Atr=03(Int.) MxPS= 10 Ivl=32ms
E: Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E: Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
E: Ad=89(I) Atr=03(Int.) MxPS= 8 Ivl=32ms
E: Ad=88(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E: Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 5 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=(none)
E: Ad=8a(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E: Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=125us
Cc: sta...@vger.kernel.org
Signed-off-by: Bjørn Mork
---
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 11b21d9410f3..02ee5b707de2 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -1943,6 +1943,8 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3e01, 0xff, 0xff, 0xff) }, /*
D-Link DWM-152/C1 */
{ USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3e02, 0xff, 0xff, 0xff) }, /*
D-Link DWM-156/C1 */
{ USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x7e11, 0xff, 0xff, 0xff) }, /*
D-Link DWM-156/A3 */
+ { USB_DEVICE_INTERFACE_CLASS(0x2020, 0x2031, 0xff),
/* Olicard 600 */
+ .driver_info = RSVD(4) },
{ USB_DEVICE_INTERFACE_CLASS(0x2020, 0x4000, 0xff) },/*
OLICARD300 - MT6225 */
{ USB_DEVICE(INOVIA_VENDOR_ID, INOVIA_SEW858) },
{ USB_DEVICE(VIATELECOM_VENDOR_ID, VIATELECOM_PRODUCT_CDS7) },
--
2.11.0
Re: PROBLEM: HUAWEI E3531 HSPA+ USB Stick "please report the device ID to the Linux USB developers"
Peter Schüller writes: > [corrected email of Kai-Heng Feng] > > Am Mo., 18. März 2019 um 15:39 Uhr schrieb Dan Williams : >> [removing netdev list...] >> >> With the modem plugged in, could you grab the output of: >> >> lsusb -v -d 12d1:14dc > > The output is on STDERR > > Couldn't open device, some information will be missing > > and on STDOUT > > Bus 001 Device 009: ID 12d1:14dc Huawei Technologies Co., Ltd. > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass2 Communications > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize064 > idVendor 0x12d1 Huawei Technologies Co., Ltd. > idProduct 0x14dc > bcdDevice1.02 > iManufacturer 1 > iProduct2 > iSerial 0 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 94 > bNumInterfaces 3 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0x80 > (Bus Powered) > MaxPower 500mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 1 > bInterfaceClass 2 Communications > bInterfaceSubClass 6 Ethernet Networking > bInterfaceProtocol 0 > iInterface 5 > CDC Header: > bcdCDC 1.10 > CDC Union: > bMasterInterface0 > bSlaveInterface 1 > CDC Ethernet: > iMacAddress 7 (??) > bmEthernetStatistics0x > wMaxSegmentSize 1514 > wNumberMCFilters0x > bNumberPowerFilters 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x83 EP 3 IN > bmAttributes3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0010 1x 16 bytes > bInterval 9 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber1 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass10 CDC Data > bInterfaceSubClass 6 > bInterfaceProtocol 0 > iInterface 6 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x82 EP 2 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x02 EP 2 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber2 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 8 Mass Storage > bInterfaceSubClass 6 SCSI > bInterfaceProtocol 80 Bulk-Only > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x84 EP 4 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x03 EP 3 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0200 1x 512 bytes > bInterval 1 OK, so there are only two class functions in that mode: Ethernet and Storage. There should be no need to add anything anywhere. The proper class drivers, cdc_ether and usb-storage, will load and probe these functions based on the class/subclass/protocol values. And your log show that this works as expected: Mar 17 14:06:32 gigabyte
Re: [PATCH] qmi_wwan: apply SET_DTR quirk to Sierra WP7607
Beniamino Galvani writes: > The 1199:68C0 USB ID is reused by Sierra WP7607 which requires the DTR > quirk to be detected. Apply QMI_QUIRK_SET_DTR unconditionally as > already done for other IDs shared between different devices. > > Signed-off-by: Beniamino Galvani Thanks. Looks like it's time to consider applying this unconditionally to all devices. Feel free to propose something like that the next time this issue comes up. Acked-by: Bjørn Mork
[PATCH] cdc_ether: trivial whitespace readability fix
This function is unreadable enough without indenting mismatches
and unnecessary line breaks.
Signed-off-by: Bjørn Mork
---
drivers/net/usb/cdc_ether.c | 26 +++---
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index b3b3c05903a1..3305f23793c7 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -179,10 +179,8 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
* probed with) and a slave/data interface; union
* descriptors sort this all out.
*/
- info->control = usb_ifnum_to_if(dev->udev,
- info->u->bMasterInterface0);
- info->data = usb_ifnum_to_if(dev->udev,
- info->u->bSlaveInterface0);
+ info->control = usb_ifnum_to_if(dev->udev, info->u->bMasterInterface0);
+ info->data = usb_ifnum_to_if(dev->udev, info->u->bSlaveInterface0);
if (!info->control || !info->data) {
dev_dbg(&intf->dev,
"master #%u/%p slave #%u/%p\n",
@@ -216,18 +214,16 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
/* a data interface altsetting does the real i/o */
d = &info->data->cur_altsetting->desc;
if (d->bInterfaceClass != USB_CLASS_CDC_DATA) {
- dev_dbg(&intf->dev, "slave class %u\n",
- d->bInterfaceClass);
+ dev_dbg(&intf->dev, "slave class %u\n", d->bInterfaceClass);
goto bad_desc;
}
skip:
- if (rndis &&
- header.usb_cdc_acm_descriptor &&
- header.usb_cdc_acm_descriptor->bmCapabilities) {
- dev_dbg(&intf->dev,
- "ACM capabilities %02x, not really RNDIS?\n",
- header.usb_cdc_acm_descriptor->bmCapabilities);
- goto bad_desc;
+ if (rndis && header.usb_cdc_acm_descriptor &&
+ header.usb_cdc_acm_descriptor->bmCapabilities) {
+ dev_dbg(&intf->dev,
+ "ACM capabilities %02x, not really RNDIS?\n",
+ header.usb_cdc_acm_descriptor->bmCapabilities);
+ goto bad_desc;
}
if (header.usb_cdc_ether_desc && info->ether->wMaxSegmentSize) {
@@ -238,7 +234,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
}
if (header.usb_cdc_mdlm_desc &&
- memcmp(header.usb_cdc_mdlm_desc->bGUID, mbm_guid, 16)) {
+ memcmp(header.usb_cdc_mdlm_desc->bGUID, mbm_guid, 16)) {
dev_dbg(&intf->dev, "GUID doesn't match\n");
goto bad_desc;
}
@@ -302,7 +298,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
if (info->control->cur_altsetting->desc.bNumEndpoints == 1) {
struct usb_endpoint_descriptor *desc;
- dev->status = &info->control->cur_altsetting->endpoint [0];
+ dev->status = &info->control->cur_altsetting->endpoint[0];
desc = &dev->status->desc;
if (!usb_endpoint_is_int_in(desc) ||
(le16_to_cpu(desc->wMaxPacketSize)
--
2.11.0
Re: Support Mac address pass through on Dell DS1000 dock
Frédéric Parrenin writes: > Le 26/11/2018 à 15:21, mario.limoncie...@dell.com a écrit : >> >> Any feedback from testing with this debugging patch to identify what's >> happening? > > This is super strange, but Mac address pass-through is now working in > my configuration, although I did not change anything. > The network is up and running and I have the right Mac Address > > For your information, I recompile the kernel and this is the message I > got in /sys/kernel/debug/dynamic_debug/control: > > drivers/net/usb/r8152.c:5149 [r8152]rtl_get_version =_ "Detected > version 0x%04x\012" > drivers/net/usb/r8152.c:1178 [r8152]vendor_mac_passthru_addr_read =_ > "pass-through bit not set.\012" > drivers/net/usb/r8152.c:1170 [r8152]vendor_mac_passthru_addr_read =_ > "Not an AD type.\012" > > Not sure if it is the information you were looking for (sorry, I am > new in kernel debugging). /sys/kernel/debug/dynamic_debug/control lists all the *available* debug messages. You still need to enable them. The "=_" in the lines above indicate that they are disabled. E.g. to enable all r8152.c related debug messages prefixed with the function name: echo 'file r8152.c +fp' >/sys/kernel/debug/dynamic_debug/control See https://www.kernel.org/doc/Documentation/admin-guide/dynamic-debug-howto.rst for more info on the dynamic debug feature. Bjørn
Re: Support Mac address pass through on Dell DS1000 dock
writes: > On Nov 22, 2018 03:41, Frédéric Parrenin > wrote: >> >> A guy answered on the amazon page that he has Mac pass-through working >> on windows 10 with this exact same dock station. >> So it really seems to be a linux-only problem. > > Well thanks for checking further. This is quite surprising to me. Maybe it's by accident because the Windows driver is matching too many devices or something? Do you happen know how this feature was implemented in Windows? > You can see in official documentation it's not mentioned: > > https://www.dell.com/support/article/us/en/04/sln301147/what-is-mac-address-pass-through-?lang=en > > I'll look forward to seeing what the debugging patch shows and need to > confir with an extended team about results after holidays. I don't remember if I mentioned this when you created the r8152 implementation, but this feature could have been easily implemented in userspace given a minimalistic ACPI driver to just read the system MAC. This way it would depend only on the Dell BIOS support, and not on some specific dock. It would even work with third party dongles etc. Having it in userspace would also have made it easier to implement an end user specific policy scheme. There is no need to restrict ourselves to the limited creativeness of Windows developers ;-) Bjørn
Re: Support Mac address pass through on Dell DS1000 dock
Greg KH writes: > I do not see any USB networking device here at all. No, It wasn't easy to see. But it's there both with and without the feature enabled: /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 5000M | Port 4: Dev 10, If 0, Class=Hub, Driver=hub/7p, 5000M |__ Port 2: Dev 11, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M This feature is only for convenience and should never prevent the network from working. Unless the other end is confused by seeing the same mac address on two ports... Better take down or disconnect the undocked interface after docking. Bjørn
Re: [PATCH 2/8] usbnet: smsc95xx: add kconfig for turbo mode
Ben Dooks writes: > Add a configuration option for the default state of turbo mode > on the smsc95xx networking driver. Some systems it is better > to default this to off as it causes significant increases in > soft-irq load. So there is already a module option allowing you to change this, using e.g kernel command line or kmod config files. It's even writable, taking effect on the next netdev open, so you can change it at runtime without reloading the module. What good does this new build-time setting do, except causing confusion wrt driver defaults? Note also that the smsc95xx and smsc75xx drivers are pretty similar. Both have the same turbo_mode setting. If you change the defaults, then they should at least be kept in sync to cause as little confusion as possible.. Bjørn
Re: [PATCH 4/7] net: qmi_wwan: add usbnet -> priv function
Ben Dooks writes:
> +static inline struct qmi_wwan_state *usbnet_to_qmi(struct usbnet *usb)
> +{
> + return (void *) &usb->data;
> +}
checkpatch doesn't like this, and it is also inconsistent with the
coding style elsewhere in the driver.
CHECK: No space is necessary after a cast
#30: FILE: drivers/net/usb/qmi_wwan.c:81:
+ return (void *) &usb->data;
total: 0 errors, 0 warnings, 1 checks, 115 lines checked
And as for consistency: I realize that "dev" is a very generic name,
but it's used consistendly for all struct usbnet pointers in the driver:
bjorn@miraculix:/usr/local/src/git/linux$ git grep 'struct usbnet'
drivers/net/usb/qmi_wwan.c
drivers/net/usb/qmi_wwan.c:static struct net_device *qmimux_find_dev(struct
usbnet *dev, u8 mux_id)
drivers/net/usb/qmi_wwan.c:static bool qmimux_has_slaves(struct usbnet *dev)
drivers/net/usb/qmi_wwan.c:static int qmimux_rx_fixup(struct usbnet *dev,
struct sk_buff *skb)
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(net);
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_rx_fixup(struct usbnet *dev,
struct sk_buff *skb)
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_manage_power(struct usbnet *dev,
int on)
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_register_subdriver(struct usbnet
*dev)
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_change_dtr(struct usbnet *dev,
bool on)
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_bind(struct usbnet *dev, struct
usb_interface *intf)
drivers/net/usb/qmi_wwan.c: BUILD_BUG_ON((sizeof(((struct usbnet
*)0)->data) <
drivers/net/usb/qmi_wwan.c:static void qmi_wwan_unbind(struct usbnet *dev,
struct usb_interface *intf)
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
The name was chosen to be consistent with the cdc_ether usage. I'd
prefer it if this function could use the "dev" name, to avoid confusing
things unnecessarly with yet another generic name like "usb". Which
isn't used anywhere else in the driver, and could just as easily refer
to a struct usb_device or struct usb_interface.
(yes, I know there are a couple of "struct usb_device *dev" cases. But
I'd rather see those renamed TBH)
I also don't see why this function should be qmi_wwan specific. No need
to duplicate the same function in every minidriver, when you can do with
two generic helpers (maybe with more meaningful names):
static inline void *usbnet_priv(const struct usbnet *dev)
{
return (void *)&dev->data;
}
static inline void *usbnet_priv0(const struct usbnet * *dev)
{
return (void *)dev->data[0];
}
Please fix the checkpatch warning and consider the other comments.
Personally I don't really think it makes the code any easier to read.
But if you want it, then I'm fine this. I guess I've grown too used to
seeing those casts ;-)
Bjørn
Re: [PATCH net-next 08/19] net: usb: aqc111: Implement TX data path
Igor Russkikh writes:
> +struct aq_tx_packet_desc {
> + struct {
> + u32 length:21;
> + u32 checksum:7;
> + u32 drop_padding:1;
> + u32 vlan_tag:1;
> + u32 cphi:1;
> + u32 dicf:1;
> + };
> + struct {
> + u32 max_seg_size:15;
> + u32 reserved:1;
> + u32 vlan_info:16;
> + };
> +};
You might want to shift and mask instead to avoid going insane when
trying to use this header on a BE system...
Bjørn
Re: [PATCH net-next 09/19] net: usb: aqc111: Implement RX data path
Igor Russkikh writes: > From: Dmitry Bezrukov > > Signed-off-by: Dmitry Bezrukov > Signed-off-by: Igor Russkikh > --- You'd want some description here. Bjørn
Re: [PATCH net-next 01/19] net: usb: aqc111: Driver skeleton for Aquantia AQtion USB to 5GbE
Igor Russkikh writes:
>> +static const struct driver_info aqc111_info = {
> + .description= "Aquantia AQtion USB to 5GbE Controller",
> +};
> +
> +#define AQC111_USB_ETH_DEV(vid, pid, table) \
> + .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> + USB_DEVICE_ID_MATCH_INT_CLASS, \
> + USB_DEVICE(vid, pid), \
> + .bInterfaceClass = USB_CLASS_VENDOR_SPEC, \
> + .driver_info = (unsigned long)&table, \
> +}, \
> +{ \
> + .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> + USB_DEVICE_ID_MATCH_INT_INFO, \
> + USB_DEVICE(vid, pid), \
> + .bInterfaceClass = USB_CLASS_COMM, \
> + .bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \
> + .bInterfaceProtocol = USB_CDC_PROTO_NONE
> +
Is the missing .driver_info for the CDC class intentional? If so, then
why include it at all?
Bjørn
Re: [PATCH net-next 03/19] net: usb: aqc111: Add implementation of read and write commands
Igor Russkikh writes:
> +static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> + u16 index, u16 size, void *data, int nopm)
> +{
> + int ret;
> + int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
> + u16 index, void *data, u16 size);
> +
> + if (nopm)
> + fn = usbnet_read_cmd_nopm;
> + else
> + fn = usbnet_read_cmd;
> +
> + ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + value, index, data, size);
> + if (size == 2)
> + le16_to_cpus(data);
> +
> + if (unlikely(ret < 0))
> + netdev_warn(dev->net,
> + "Failed to read(0x%x) reg index 0x%04x: %d\n",
> + cmd, index, ret);
> + return ret;
> +}
> +
> +static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> + u16 index, u16 size, void *data)
> +{
> + return __aqc111_read_cmd(dev, cmd, value, index, size, data, 1);
> +}
> +
> +static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +u16 index, u16 size, void *data)
> +{
> + return __aqc111_read_cmd(dev, cmd, value, index, size, data, 0);
> +}
> +
Why would you want to do something like this instead of simply
implementing aqc111_read_cmd_nopm() and aqc111_read_cmd() as separate
functions? The function pointer stuff is incredibly ugly, as Oliver
pointed out. It wasn't done like that in usbnet.c, so why should we do
it like that here?
And the "if (size == 2) le16_to_cpus(data)" looks like something that
will come back and haunt you. Will this code never read larger
integers? Maybe add some sanity checks then, just in case...
Or simply add more helpers. An additional pair of helpers for reading
16bit integers might simplify your code quite a bit.
Bjørn
Re: [PATCH net-next 04/19] net: usb: aqc111: Various callbacks implementation
Oliver Neukum writes:
> On Fr, 2018-10-05 at 10:24 +, Igor Russkikh wrote:
>> From: Dmitry Bezrukov
>>
>> Reset, stop callbacks, driver unbind callback.
>> More register defines required for these callbacks.
>>
>> Signed-off-by: Dmitry Bezrukov
>> Signed-off-by: Igor Russkikh
>> ---
>> drivers/net/usb/aqc111.c | 48 ++
>> drivers/net/usb/aqc111.h | 101
>> +++
>> 2 files changed, 149 insertions(+)
>>
>> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
>> index 7f3e5a615750..22bb259d71fb 100644
>> --- a/drivers/net/usb/aqc111.c
>> +++ b/drivers/net/usb/aqc111.c
>> @@ -169,12 +169,60 @@ static int aqc111_bind(struct usbnet *dev, struct
>> usb_interface *intf)
>>
>> static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
>> {
>> +u8 reg8;
>> +u16 reg16;
>> +
>> +/* Force bz */
>> +reg16 = SFR_PHYPWR_RSTCTL_BZ;
>> +aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL,
>> + 2, 2, ®16);
>
> No, I am sorry, you are doing DMA on the kernel stack. That is not
> allowed. These functions will all have to be fixed.
Huh? No, he doesn't. That's the whole point with
usbnet_read_cmd_nopm(), isn't it?
Bjørn
[PATCH v4] usb: export firmware port location in sysfs
The platform firmware "location" data is used to find port peer
relationships. But firmware is an unreliable source, and there are
real world examples of errors leading to missing or wrong peer
relationships. Debugging this is currently hard.
Exporting the location attribute makes it easier to spot mismatches
between the firmware data and the real world.
Signed-off-by: Bjørn Mork
---
v4: remove unrelated ABI doc changes, thanks to Alan Stern for noticing
v3: resurrect missing hunk, as pointed out by the kbuild test robot
v2: Sorry, forgot to rebase the old branch before sending v1
This patch got stuck in one of my debugging branches. It proved very
useful to me while trying to figure out why the "peer" link was useless
on a specific host. And if it was useful to me once, then maybe it will
be to someone else as well...
tl;dr; The full use case for anyone interested:
Some current LTE modems with USB3 SS support come with a bootloader
supporting USB2 only. The application and bootloader modes are provided
by different softwares running on the modem (which of course is just
another Linux system with a UDC). None of the descriptors are therefore
guaranteed to be identical, or even similar.
Doing a firmware upgrade of such a device involves
- some preparation in application mode (USB3),
- rebooting into bootloader mode (USB2), and
- finally booting into the upgraded application firmware (USB3) to verify
the upgrade.
The firmware upgrade tool should make sure it talks to the same physical
device in all three phases. The only semi-reliable way to do that is to
look for "new" devices in the expected mode, connected to the same physical
USB port. But the locical port will change due to the USB2/3 switch, and
all we are left with is the "peer" link. Which can, and do, fail due to
buggy ACPI tables.
This patch won't solve that problem, but it makes it a lot easier to
detect.
Lesson for the next time: Either submit rightaway or just delete it.
Three attempts to get this even building without a warning is more
than embarrassing. Sorry about that. It *is* build tested now. And
even run tested. Honestly ;-)
Documentation/ABI/testing/sysfs-bus-usb | 10 ++
drivers/usb/core/port.c | 10 ++
2 files changed, 20 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-usb
b/Documentation/ABI/testing/sysfs-bus-usb
index 08d456e07b53..7ada65345251 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,16 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
+What: /sys/bus/usb/devices/.../(hub interface)/portX/location
+Date: October 2018
+Contact: Bjørn Mork
+Description:
+ Some platforms provide usb port physical location through
+ firmware. This is used by the kernel to pair up logical ports
+ mapping to the same physical connector. The attribute exposes
the
+ raw location value as a hex integer.
+
+
What: /sys/bus/usb/devices/.../(hub interface)/portX/quirks
Date: May 2018
Contact: Nicolas Boichat
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 4a2143195395..1a06a4b5fbb1 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -16,6 +16,15 @@ static int usb_port_block_power_off;
static const struct attribute_group *port_dev_group[];
+static ssize_t location_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ return sprintf(buf, "0x%08x\n", port_dev->location);
+}
+static DEVICE_ATTR_RO(location);
+
static ssize_t connect_type_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -140,6 +149,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
static struct attribute *port_dev_attrs[] = {
&dev_attr_connect_type.attr,
+ &dev_attr_location.attr,
&dev_attr_quirks.attr,
&dev_attr_over_current_count.attr,
NULL,
--
2.11.0
Re: [PATCH v3] usb: export firmware port location in sysfs
Alan Stern writes: >> diff --git a/Documentation/ABI/testing/sysfs-bus-usb >> b/Documentation/ABI/testing/sysfs-bus-usb >> index 08d456e07b53..8f394c976fee 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-usb >> +++ b/Documentation/ABI/testing/sysfs-bus-usb >> @@ -173,14 +173,14 @@ Description: >> The file will be present for all speeds of USB devices, and will >> always read "no" for USB 1.1 and USB 2.0 devices. >> >> -What: /sys/bus/usb/devices/.../(hub interface)/portX >> +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX > > This name change doesn't have anything to do with the purpose of the > patch. It also doesn't make any sense, and in fact it isn't actually > implemented in the patch. Yuck. You're right of course. Will fix. Bjørn
[PATCH v3] usb: export firmware port location in sysfs
The platform firmware "location" data is used to find port peer
relationships. But firmware is an unreliable source, and there are
real world examples of errors leading to missing or wrong peer
relationships. Debugging this is currently hard.
Exporting the location attribute makes it easier to spot mismatches
between the firmware data and the real world.
Signed-off-by: Bjørn Mork
---
v3: resurrect missing hunk, as pointed out by the kbuild test robot
v2: Sorry, forgot to rebase the old branch before sending v1
This patch got stuck in one of my debugging branches. It proved very
useful to me while trying to figure out why the "peer" link was useless
on a specific host. And if it was useful to me once, then maybe it will
be to someone else as well...
tl;dr; The full use case for anyone interested:
Some current LTE modems with USB3 SS support come with a bootloader
supporting USB2 only. The application and bootloader modes are provided
by different softwares running on the modem (which of course is just
another Linux system with a UDC). None of the descriptors are therefore
guaranteed to be identical, or even similar.
Doing a firmware upgrade of such a device involves
- some preparation in application mode (USB3),
- rebooting into bootloader mode (USB2), and
- finally booting into the upgraded application firmware (USB3) to verify
the upgrade.
The firmware upgrade tool should make sure it talks to the same physical
device in all three phases. The only semi-reliable way to do that is to
look for "new" devices in the expected mode, connected to the same physical
USB port. But the locical port will change due to the USB2/3 switch, and
all we are left with is the "peer" link. Which can, and do, fail due to
buggy ACPI tables.
This patch won't solve that problem, but it makes it a lot easier to
detect.
Lesson for the next time: Either submit rightaway or just delete it.
Three attempts to get this even building without a warning is more
than embarrassing. Sorry about that. It *is* build tested now. And
even run tested. Honestly ;-)
Documentation/ABI/testing/sysfs-bus-usb | 18 ++
drivers/usb/core/port.c | 10 ++
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-usb
b/Documentation/ABI/testing/sysfs-bus-usb
index 08d456e07b53..8f394c976fee 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -173,14 +173,14 @@ Description:
The file will be present for all speeds of USB devices, and will
always read "no" for USB 1.1 and USB 2.0 devices.
-What: /sys/bus/usb/devices/.../(hub interface)/portX
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX
Date: August 2012
Contact: Lan Tianyu
Description:
The /sys/bus/usb/devices/.../(hub interface)/portX
is usb port device's sysfs directory.
-What: /sys/bus/usb/devices/.../(hub interface)/portX/connect_type
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/connect_type
Date: January 2013
Contact: Lan Tianyu
Description:
@@ -189,7 +189,17 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
-What: /sys/bus/usb/devices/.../(hub interface)/portX/quirks
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/location
+Date: October 2018
+Contact: Bjørn Mork
+Description:
+ Some platforms provide usb port physical location through
+ firmware. This is used by the kernel to pair up logical ports
+ mapping to the same physical connector. The attribute exposes
the
+ raw location value as a hex integer.
+
+
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/quirks
Date: May 2018
Contact: Nicolas Boichat
Description:
@@ -211,7 +221,7 @@ Description:
used to help make enumeration work better on some high speed
devices.
-What: /sys/bus/usb/devices/.../(hub
interface)/portX/over_current_count
+What: /sys/bus/usb/devices/.../(hub
interface)/usbY-portX/over_current_count
Date: February 2018
Contact: Richard Leitner
Description:
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 4a2143195395..1a06a4b5fbb1 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -16,6 +16,15 @@ static int usb_port_block_power_off;
static const struct attribute_group *port_dev_group[];
+static ssize_t location_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct usb_port *port_dev = to_usb
[PATCH v2] usb: export firmware port location in sysfs
The platform firmware "location" data is used to find port peer
relationships. But firmware is an unreliable source, and there are
real world examples of errors leading to missing or wrong peer
relationships. Debugging this is currently hard.
Exporting the location attribute makes it easier to spot mismatches
between the firmware data and the real world.
Signed-off-by: Bjørn Mork
---
v2: Sorry, forgot to rebase the old branch before sending v1
This patch got stuck in one of my debugging branches. It proved very
useful to me while trying to figure out why the "peer" link was useless
on a specific host. And if it was useful to me once, then maybe it will
be to someone else as well...
tl;dr; The full use case for anyone interested:
Some current LTE modems with USB3 SS support come with a bootloader
supporting USB2 only. The application and bootloader modes are provided
by different softwares running on the modem (which of course is just
another Linux system with a UDC). None of the descriptors are therefore
guaranteed to be identical, or even similar.
Doing a firmware upgrade of such a device involves
- some preparation in application mode (USB3),
- rebooting into bootloader mode (USB2), and
- finally booting into the upgraded application firmware (USB3) to verify
the upgrade.
The firmware upgrade tool should make sure it talks to the same physical
device in all three phases. The only semi-reliable way to do that is to
look for "new" devices in the expected mode, connected to the same physical
USB port. But the locical port will change due to the USB2/3 switch, and
all we are left with is the "peer" link. Which can, and do, fail due to
buggy ACPI tables.
This patch won't solve that problem, but it makes it a lot easier to
detect.
Documentation/ABI/testing/sysfs-bus-usb | 18 ++
drivers/usb/core/port.c | 9 +
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-usb
b/Documentation/ABI/testing/sysfs-bus-usb
index 08d456e07b53..8f394c976fee 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -173,14 +173,14 @@ Description:
The file will be present for all speeds of USB devices, and will
always read "no" for USB 1.1 and USB 2.0 devices.
-What: /sys/bus/usb/devices/.../(hub interface)/portX
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX
Date: August 2012
Contact: Lan Tianyu
Description:
The /sys/bus/usb/devices/.../(hub interface)/portX
is usb port device's sysfs directory.
-What: /sys/bus/usb/devices/.../(hub interface)/portX/connect_type
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/connect_type
Date: January 2013
Contact: Lan Tianyu
Description:
@@ -189,7 +189,17 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
-What: /sys/bus/usb/devices/.../(hub interface)/portX/quirks
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/location
+Date: October 2018
+Contact: Bjørn Mork
+Description:
+ Some platforms provide usb port physical location through
+ firmware. This is used by the kernel to pair up logical ports
+ mapping to the same physical connector. The attribute exposes
the
+ raw location value as a hex integer.
+
+
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/quirks
Date: May 2018
Contact: Nicolas Boichat
Description:
@@ -211,7 +221,7 @@ Description:
used to help make enumeration work better on some high speed
devices.
-What: /sys/bus/usb/devices/.../(hub
interface)/portX/over_current_count
+What: /sys/bus/usb/devices/.../(hub
interface)/usbY-portX/over_current_count
Date: February 2018
Contact: Richard Leitner
Description:
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 4a2143195395..47e45d317201 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -16,6 +16,15 @@ static int usb_port_block_power_off;
static const struct attribute_group *port_dev_group[];
+static ssize_t location_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ return sprintf(buf, "0x%08x\n", port_dev->location);
+}
+static DEVICE_ATTR_RO(location);
+
static ssize_t connect_type_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
--
2.11.0
[PATCH] usb: export firmware port location in sysfs
The platform firmware "location" data is used to find port peer
relationships. But firmware is an unreliable source. Errors in ACPI
tables can lead to missing or wrong peer relationships. Debugging
this is currently hard.
Exporting the location attribute makes it easier to spot mismatches
between the firmware data and the real world.
Signed-off-by: Bjørn Mork
---
This patch got stuck in one of my debugging branches. It proved very
useful to me while trying to figure out why the "peer" link was useless
on a specific host. And if it was useful to me once, then maybe it will
be to someone else as well...
tl;dr; The full use case for anyone interested:
Some current LTE modems with USB3 SS support come with a bootloader
supporting USB2 only. The application and bootloader modes are provided
by different softwares running on the modem (which of course is just
another Linux system with a UDC). None of the descriptors are therefore
guaranteed to be identical, or even similar.
Doing a firmware upgrade of such a device involves
- some preparation in application mode (USB3),
- rebooting into bootloader mode (USB2), and
- finally booting into the upgraded application firmware (USB3) to verify
the upgrade.
The firmware upgrade tool should make sure it talks to the same physical
device in all three phases. The only semi-reliable way to do that is to
look for "new" devices in the expected mode, connected to the same physical
USB port. But the locical port will change due to the USB2/3 switch, and
all we are left with is the "peer" link. Which can, and do, fail due to
buggy ACPI tables.
This patch won't solve that problem, but it makes it a lot easier to
detect.
Documentation/ABI/testing/sysfs-bus-usb | 15 ---
drivers/usb/core/port.c | 9 +
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-usb
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..738b420d 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -173,14 +173,14 @@ Description:
The file will be present for all speeds of USB devices, and will
always read "no" for USB 1.1 and USB 2.0 devices.
-What: /sys/bus/usb/devices/.../(hub interface)/portX
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX
Date: August 2012
Contact: Lan Tianyu
Description:
The /sys/bus/usb/devices/.../(hub interface)/portX
is usb port device's sysfs directory.
-What: /sys/bus/usb/devices/.../(hub interface)/portX/connect_type
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/connect_type
Date: January 2013
Contact: Lan Tianyu
Description:
@@ -189,7 +189,16 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
-What: /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
+What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/location
+Date: October 2018
+Contact: Bjørn Mork
+Description:
+ Some platforms provide usb port physical location through
+ firmware. This is used by the kernel to pair up logical ports
+ mapping to the same physical connector. The attribute exposes
the
+ raw location value as a hex integer.
+
+What: /sys/bus/usb/devices/.../(hub
interface)/usbY-portX/usb3_lpm_permit
Date: November 2015
Contact: Lu Baolu
Description:
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..baa35ba410b6 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -16,6 +16,15 @@ static int usb_port_block_power_off;
static const struct attribute_group *port_dev_group[];
+static ssize_t location_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ return sprintf(buf, "0x%08x\n", port_dev->location);
+}
+static DEVICE_ATTR_RO(location);
+
static ssize_t connect_type_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
--
2.11.0
[PATCH net,stable] qmi_wwan: set DTR for modems in forced USB2 mode
Recent firmware revisions have added the ability to force
these modems to USB2 mode, hiding their SuperSpeed
capabilities from the host. The driver has been using the
SuperSpeed capability, as shown by the bcdUSB field of the
device descriptor, to detect the need to enable the DTR
quirk. This method fails when the modems are forced to
USB2 mode by the modem firmware.
Fix by unconditionally enabling the DTR quirk for the
affected device IDs.
Reported-by: Fred Veldini
Reported-by: Deshu Wen
Signed-off-by: Bjørn Mork
---
drivers/net/usb/qmi_wwan.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index e3270deecec2..533b6fb8d923 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -1213,13 +1213,13 @@ static const struct usb_device_id products[] = {
{QMI_FIXED_INTF(0x1199, 0x9061, 8)},/* Sierra Wireless Modem */
{QMI_FIXED_INTF(0x1199, 0x9063, 8)},/* Sierra Wireless EM7305 */
{QMI_FIXED_INTF(0x1199, 0x9063, 10)}, /* Sierra Wireless EM7305 */
- {QMI_FIXED_INTF(0x1199, 0x9071, 8)},/* Sierra Wireless MC74xx */
- {QMI_FIXED_INTF(0x1199, 0x9071, 10)}, /* Sierra Wireless MC74xx */
- {QMI_FIXED_INTF(0x1199, 0x9079, 8)},/* Sierra Wireless EM74xx */
- {QMI_FIXED_INTF(0x1199, 0x9079, 10)}, /* Sierra Wireless EM74xx */
- {QMI_FIXED_INTF(0x1199, 0x907b, 8)},/* Sierra Wireless EM74xx */
- {QMI_FIXED_INTF(0x1199, 0x907b, 10)}, /* Sierra Wireless EM74xx */
- {QMI_FIXED_INTF(0x1199, 0x9091, 8)},/* Sierra Wireless EM7565 */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x9071, 8)}, /* Sierra Wireless MC74xx */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x9071, 10)},/* Sierra Wireless MC74xx */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x9079, 8)}, /* Sierra Wireless EM74xx */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x9079, 10)},/* Sierra Wireless EM74xx */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x907b, 8)}, /* Sierra Wireless EM74xx */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x907b, 10)},/* Sierra Wireless EM74xx */
+ {QMI_QUIRK_SET_DTR(0x1199, 0x9091, 8)}, /* Sierra Wireless EM7565 */
{QMI_FIXED_INTF(0x1bbb, 0x011e, 4)},/* Telekom Speedstick LTE II
(Alcatel One Touch L100V LTE) */
{QMI_FIXED_INTF(0x1bbb, 0x0203, 2)},/* Alcatel L800MA */
{QMI_FIXED_INTF(0x2357, 0x0201, 4)},/* TP-LINK HSUPA Modem MA180 */
--
2.11.0
Re: [PATCH] option: Improve Quectel EP06 detection
Dan Williams 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
Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module
Aleksander Morgado writes: > Being a Qualcomm based chipset, I believe qcserial would be more appropriate. Plenty of Qualcomm devices are handled by option. IMHO, there is no point in adding device specific interface matching to qcserial, unless it is a reusable pattern like the ones we have there before. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module
Oliver Neukum writes: > On Di, 2018-06-26 at 09:40 +0200, Bjørn Mork wrote: >> This code can make Linux default to a MBIM configuration if the MBIM >> function uses the first interface in that configuration, even if this >> configuration is not the first one. Availability of a driver is not >> considered. Except for RNDIS, just to make it the whole mess even more >> confusing > > How would you consider it? We chose a configuration before we load > drivers. Even if we looked at the currently available drivers we'd end > up with a choice depending on which devices were used in the past. > A nondeterministic choice would be awkward. > > We can load drivers for all configurations' interfaces, but we cannot > really wait for the loads to happen at that stage. No, we do not want more driver support guesswork. Going back in time, I believe it would be far better to simply let the firmware decide the default by using the first configuration. It's the least surprising choice, and the likely tested configuration. And it would simplify the code. You are of course correct that we have no knowledge of available, matching, or successfully probing, drivers at this point. All the problems come from the attempt to consider driver availability anyway. The preference for class functions is clearly(?) based on an assumption that class drivers are more likely to be available. But I do realize that this was more difficult when the code was written. We did not support many vendor specific functions, and there wasn't mcuh reason to believe this would change over time. And it might have been more complicated/impossible to override the kernel default in userspace, writing to bConfigurationValue from something like udev? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module
Aleksander Morgado writes: > On Tue, Jun 26, 2018 at 8:09 AM, Johan Hovold wrote: >> On Sat, Jun 23, 2018 at 11:24:08PM +0200, Aleksander Morgado wrote: >>> This module exposes two USB configurations: a QMI+AT capable setup on >>> USB config #1 and a MBIM capable setup on USB config #2. >>> >>> By default the kernel will choose the MBIM capable configuration as >>> long as the cdc_mbim driver is available. This patch adds support for >>> the serial ports in the secondary configuration. >> >> Could you please post the usb-devices output for this device? >> >> Depending on another driver to select a specific configuration seems >> fragile (and that behaviour is even configurable). >> > > This would be when running on configuration #1: > > T: Bus=04 Lev=03 Prnt=04 Port=02 Cnt=01 Dev#= 7 Spd=5000 MxCh= 0 > D: Ver= 3.10 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs= 2 > P: Vendor=413c ProdID=81d7 Rev=03.18 > S: Manufacturer=DELL > S: Product=DW5821e Snapdragon X20 LTE > S: SerialNumber=0123456789ABCDEF > C: #Ifs= 5 Cfg#= 1 Atr=a0 MxPwr=896mA > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=qcserial > I: If#=0x1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > I: If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=qcserial > I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=qcserial > I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none) > > This would be when running on configuration #2: > > T: Bus=04 Lev=03 Prnt=04 Port=02 Cnt=01 Dev#= 6 Spd=5000 MxCh= 0 > D: Ver= 3.10 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs= 2 > P: Vendor=413c ProdID=81d7 Rev=03.18 > S: Manufacturer=DELL > S: Product=DW5821e Snapdragon X20 LTE > S: SerialNumber=0123456789ABCDEF > C: #Ifs= 3 Cfg#= 2 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= 1 Cls=ff(vend.) Sub=ff Prot=ff Driver=qcserial Thanks. This illustrates the arbitrary default configuration choice very well: Imagine switching the order of the qcserial and mbim functions, making qcserial use the first interface. Linux would then select cfg #1 as default, even if the set of functions in both configuratoons were the same as before. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module
Johan Hovold writes:
> On Sat, Jun 23, 2018 at 11:24:08PM +0200, Aleksander Morgado wrote:
>> This module exposes two USB configurations: a QMI+AT capable setup on
>> USB config #1 and a MBIM capable setup on USB config #2.
>>
>> By default the kernel will choose the MBIM capable configuration as
>> long as the cdc_mbim driver is available. This patch adds support for
>> the serial ports in the secondary configuration.
>
> Could you please post the usb-devices output for this device?
>
> Depending on another driver to select a specific configuration seems
> fragile (and that behaviour is even configurable).
I believe Aleksander might be referring to usb_choose_configuration() in
drivers/usb/core/generic.c? It does some confusing things with
multi-function/multi-configuration devices, explained by this comment:
/* From the remaining configs, choose the first one whose
* first interface is for a non-vendor-specific class.
* Reason: Linux is more likely to have a class driver
* than a vendor-specific driver. */
This code can make Linux default to a MBIM configuration if the MBIM
function uses the first interface in that configuration, even if this
configuration is not the first one. Availability of a driver is not
considered. Except for RNDIS, just to make it the whole mess even more
confusing
The result is of course completely arbitrary on any multi-function
device, where vendor-specific functions may be mixed with class
functions in any order. The result will also change if you e.g. enable
a vendor specific debug function in the MBIM configuration, and this
function happens to use the first interface.
The logic in usb_choose_configuration() is obviously outdated. I assume
it was created for single function devices, where that single function
was exposed as either a class function or a vendor specific function
using separate configurations. This sort of device is still quite
common for a few device classes, like for example ethernet NICs. But
things have changed a lot for these as well. Linux now has extensive
support for vendor sepcific USB functions of all sorts. Not that I
need to tell you that :-) The legacy code in usb_choose_configuration()
is now often an obstacle making it more difficult for drivers providing
the best possible support in Linux. And we end up with horrible hacks
like this config-dependent blacklist entry in cdc_ether:
#if IS_ENABLED(CONFIG_USB_RTL8152)
/* Linksys USB3GIGV1 Ethernet Adapter */
{
USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
.driver_info = 0,
},
#endif
matched with code in the rtl8152 driver to switch the device back to its
default configuration:
static int rtl8152_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct usb_device *udev = interface_to_usbdev(intf);
u8 version = rtl_get_version(intf);
struct r8152 *tp;
struct net_device *netdev;
int ret;
if (version == RTL_VER_UNKNOWN)
return -ENODEV;
if (udev->actconfig->desc.bConfigurationValue != 1) {
usb_driver_set_configuration(udev, 1);
return -ENODEV;
}
..
Extremely ugly, and completely unnecessary if it weren't for that extra
mess in usb_choose_configuration(). And the net effect is that the
users end up losing the option of using the class driver for this device,
since that will be black listed if they (or the distro) built the vendor
specific driver.
Can we fix this? I've thought about it more than once for a few years,
but so far I've rejected the thought. The Linux defaults coming out of
usb_choose_configuration() are arbitrary, and often different from any
other OS, confusing end users. But the Linux defaults are stable as
long as the device configration is stable. Changing
usb_choose_configuration() will break some existing setups. The
breakages will of course be easy fixable from userspace, but still -
proabably out question for Linux?
Sorry if I misunderstodd you, Aleksander. I guess you could be thinking
about the usb_modeswitch logic too, which does consider cdc_mbim
availability. The rant is still valid, though :-)
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-wdm: don't enable interrupts in USB-giveback
Sebastian Andrzej Siewior writes: > On 2018-06-12 12:43:01 [-0400], Alan Stern wrote: >> On Tue, 12 Jun 2018, Sebastian Andrzej Siewior wrote: >> >> > In the code path >> > __usb_hcd_giveback_urb() >> > -> wdm_in_callback() >> >-> service_outstanding_interrupt() >> > >> > The function service_outstanding_interrupt() will unconditionally enable >> > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL. >> > If the HCD completes in BH (like ehci does) then the context remains >> > atomic due local_bh_disable() and enabling interrupts does not change >> > this. >> > >> > Add an argument to service_outstanding_interrupt() which decides >> > whether or not it is save to enable interrupts during unlocking and use >> > GFP_KERNEL or not. >> >> Wouldn't it be easier just to use spin_lock_irqsave() and GFP_ATOMIC >> all the time? That's what people normally do with code that can be >> called in both process and interrupt contexts. > > service_outstanding_interrupt() does unlock + lock instead lock + > unlock. If you want to have this "always" working (without the > argument), we could do the false case: > + spin_unlock(&desc->iuspin); > + rv = usb_submit_urb(desc->response, GFP_ATOMIC); > + spin_lock(&desc->iuspin); > > all the time. I wanted to preserve the GFP_KERNEL part which is probably > used more often but fell that this is not needed I could drop that part. Yes, the atomic case should be rare. It will only happen on errors, and IIUC that's only to work around issues caused by reporting errors back to userspace without actually wanting to err out anyway. I believe it would be better to decide one an error policy and stick to that. Then you could just simplify away that whole mess, by either ignoring the error and continue or bailing out and die. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: qmi_wwan: Add Netgear Aircard 779S
Josh Hill writes: > Add support for Netgear Aircard 779S > > Signed-off-by: Josh Hill Acked-by: Bjørn Mork Please queue this for stable too. Thanks. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] qmi_wwan: apply SET_DTR quirk to the SIMCOM shared device ID
SIMCOM are reusing a single device ID for many (all of their?)
different modems, based on different chipsets and firmwares. Newer
Qualcomm chipset generations require setting DTR to wake the QMI
function. The SIM7600E modem is using such a chipset, making it
fail to work with this driver despite the device ID match.
Fix by unconditionally enabling the SET_DTR quirk for all SIMCOM
modems using this specific device ID. This is similar to what
we already have done for another case of device IDs recycled over
multiple chipset generations: 14cf4a771b30 ("drivers: net: usb:
qmi_wwan: add QMI_QUIRK_SET_DTR for Telit PID 0x1201")
Initial testing on an older SIM7100 modem shows no immediate side
effects.
Reported-by: Sebastian Sjoholm
Cc: Reinhard Speyerer
Signed-off-by: Bjørn Mork
---
I propose this for net-next for now to get some extra testing
exposure before going into stable. I'll send a separate request
for stable backport as soon as I'm satisfied that there are no
hidden issues with some specific modem.
Bjørn
drivers/net/usb/qmi_wwan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 42565dd33aa6..148e78f8b48c 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -1248,7 +1248,7 @@ static const struct usb_device_id products[] = {
{QMI_FIXED_INTF(0x03f0, 0x4e1d, 8)},/* HP lt4111 LTE/EV-DO/HSPA+
Gobi 4G Module */
{QMI_FIXED_INTF(0x03f0, 0x9d1d, 1)},/* HP lt4120 Snapdragon X5 LTE
*/
{QMI_FIXED_INTF(0x22de, 0x9061, 3)},/* WeTelecom WPD-600N */
- {QMI_FIXED_INTF(0x1e0e, 0x9001, 5)},/* SIMCom 7230E */
+ {QMI_QUIRK_SET_DTR(0x1e0e, 0x9001, 5)}, /* SIMCom 7100E, 7230E, 7600E
++ */
{QMI_QUIRK_SET_DTR(0x2c7c, 0x0125, 4)}, /* Quectel EC25, EC20 R2.0
Mini PCIe */
{QMI_QUIRK_SET_DTR(0x2c7c, 0x0121, 4)}, /* Quectel EC21 Mini PCIe */
{QMI_FIXED_INTF(0x2c7c, 0x0296, 4)},/* Quectel BG96 */
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net,stable] qmi_wwan: do not steal interfaces from class drivers
The USB_DEVICE_INTERFACE_NUMBER matching macro assumes that
the { vendorid, productid, interfacenumber } set uniquely
identifies one specific function. This has proven to fail
for some configurable devices. One example is the Quectel
EM06/EP06 where the same interface number can be either
QMI or MBIM, without the device ID changing either.
Fix by requiring the vendor-specific class for interface number
based matching. Functions of other classes can and should use
class based matching instead.
Fixes: 03304bcb5ec4 ("net: qmi_wwan: use fixed interface number matching")
Signed-off-by: Bjørn Mork
---
It's quite possible that the fix should be integrated in the
USB_DEVICE_INTERFACE_NUMBER macro instead. But that has grown a few
other users since it was added, so changing it now seems risky.
Another option is of course adding a new match macro with the
USB_CLASS_VENDOR_SPEC match integrated. Maybe best?
But I'm proposing this as-is for now, since this quickfix seems most
suitable for stable backporting.
drivers/net/usb/qmi_wwan.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 51c68fc416fa..42565dd33aa6 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -1344,6 +1344,18 @@ static int qmi_wwan_probe(struct usb_interface *intf,
id->driver_info = (unsigned long)&qmi_wwan_info;
}
+ /* There are devices where the same interface number can be
+* configured as different functions. We should only bind to
+* vendor specific functions when matching on interface number
+*/
+ if (id->match_flags & USB_DEVICE_ID_MATCH_INT_NUMBER &&
+ desc->bInterfaceClass != USB_CLASS_VENDOR_SPEC) {
+ dev_dbg(&intf->dev,
+ "Rejecting interface number match for class %02x\n",
+ desc->bInterfaceClass);
+ return -ENODEV;
+ }
+
/* Quectel EC20 quirk where we've QMI on interface 4 instead of 0 */
if (quectel_ec20_detected(intf) && desc->bInterfaceNumber == 0) {
dev_dbg(&intf->dev, "Quectel EC20 quirk, skipping interface
0\n");
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: option: adding support for ublox R410M
Johan Hovold writes: > On Thu, Apr 26, 2018 at 02:48:54PM +0700, Lars Melin wrote: >> On 4/26/2018 14:09, Johan Hovold wrote: >> > On Thu, Apr 26, 2018 at 02:28:31PM +0800, SZ Lin (林上智) wrote: >> >> This patch adds support for ublox R410M PID 0x90b2 USB modem to option >> >> driver, this module supports LTE Cat M1 / NB1. >> >> >> >> Interface layout: >> >> 0: QCDM/DIAG >> >> 1: ADB >> >> 2: AT >> >> 3: RMNET >> >> >> >> Signed-off-by: SZ Lin (林上智) >> >> Cc: stable >> > >> > Applied, thanks. >> > >> > Johan >> >> With a Qualcomm Device Management interface, shouldn't this modem be >> driven by qcserial? > > Hmm, we already have some QCDM interfaces handled by option and qcaux so > it's not that clear-cut. > > Dan and Björn had a discussion about this a while back, so adding them > on CC. It seems to me that this device does not fit the intended use (or > Gobi 1000 layout) for qcserial, but I may be mistaken. tl;dr; I don't think qcserial is relevant unless a device matches one of the pre-defined layout schemes. But I'm too new in this game to say anything about the initial intentions... My view of the present situation is that qcserial handles interface layouts shared among many devices, while option handles interface layouts which are unique per device, and vendor+class based function mappings. But I could be wrong. Anyway, Qualcomm based designs are definitely handled by both drivers. Using qcserial only makes sense if the interface layout matches one of the defined shared schemes, which currently are: QCSERIAL_G2K = 0, /* Gobi 2000 */ QCSERIAL_G1K = 1, /* Gobi 1000 */ QCSERIAL_SWI = 2, /* Sierra Wireless */ QCSERIAL_HWI = 3, /* Huawei */ or if similar logic is added for a new vendor/shceme And I see multi-vendor-id usage as the main reason for these schemes. There isn't much reason if you can make it a single match against a vendor-id. The original Gobi devices are obviously multi-vendor, and Sierra Wireless and Huwaei are OEMs making devices for a number of laptop vendors. This causes traditional vendor-id matching to fail, as e.g. a HP device can be made by either OEMs (or others). qcserial has become a convenient way to map a long list of full device IDs to a specific OEM layout. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: usb: qmi_wwan: add support for ublox R410M PID 0x90b2
"SZ Lin (林上智)" writes: > This patch adds support for PID 0x90b2 of ublox R410M. Acked-by: Bjørn Mork -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: qmi_wwan: add Wistron Neweb D19Q1
Pawel Dembicki writes: > This modem is embedded on dlink dwr-960 router. > The oem configuration states: > > T: Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 2 Spd=480 MxCh= 0 > D: Ver= 2.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=1435 ProdID=d191 Rev=ff.ff > S: Manufacturer=Android > S: Product=Android > S: SerialNumber=0123456789ABCDEF > C:* #Ifs= 6 Cfg#= 1 Atr=80 MxPwr=500mA > I:* If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none) > E: Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=(none) > E: Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none) > E: Ad=84(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > E: Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none) > E: Ad=86(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > E: Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > E: Ad=88(I) Atr=03(Int.) MxPS= 8 Ivl=32ms > E: Ad=87(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 5 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=(none) > E: Ad=89(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=125us > > Tested on openwrt distribution > > Signed-off-by: Pawel Dembicki Acked-by: Bjørn Mork -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cdc_ether: flag the Cinterion PLS8 modem by gemalto as WWAN
Bassem Boubaker writes: > +#define GEMALTO_VENDOR_ID0x1e2d This is defined as CINTERION_VENDOR_ID in drivers/usb/serial/option.c. I have no idea which defintion is most correct, but I believe the macros should be kept identical whatever you decide. Anything else is just unnecessarily confusing. IMHO the company name tracking macros have grown beyond useful a long time ago. They just make it harder to grep for the IDs without adding any useful information whatsoever. And because you have cases like this where the same number end up having different names, they sometimes hide information which a plain number would have revealed. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+
"Rafael J. Wysocki" writes: > On 2/4/2018 9:28 PM, Bjørn Mork wrote: > >> But I do wonder if the attached (completely untested!!) patch makes >> things any better? > > I don't think so, the macro is needed too. Doh! Obviously. Don't know how I managed to miss that. > I'll queue up a full revert of 662591461c4b9a1e3b. Still with the additional exception for "ec == first_ec"? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+
Greg KH writes:
> On Sat, Feb 03, 2018 at 07:25:54PM +0100, Markus Demleitner wrote:
>
>> It's 662591461c4b9a1e3b9b159dbf37648a585ebaae. To my eyes, it even
>> looks plausible that it's causing the problematic behaviour, but
>> since I can't say I understand what I'd be doing if I dabbled with
>> the change, I've refrained from guessing how to fix it.
>>
>> I'm happy to try patches, though.
>
> Ok, thanks. I've added the authors of this patch to the email here,
> perhaps they have an idea of what is going on?
This thing made me curious enough to dive into code I don't understand,
as I have experienced the annoying crazy fan behaviour in resume a few
times on my X1 Carbon 4th gen.
Maybe I missed something, but it looks like commit
c3a696b6e8f8 ("ACPI / EC: Use busy polling mode when GPE is not enabled")
introduced suspend/resume busy polling for the "boot EC" unintentionally?
The patch moved acpi_ec_leave_noirq() and acpi_ec_leave_noirq()
functions outside the #ifdef CONFIG_PM_SLEEP, so they could be reused
while installing handlers. But when doing that the
if (ec == first_ec)
conditions on suspend/resume were silently dropped. I assume the
intention might have been to move those intto acpi_ec_suspend_noirq()
and acpi_ec_resume_noirq() instead? But that didn't happen AFAICS.
Or did I misunderstand this completely? Not unlikely given that I have
zero clue about what this code is doing...
But I do wonder if the attached (completely untested!!) patch makes
things any better?
Bjørn
>From 82b8f437a243854a3f1d3c82f85520fd2b71 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?=
Date: Sun, 4 Feb 2018 21:15:36 +0100
Subject: [PATCH] Revert "ACPI / EC: Drop EC noirq hooks to fix a regression"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This reverts commit 662591461c4b9a1e3b9b159dbf37648a585ebaae
and re-introduce the conditions dropped by commit c3a696b6e8f8
("ACPI / EC: Use busy polling mode when GPE is not enabled")
Fixes: c3a696b6e8f8 ("ACPI / EC: Use busy polling mode when GPE is not enabled")
Signed-off-by: Bjørn Mork
---
drivers/acpi/ec.c | 20
1 file changed, 20 insertions(+)
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d9f38c645e4a..24a772f66847 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1905,6 +1905,26 @@ int __init acpi_ec_ecdt_probe(void)
}
#ifdef CONFIG_PM_SLEEP
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+ struct acpi_ec *ec =
+ acpi_driver_data(to_acpi_device(dev));
+
+ if (ec == first_ec)
+ acpi_ec_enter_noirq(ec);
+ return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+ struct acpi_ec *ec =
+ acpi_driver_data(to_acpi_device(dev));
+
+ if (ec == first_ec)
+ acpi_ec_leave_noirq(ec);
+ return 0;
+}
+
static int acpi_ec_suspend(struct device *dev)
{
struct acpi_ec *ec =
--
2.11.0
Re: [PATCH] option: Add support for Quectel EP06
Johan Hovold writes: > IIRC we have already started reusing entries, but the names have not > always been made generic in the process. I think Bjørn may have proposed > a generic naming scheme at some point, but that essentially just meant > we encoded the two masks in the name. Maybe. I dont' remember. > Then we may just move over to > encoding the masks directly in driver_data instead, at least if 16 bits > is enough. Agreed. >> IIRC I considered just dumping the BIT(x) into the .driver_info but >> then we'd only have 16 bits for each of send_setup and reserved on 32- >> bit arches and I wasn't sure that was enough. I've seen some devices >> with lots of interfaces. But doing it this way might have been clearer >> than a sidecar struct like option_blacklist_info. > > Yeah, we should probably consider moving over to something like that. > 16 bits would at least be enough for the devices we currently have > blacklists for. Yes, I think the current driver documents pretty well that we don't need backlists for high interface numbers. Checking the devices I have ever used, the only ones I found with interface numbers higher than 16 were the Sierra Wireless modems of the MDM9200/MDM9600 generation. THe used 19 and 20 for two of their QMI/RMNET functions. But they are handled by the qcserial driver, so that's not an issue wrt option. I say go for the simple bitmasks! You might also consider a general blacklist for stuff like ADB which always need blacklisting. By now, Google owns ff/42/xx whether you like it or not. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: usb: qmi_wwan: add support for YUGA CLM920-NC5 PID 0x9625
"SZ Lin (林上智)" writes:
> This patch adds support for PID 0x9625 of YUGA CLM920-NC5.
>
> YUGA CLM920-NC5 needs to enable QMI_WWAN_QUIRK_DTR before QMI operation.
>
> qmicli -d /dev/cdc-wdm0 -p --dms-get-revision
> [/dev/cdc-wdm0] Device revision retrieved:
> Revision: 'CLM920_NC5-V1 1 [Oct 23 2016 19:00:00]'
>
> Signed-off-by: SZ Lin (林上智)
> ---
> drivers/net/usb/qmi_wwan.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 3000ddd1c7e2..728819feab44 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -1100,6 +1100,7 @@ static const struct usb_device_id products[] = {
> {QMI_FIXED_INTF(0x05c6, 0x9084, 4)},
> {QMI_FIXED_INTF(0x05c6, 0x920d, 0)},
> {QMI_FIXED_INTF(0x05c6, 0x920d, 5)},
> + {QMI_QUIRK_SET_DTR(0x05c6, 0x9625, 4)}, /* YUGA CLM920-NC5 */
> {QMI_FIXED_INTF(0x0846, 0x68a2, 8)},
> {QMI_FIXED_INTF(0x12d1, 0x140c, 1)},/* Huawei E173 */
> {QMI_FIXED_INTF(0x12d1, 0x14ac, 1)},/* Huawei E1820 */
Perfect. Thanks
Acked-by: Bjørn Mork
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5
"SZ Lin (林上智)" writes:
>> Johan Hovold writes:
>>
>> >> +static const struct option_blacklist_info yuga_clm920_nc5_blacklist = {
>> >> + .reserved = BIT(0) | BIT(1) | BIT(4), };
>> >
>> > Do you really need to blacklist the first interface?
>>
>> Good question. Interface #0 does look a lot like a Qualcomm DM/DIAG
>> function, based
>> on two bulk endpoints, no additional descriptors and the fact that it is the
>> first interface.
>> If so, then we do want a serial driver for it. There is a basic libqcdm
>> implementation in
>> ModemManager if you want to test it out.
>>
>
> I have confirmed that interface #0 is QCDM/DIAG port in this module,
>and thus I will remove this from reserved list in next patch.
>Furthermore, interface #1 is ADB port. Should I also remove this from
>reserved list?
No. ADB is handled by userspace tools using libusb. It should not be
bound to any serial driver, so you will need to blacklist it. But you
need to keep the blacklist anyway to include the QCDM/DIAG port
I assume Johan's alternative was to match class/subclass/protocol
against ff/00/00, which would have worked if you only wanted to include
interfaces 2 and 3.
>> And I expect interface #4 is QMI/rmnet? Feel free to confirm that
>> assumption with a
>> patch against qmi_wwan :-)
>>
> Yes, it is. I will send qmi_wwan patch by all means.
Thanks
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5
Johan Hovold writes:
>> +static const struct option_blacklist_info yuga_clm920_nc5_blacklist = {
>> +.reserved = BIT(0) | BIT(1) | BIT(4),
>> +};
>
> Do you really need to blacklist the first interface?
Good question. Interface #0 does look a lot like a Qualcomm DM/DIAG
function, based on two bulk endpoints, no additional descriptors and the
fact that it is the first interface. If so, then we do want a serial
driver for it. There is a basic libqcdm implementation in ModemManager
if you want to test it out.
And I expect interface #4 is QMI/rmnet? Feel free to confirm that
assumption with a patch against qmi_wwan :-)
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] qmi_wwan: set FLAG_SEND_ZLP to avoid network initiated disconnect
Vamsi Samavedam writes: > On Thu, Dec 14, 2017 at 07:55:50PM +0100, Bjørn Mork wrote: >> It has been reported that the dummy byte we add to avoid >> ZLPs can be forwarded by the modem to the PGW/GGSN, and that >> some operators will drop the connection if this happens. >> >> In theory, QMI devices are based on CDC ECM and should as such >> both support ZLPs and silently ignore the dummy byte. The latter >> assumption failed. Let's test out the first. >> >> Signed-off-by: Bjørn Mork >> --- >> I am a bit worried about the effect of this change on all the >> devices I can't test myself. But trying it is the only way we >> can ever find out > > flag zlp is tested with pids: 9034,9048,904c,9075, 908E,9079, > 908A,909F and 90A4. In general, qualcomm usb firmware can > handle zlps. Thanks. That's very good to know. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] qmi_wwan: set FLAG_SEND_ZLP to avoid network initiated disconnect
It has been reported that the dummy byte we add to avoid
ZLPs can be forwarded by the modem to the PGW/GGSN, and that
some operators will drop the connection if this happens.
In theory, QMI devices are based on CDC ECM and should as such
both support ZLPs and silently ignore the dummy byte. The latter
assumption failed. Let's test out the first.
Signed-off-by: Bjørn Mork
---
I am a bit worried about the effect of this change on all the
devices I can't test myself. But trying it is the only way we
can ever find out
drivers/net/usb/qmi_wwan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 304ec6555cd8..1ed00519f29e 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -826,7 +826,7 @@ static int qmi_wwan_resume(struct usb_interface *intf)
static const struct driver_infoqmi_wwan_info = {
.description= "WWAN/QMI device",
- .flags = FLAG_WWAN,
+ .flags = FLAG_WWAN | FLAG_SEND_ZLP,
.bind = qmi_wwan_bind,
.unbind = qmi_wwan_unbind,
.manage_power = qmi_wwan_manage_power,
@@ -835,7 +835,7 @@ static const struct driver_info qmi_wwan_info = {
static const struct driver_infoqmi_wwan_info_quirk_dtr = {
.description= "WWAN/QMI device",
- .flags = FLAG_WWAN,
+ .flags = FLAG_WWAN | FLAG_SEND_ZLP,
.bind = qmi_wwan_bind,
.unbind = qmi_wwan_unbind,
.manage_power = qmi_wwan_manage_power,
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: qcserial: add Sierra Wireless EM7565
Reinhard Speyerer writes: > Sierra Wireless EM7565 devices use the DEVICE_SWI layout for their > serial ports > > T: Bus=01 Lev=03 Prnt=29 Port=01 Cnt=02 Dev#= 31 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=1199 ProdID=9091 Rev= 0.06 > S: Manufacturer=Sierra Wireless, Incorporated > S: Product=Sierra Wireless EM7565 Qualcomm Snapdragon X16 LTE-A > S: SerialNumber= > C:* #Ifs= 4 Cfg#= 1 Atr=a0 MxPwr=500mA > I:* If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=qcserial > 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= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=qcserial > E: Ad=83(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > 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= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=qcserial > E: Ad=85(I) Atr=03(Int.) MxPS= 10 Ivl=32ms > E: Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 8 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > E: Ad=86(I) Atr=03(Int.) MxPS= 8 Ivl=32ms > E: Ad=8e(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=0f(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > > but need sendsetup = true for the GPS port to make it work properly. > Add a new DEVICE_SW2 layout variant and use it for the EM7565 PID 0x9091. > Add a DEVICE_SWI entry for the EM7565 QDL PID 0x9090. I wonder if the new variant is strictly necessary? I got a feeling that the old behaviour is arbitrary, and that older Sierra devices might work both with and without sendsetup too. Did you try sendsetup on the NMEA port with any older Sierra device? I just did a quick test on the EM7455 in my laptop, and it doesn't seem to mind at least. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net,stable] net: qmi_wwan: add Sierra EM7565 1199:9091
ssjoh...@mac.com writes:
> From: Sebastian Sjoholm
>
> From: Sebastian Sjoholm
>
> Sierra Wireless EM7565 is an Qualcomm MDM9x50 based M.2 modem.
> The USB id is added to qmi_wwan.c to allow QMI communication with the EM7565.
>
> Signed-off-by: Sebastian Sjoholm
> ---
> [The corresponding qcserial patch will be submitted by Reinhard Speyerer.]
>
> ---
> drivers/net/usb/qmi_wwan.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 304ec6555cd8..3cebd6683938 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -1204,6 +1204,7 @@ static const struct usb_device_id products[] = {
> {QMI_FIXED_INTF(0x1199, 0x9079, 10)}, /* Sierra Wireless EM74xx */
> {QMI_FIXED_INTF(0x1199, 0x907b, 8)},/* Sierra Wireless EM74xx */
> {QMI_FIXED_INTF(0x1199, 0x907b, 10)}, /* Sierra Wireless EM74xx */
> + {QMI_FIXED_INTF(0x1199, 0x9091, 8)},/* Sierra Wireless EM7565 */
> {QMI_FIXED_INTF(0x1bbb, 0x011e, 4)},/* Telekom Speedstick LTE II
> (Alcatel One Touch L100V LTE) */
> {QMI_FIXED_INTF(0x1bbb, 0x0203, 2)},/* Alcatel L800MA */
> {QMI_FIXED_INTF(0x2357, 0x0201, 4)},/* TP-LINK HSUPA Modem MA180 */
Looks good except for the duplicate 'From' line. Drop that and you can
add
Acked-by: Bjørn Mork
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbnet: fix alignment for frames with no ethernet header
David Miller writes:
> From: Bjørn Mork
> Date: Wed, 6 Dec 2017 20:21:24 +0100
>
>> The qmi_wwan minidriver support a 'raw-ip' mode where frames are
>> received without any ethernet header. This causes alignment issues
>> because the skbs allocated by usbnet are "IP aligned".
>>
>> Fix by allowing minidrivers to disable the additional alignment
>> offset. This is implemented using a per-device flag, since the same
>> minidriver also supports 'ethernet' mode.
>>
>> Fixes: 32f7adf633b9 ("net: qmi_wwan: support "raw IP" mode")
>> Reported-and-tested-by: Jay Foster
>> Signed-off-by: Bjørn Mork
>
> Looks good, applied and queued up for -stable.
Thanks. I can see it in the -stable queue, but it didn't show up here
yet: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git
Did it get stuck somewhere?
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbnet: fix alignment for frames with no ethernet header
The qmi_wwan minidriver support a 'raw-ip' mode where frames are
received without any ethernet header. This causes alignment issues
because the skbs allocated by usbnet are "IP aligned".
Fix by allowing minidrivers to disable the additional alignment
offset. This is implemented using a per-device flag, since the same
minidriver also supports 'ethernet' mode.
Fixes: 32f7adf633b9 ("net: qmi_wwan: support "raw IP" mode")
Reported-and-tested-by: Jay Foster
Signed-off-by: Bjørn Mork
---
drivers/net/usb/qmi_wwan.c | 2 ++
drivers/net/usb/usbnet.c | 5 -
include/linux/usb/usbnet.h | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index c750cf7c042b..304ec6555cd8 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -261,9 +261,11 @@ static void qmi_wwan_netdev_setup(struct net_device *net)
net->hard_header_len = 0;
net->addr_len= 0;
net->flags = IFF_POINTOPOINT | IFF_NOARP |
IFF_MULTICAST;
+ set_bit(EVENT_NO_IP_ALIGN, &dev->flags);
netdev_dbg(net, "mode: raw IP\n");
} else if (!net->header_ops) { /* don't bother if already set */
ether_setup(net);
+ clear_bit(EVENT_NO_IP_ALIGN, &dev->flags);
netdev_dbg(net, "mode: Ethernet\n");
}
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 80348b6a8646..d56fe32bf48d 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -484,7 +484,10 @@ static int rx_submit (struct usbnet *dev, struct urb *urb,
gfp_t flags)
return -ENOLINK;
}
- skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
+ if (test_bit(EVENT_NO_IP_ALIGN, &dev->flags))
+ skb = __netdev_alloc_skb(dev->net, size, flags);
+ else
+ skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
if (!skb) {
netif_dbg(dev, rx_err, dev->net, "no rx skb\n");
usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index a69877734c4e..e2ec3582e549 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -82,6 +82,7 @@ struct usbnet {
# define EVENT_RX_KILL10
# define EVENT_LINK_CHANGE11
# define EVENT_SET_RX_MODE12
+# define EVENT_NO_IP_ALIGN13
};
static inline struct usb_driver *driver_of(struct usb_interface *intf)
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net,stable] net: qmi_wwan: add Quectel BG96 2c7c:0296
Sebastian Sjoholm writes: > Quectel BG96 is an Qualcomm MDM9206 based IoT modem, supporting both > CAT-M and NB-IoT. Tested hardware is BG96 mounted on Quectel development > board (EVB). The USB id is added to qmi_wwan.c to allow QMI > communication with the BG96. > > Signed-off-by: Sebastian Sjoholm Perfect. Thanks. Acked-by: Bjørn Mork -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: qmi_wwan: add Dell DW5818, DW5819
Shrirang Bagul writes: > Dell Wireless 5819/5818 devices are re-branded Sierra Wireless MC74 > series modems which will by default boot with vid 0x413c and pid's > 0x81cf, 0x81d0, 0x81d1,0x81d2. Along with qcserial, these modems support > qmi_wwan on the usb interface #12. NAK, Interace #12 is MBIM, as shown by the device descriptors. Please provide those descriptors and you will see that this interface is clearly a CDC MBIM class interface. Yes, I know these modems probe the control protocol so that you can make QMI work on an MBIM control interface by sending it a QMI request as the first messsage. This is still wrong, abusing a quirky firmware feature. You need to reconfigure the modem for QMI using the Sierra specific AT command or QMI request (tunneled in MBIM!) to properly switch it to QMI mode, which will appear as a vendor specific interface number 8 (and 10 if you enable both QMI functions). Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net,stable] net: qmi_wwan: add Quectel BG96 2c7c:0296
On November 20, 2017 5:19:21 AM GMT+01:00, ssjoh...@mac.com wrote:
>From: ssjoholm
>
>Signed-off-by: ssjoholm
>
>Quectel BG96 is an Qualcomm MDM9206 based IoT modem, supporting both
>CAT-M and NB-IoT. Tested hardware is BG96 mounted on Quectel
>development board (EVB).
>The USB id is added to qmi_wwan.c to allow QMI communication with the
>BG96.
>---
> drivers/net/usb/qmi_wwan.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
>index 720a3a248070..c750cf7c042b 100644
>--- a/drivers/net/usb/qmi_wwan.c
>+++ b/drivers/net/usb/qmi_wwan.c
>@@ -1239,6 +1239,7 @@ static const struct usb_device_id products[] = {
> {QMI_FIXED_INTF(0x1e0e, 0x9001, 5)},/* SIMCom 7230E */
> {QMI_QUIRK_SET_DTR(0x2c7c, 0x0125, 4)}, /* Quectel EC25, EC20 R2.0
>Mini PCIe */
> {QMI_QUIRK_SET_DTR(0x2c7c, 0x0121, 4)}, /* Quectel EC21 Mini PCIe */
>+ {QMI_FIXED_INTF(0x2c7c, 0x0296, 4)},/* Quectel BG96 */
>
> /* 4. Gobi 1000 devices */
> {QMI_GOBI1K_DEVICE(0x05c6, 0x9212)},/* Acer Gobi Modem Device */
Patch looks fine. But you need to use your full name in the tags.
See the part about identity;
https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup
And the SOB and other tags go after the rest of the commit message. Your SOB
should always be the last line.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net,stable] net: cdc_ncm: GetNtbFormat endian fix
The GetNtbFormat and SetNtbFormat requests operate on 16 bit little
endian values. We get away with ignoring this most of the time, because
we only care about USB_CDC_NCM_NTB16_FORMAT which is 0x. This
fails for USB_CDC_NCM_NTB32_FORMAT.
Fix comparison between LE value from device and constant by converting
the constant to LE.
Reported-by: Ben Hutchings
Fixes: 2b02c20ce0c2 ("cdc_ncm: Set NTB format again after altsetting switch for
Huawei devices")
Cc: Enrico Mioso
Cc: Christian Panton
Signed-off-by: Bjørn Mork
---
drivers/net/usb/cdc_ncm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 47cab1bde065..9e1b74590682 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -771,7 +771,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
int err;
u8 iface_no;
struct usb_cdc_parsed_header hdr;
- u16 curr_ntb_format;
+ __le16 curr_ntb_format;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -889,7 +889,7 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
goto error2;
}
- if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
+ if (curr_ntb_format == cpu_to_le16(USB_CDC_NCM_NTB32_FORMAT)) {
dev_info(&intf->dev, "resetting NTB format to 16-bit");
err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
USB_TYPE_CLASS | USB_DIR_OUT
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4.4 27/56] cdc_ncm: Set NTB format again after altsetting switch for Huawei devices
Ben Hutchings writes:
> On Tue, 2017-07-11 at 17:21 +0200, Enrico Mioso wrote:
>> From: Enrico Mioso
>>
>> commit 2b02c20ce0c28974b44e69a2e2f5ddc6a470ad6f upstream.
> [...]
>> --- a/drivers/net/usb/cdc_ncm.c
>> +++ b/drivers/net/usb/cdc_ncm.c
>> @@ -724,8 +724,10 @@ int cdc_ncm_bind_common(struct usbnet *d
>> u8 *buf;
>> int len;
>> int temp;
>> +int err;
>> u8 iface_no;
>> struct usb_cdc_parsed_header hdr;
>> +u16 curr_ntb_format;
> [...]
>> +err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_FORMAT,
>> + USB_TYPE_CLASS | USB_DIR_IN |
>> USB_RECIP_INTERFACE,
>> + 0, iface_no, &curr_ntb_format, 2);
>> +if (err < 0) {
>> +goto error2;
>> +}
>> +
>> +if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
> [...]
>
> usbnet_read_cmd() doesn't do any byte-swapping, so it looks like
> curr_ntb_format will have little-endian byte order (__le16 not u16).
> The comparison will then need to be done using
> le16_to_cpu(curr_ntb_format).
Right. Thanks. Fix coming up ASAP.
I cannot explain why since we are obviously reading and writing 2 bytes
here, but for some reason I've always thought of this value as 8 bit.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Problems with Fibocom-L831EU on Thinkpad x270
On November 15, 2017 12:42:17 AM GMT+01:00, Alexander Mikhalevich wrote: >Hi, >I've recently bought Thinkpad x270 with Fibocom-L831EU WWAN module >which is MBIM device. If I try to use it on my Gentoo Linux (kernel >4.12.12, libmbim 1.14) it doesn't work. >The symptoms are the same as it was described in this mailing list >earlier: https://www.spinics.net/lists/linux-usb/msg158286.html >(mbim-network and mbimcli --connect complete successfully, >dhcpcd can't get IPv4 afterwards, I have EPIPE error in dmesg). If it is the same problem, then I believe it is fixed by https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/drivers/usb/class/cdc-wdm.c?h=linux-4.14.y&id=8fec9355a968ad240f3a2e9ad55b823cf1cc52ff But that was too late for 4.12 stable, so you might have to ask your distro to add it to their kernel. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net] qmi_wwan: Add missing skb_reset_mac_header-call
Kristian Evensen writes:
> When we receive a packet on a QMI device in raw IP mode, we should call
> skb_reset_mac_header() to ensure that skb->mac_header contains a valid
> offset in the packet. While it shouldn't really matter, the packets have
> no MAC header and the interface is configured as-such, it seems certain
> parts of the network stack expects a "good" value in skb->mac_header.
Thanks. Yes, I see that the tun driver also does this.
Acked-by: Bjørn Mork
And his should probably go to stable as well? with a
Fixes: 32f7adf633b9 ("net: qmi_wwan: support "raw IP" mode")
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net,stable] net: cdc_ether: fix divide by 0 on bad descriptors
Setting dev->hard_mtu to 0 will cause a divide error in
usbnet_probe. Protect against devices with bogus CDC Ethernet
functional descriptors by ignoring a zero wMaxSegmentSize.
Signed-off-by: Bjørn Mork
---
I believe the problem found by syzcaller in qmi_wwan also applies
to cdc_ether. We cannot allow the .bind callback to set
dev->hard_mtu to 0.
drivers/net/usb/cdc_ether.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 3e7a3ac3a362..05dca3e5c93d 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -230,7 +230,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
goto bad_desc;
}
- if (header.usb_cdc_ether_desc) {
+ if (header.usb_cdc_ether_desc && info->ether->wMaxSegmentSize) {
dev->hard_mtu = le16_to_cpu(info->ether->wMaxSegmentSize);
/* because of Zaurus, we may be ignoring the host
* side link address we were given.
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net,stable] net: qmi_wwan: fix divide by 0 on bad descriptors
A CDC Ethernet functional descriptor with wMaxSegmentSize = 0 will
cause a divide error in usbnet_probe:
divide error: [#1] PREEMPT SMP KASAN
Modules linked in:
CPU: 0 PID: 24 Comm: kworker/0:1 Not tainted 4.14.0-rc8-44453-g1fdc1a82c34f #56
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
task: 88006bef5c00 task.stack: 88006bf6
RIP: 0010:usbnet_update_max_qlen+0x24d/0x390 drivers/net/usb/usbnet.c:355
RSP: 0018:88006bf67508 EFLAGS: 00010246
RAX: 000163c8 RBX: 8800621fce40 RCX: 8800621fcf34
RDX: RSI: 837ecb7a RDI: 8800621fcf34
RBP: 88006bf67520 R08: 88006bef5c00 R09: ed000c43f881
R10: ed000c43f880 R11: 8800621fc406 R12: 0003
R13: 85c71de0 R14: R15:
FS: () GS:88006ca0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7ffe9c0d6dac CR3: 614f4000 CR4: 06f0
Call Trace:
usbnet_probe+0x18b5/0x2790 drivers/net/usb/usbnet.c:1783
qmi_wwan_probe+0x133/0x220 drivers/net/usb/qmi_wwan.c:1338
usb_probe_interface+0x324/0x940 drivers/usb/core/driver.c:361
really_probe drivers/base/dd.c:413
driver_probe_device+0x522/0x740 drivers/base/dd.c:557
Fix by simply ignoring the bogus descriptor, as it is optional
for QMI devices anyway.
Fixes: 423ce8caab7e ("net: usb: qmi_wwan: New driver for Huawei QMI based WWAN
devices")
Reported-by: Andrey Konovalov
Signed-off-by: Bjørn Mork
---
drivers/net/usb/qmi_wwan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 8c3733608271..a4f229edcceb 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -681,7 +681,7 @@ static int qmi_wwan_bind(struct usbnet *dev, struct
usb_interface *intf)
}
/* errors aren't fatal - we can live with the dynamic address */
- if (cdc_ether) {
+ if (cdc_ether && cdc_ether->wMaxSegmentSize) {
dev->hard_mtu = le16_to_cpu(cdc_ether->wMaxSegmentSize);
usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress);
}
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb/net/qmi_wwan: divide error in qmi_wwan_probe/usbnet_probe
Andrey Konovalov writes:
> Hi!
>
> I've got the following report while fuzzing the kernel with syzkaller.
Thanks. It would have helped a lot of you said *what* you were fuzzing,
though But based on where the bug is, I assume it is USB
descriptors?
> On commit 39dae59d66acd86d1de24294bd2f343fd5e7a625 (4.14-rc8).
>
> qmi_wwan 1-1:0.4: cdc-wdm0: USB WDM device
> divide error: [#1] PREEMPT SMP KASAN
> Modules linked in:
> CPU: 0 PID: 24 Comm: kworker/0:1 Not tainted 4.14.0-rc8-44453-g1fdc1a82c34f
> #56
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: usb_hub_wq hub_event
> task: 88006bef5c00 task.stack: 88006bf6
> RIP: 0010:usbnet_update_max_qlen+0x24d/0x390 drivers/net/usb/usbnet.c:355
> RSP: 0018:88006bf67508 EFLAGS: 00010246
> RAX: 000163c8 RBX: 8800621fce40 RCX: 8800621fcf34
> RDX: RSI: 837ecb7a RDI: 8800621fcf34
> RBP: 88006bf67520 R08: 88006bef5c00 R09: ed000c43f881
> R10: ed000c43f880 R11: 8800621fc406 R12: 0003
> R13: 85c71de0 R14: R15:
> FS: () GS:88006ca0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 7ffe9c0d6dac CR3: 614f4000 CR4: 06f0
> Call Trace:
> usbnet_probe+0x18b5/0x2790 drivers/net/usb/usbnet.c:1783
> qmi_wwan_probe+0x133/0x220 drivers/net/usb/qmi_wwan.c:1338
So, looking over this again and again, the only obviously risky division
I can see is the usbnet_update_max_qlen() call where we divide on both
dev->rx_urb_size and dev->hard_mtu.
I don't think dev->rx_urb_size can be 0 unless dev->hard_mtu is 0. But
the latter is possible, and this is a bug. In qmi_wwan_bind(), which is
called by usbnet_probe() prior to the usbnet_update_max_qlen() call, we
do
if (cdc_ether) {
dev->hard_mtu = le16_to_cpu(cdc_ether->wMaxSegmentSize);
usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress);
}
which will be fatal if cdc_ether->wMaxSegmentSize is 0. I assume that
is what your fuzzer did? Fix coming up RSN. Thanks a lot for pointing
this out.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbip port number limits
Juan Zea writes: > Having a look at the code of the VHCI driver at > https://github.com/torvalds/linux/tree/master/drivers/usb/usbip , I > found these two parameters: USBIP_VHCI_HC_PORTS and USBIP_VHCI_NR_HCS > > I've compiled VHCI driver from latest source above, altering both > parameters to get higher number of controllers and ports per > controller (not only the driver, but also the usbip cli tool at > https://github.com/torvalds/linux/tree/master/tools/usb/usbip ), and > run into several problems: > > 1. USBIP_VHCI_HC_PORTS x USBIP_VHCI_NR_HCS <= 128 > > It seems, whatever the combination of both parameters, usbip command > line tool won't accept that your driver is compiled with numbers > violating the above rule. Is this a problem of the tool code or is > there a reason behind this? As far as I 've researched, the 127 > devices limit in Linux is per controller, so increasing the number of > controllers in usbip driver shouldn't be limited this way, am I wrong? > > 2. I've compiled with 4 ports per controller and 32 controllers. Given > that the drivers creates an usb2 and an usb3 controller, it results in > 64 controllers with four ports each: 128 ports. Here, usbip cli tool > works. But... if you try to attach a device, the kernel dumps a trace > that starts with this: > > Oct 30 10 :43:14 test-mononode-ubuntu-16 kernel: [ 44.138068] usbcore: failed > to get bus number > Oct 30 10 :43:14 test-mononode-ubuntu-16 kernel: [ 44.138813] vhci_hcd: > usb_add_hcd hs failed -7 > Oct 30 10 :43:14 test-mononode-ubuntu-16 kernel: [ 44.139559] vhci_hcd: probe > of vhci_hcd.31 failed with error -7 > Oct 30 10 :44:23 test-mononode-ubuntu-16 kernel: [ 113.345824] BUG: unable to > handle kernel NULL pointer dereference at 0230 > Oct 30 10 :44:23 test-mononode-ubuntu-16 kernel: [ 113.347584] IP: > status_show+0x13c/0x360 [vhci_hcd] > Oct 30 10 :44:23 test-mononode-ubuntu-16 kernel: [ 113.348592] PGD 1398cc067 > Oct 30 10 :44:23 test-mononode-ubuntu-16 kernel: [ 113.348593] PUD 136a96067 > Oct 30 10 :44:23 test-mononode-ubuntu-16 kernel: [ 113.349119] PMD 0 > > This leads me to thinking there is some problem with such a big number > of controllers, although the controller number parameters is supposed > to have a max value of 128. Could this be related to the kernel I'm > using? Maybe I should compile the full kernel to get higher numbers > working? You are correct. There are multiple issues here: 1) the drivers/usb/core/hcd.c has a limit on the number of USB buses. This is currently 64, and not configurable without editing the source. So setting USBIP_VHCI_NR_HCS to anything higher than ~30 is futile and has probably never been tested. It cannot work 2) The sysfs code does not consider the possibility that one or more of the VHCIs could have failed to register. It will happily register attributes for all "vhci_num_controllers", and does little to sanity check anything when accessing those attributes. So it should not come as a surprise that this goes Ooops if one or more controllers have failed to register. > 3. For this last test, I compiled with smaller numbers, just to test > how the several controllers thing works. Specifically, one port per > controller, 16 controllers in total. This leads to the module loading, > the controllers listable with lsusb, and no problems in kernel log. I > attach one device, and it goes to the first controller. Working... no > problem at all. But, if you attach a second device, the command hangs > and I get this in syslog: > > Oct 30 11 :07:14 test-mononode-ubuntu-16 kernel: [ 1078.239613] vhci_hcd > vhci_hcd.0: port 0 already used > > This message was repeated for more than 7000 times before I could stop > the command. So... my conclusion is that usbip won't work correctly if > compiled with more than one controller. Is this related to the usbip > cli tool or is there any problem at the driver? I think it would be great if you could work a little on the usbip drivers. The multi-controller support was very immature when it was merged, and has not gotten much love since. I still think using a fixed number of controllers was a big mistake. It would have been much better to add controllers dynamically when you needed more ports. The bugs you have found are fallout of the shortcuts taken when the code was merged. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs
"Gustavo A. R. Silva" writes:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Notice that in this particular case I replaced "...drop on through"
> comments with a proper "fall through" comment on its own line, which
> is what GCC is expecting to find.
Sounds to me like GCC is the wrong tool for this. But I would of course
not mind if it was *just* the text. However, as your patch cleary
shows, the simplified logic leads to real problems:
> @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct edgeport_serial
> *edge_serial,
> edge_serial->rxState = EXPECT_DATA;
> break;
> }
> - /* Else, drop through */
> }
> + /* fall through */
> case EXPECT_DATA: /* Expect data */
> if (bufferLength < edge_serial->rxBytesRemaining) {
> rxLen = bufferLength;
The original comment clearly marked a *conditional* fall through at the
correct place. Your patch makes it appear as if there is an
unconditional fall through here. There is not. The fallthrough only
applies to one of a number of nested if blocks. There are no less than
3 break statements in the same case block.
Not a big deal maybe, just as the lack of any "fall through" comment
isn't a big deal in the first place. But this change is clearly making
this code harder to read, and the change is therefore harmful IMHO.
If you can't make -Wimplicit-fallthrough work without removing such
precise fallthrough markings, then my proposal is to drop it and use
some other tool.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rndis_host: support Novatel Verizon USB730L — Linux USB
Gal Shalif writes: > Bjoren> You can always unbind the driver, right? And usb_modeswitch will > even do it for you. > I'm not a USB mode switch expert, so please explain what is the mode switch > file that you suggest. > > As I already said before - my kernel configuration will not allow a USB > mode switch (to enterprise mode) without blacklisting the HID interface - > it may be because the HID driver is statically compile into my kernel. > > To summaries: > Without blacklisting - I cannot USB mode switch to USB730L enterprise mode > (product ID 9032). > With blacklisting - it is O.K. > > My kernel configuration: > iMX6 hardware > version 3.14.15 > ARM 32 bit > No IPV6 > Static compile if HID driver It doesn't matter if the driver is a module or built-in. You can still unbind it using the 'unbind' sysfs driver attribute. But I don't think a driver can prevent switching configurations. Please show us what happens when you write 3 to the bConfigurationValue attribute: echo 3 >/sys/bus/usb/devices/x-y/bConfigurationValue where x-y is the usb+port numbers. You can get them from e.g dmesg. And while you are at it: Please show the relevant parts of the dmesg output. lsusb -v output would also be nice to understand what part the HID driver plays here. If driver unbinding is really necessary, then I think you should test if a manual unbind will do. You may be right that some extra magic is needed. But I think it would be good to have a precise description of how that magic works, and not just "With blacklisting - it is O.K.". The hid blacklist connection does not make sense. Or at least it doesn't to me. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rndis_host: support Novatel Verizon USB730L — Linux USB
Oliver Neukum writes: > Am Dienstag, den 24.10.2017, 23:17 +0300 schrieb Gal Shalif: >> The option driver is compiled as a module. >> The option driver patch, and other related patches are listed within >> my previous email: > > Hi, > > but you are adding the device unconditionally to the HID blacklist. > Thus the HID part becomes unusable even if you compile a kernel without > the option driver. I don't understand why the blacklist is necessary at all. You can always unbind the driver, right? And usb_modeswitch will even do it for you. The referenced doc does not seem to mention any HID interface. And all examples seem to switch this modem using a simple "set configuration" request. Changing configs will automatically unbind any interface drivers. So why do we need a blacklist entry here? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: qcserial: add Dell DW5818, DW5819
Shrirang Bagul writes: > On Tue, 2017-10-03 at 15:37 +0200, Johan Hovold wrote: > >> Don't you want to add these to qmi_wwan as well? > > I haven't tested these devices with qmi_wwan. Perhaps a separate patch once > it's > verified. Please do, if you can. I realize that QMI isn't a default config for these modems, but it's always good to have the driver support in case some user wants to switch configs. Note that they most likely support two "RMNET" interfaces, so two entries will be necessary to make the support complete. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rndis_host: support Novatel Verizon USB730L
David Miller writes: > From: Aleksander Morgado > Date: Wed, 27 Sep 2017 23:31:03 +0200 > >> I'm not sure if binding this logic to a specific vid:pid (1410:9030) >> would be more appropriate here, or if it's ok to just bind >> class/subclass/protocol (as in the activesync case). Let me know >> what you think. > > I don't have enough USB Networking knowledge to make a decision here. > > Some things seem confusing. For example, if we should be matching > USB_CLASS_MISC, subclass=4, protocol=1 for RNDIS over Ethernet, then > we why aren't we also matching USB_CLASS_MISC, subclass=4, protocol=2 > for RNDIS over wireless and instead are matching "Remote RNDIS" in > the form of USB_CLASS_WIRELSS, subclass=1, protocol=3? > > I really don't understand any of this magic :-) > > So someone more knowledgable needs to review this. Let me just say that I am not qualified. But since this needs a review, I am going to offer my view anyway. FWIW, I don't think *anyone* understand this magic... I certainly don't. We can pretty much ignore the USB-IF and any specs, since that is what the vendors appear to do. They provide device specific drivers for Windows, so all they care about is that their device "works" with their driver. But in Linux we prefer to create drivers for device classes whenever we can, to avoid having to add every single device by ID. So we try to guess future patterns based on the devices we have observed, even when there is no clear spec. This is what Aleksander does here. He has a device with a 'Cls=ef(misc ) Sub=04 Prot=01' function. This device works with the rndis_host driver. That is all we know. We cannot prove that a class match is correct. But it does make sense to try it. At least we know that this works for one device. Adding anything else, e.g. based on the table at http://www.usb.org/developers/defined_class/#BaseClassEFh , is a bit more risky. We don't know if a driver will work with *any* such device until we've actually seen one. This is just my opinion, and probably full of bogus assumptions as usual. I was sort of hoping that some expert would speak up so I didn't have to :-) But FWIW: Reviewed-by: Bjørn Mork Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: cdc-wdm: ignore -EPIPE from GetEncapsulatedResponse
The driver will forward errors to userspace after turning most of them
into -EIO. But all status codes are not equal. The -EPIPE (stall) in
particular can be seen more as a result of normal USB signaling than
an actual error. The state is automatically cleared by the USB core
without intervention from either driver or userspace.
And most devices and firmwares will never trigger a stall as a result
of GetEncapsulatedResponse. This is in fact a requirement for CDC WDM
devices. Quoting from section 7.1 of the CDC WMC spec revision 1.1:
The function shall not return STALL in response to
GetEncapsulatedResponse.
But this driver is also handling GetEncapsulatedResponse on behalf of
the qmi_wwan and cdc_mbim drivers. Unfortunately the relevant specs
are not as clear wrt stall. So some QMI and MBIM devices *will*
occasionally stall, causing the GetEncapsulatedResponse to return an
-EPIPE status. Translating this into -EIO for userspace has proven to
be harmful. Treating it as an empty read is safer, making the driver
behave as if the device was conforming to the CDC WDM spec.
There have been numerous reports of issues related to -EPIPE errors
from some newer CDC MBIM devices in particular, like for example the
Fibocom L831-EAU. Testing on this device has shown that the issues
go away if we simply ignore the -EPIPE status. Similar handling of
-EPIPE is already known from e.g. usb_get_string()
The -EPIPE log message is still kept to let us track devices with this
unexpected behaviour, hoping that it attracts attention from firmware
developers.
Cc:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100938
Reported-and-tested-by: Christian Ehrig
Reported-and-tested-by: Patrick Chilton
Reported-and-tested-by: Andreas Böhler
Signed-off-by: Bjørn Mork
---
The fallout of this problem has been reported by a number of people
for a long time. Many more than those being listed above. Apologies
to all who didn't get the appropriate credit. Sorry, I am lousy with
paper work and lost track of you all.
Hoping the fix will make up for it...
Bjørn
drivers/usb/class/cdc-wdm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 0b845e550fbd..9f001659807a 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -194,8 +194,10 @@ static void wdm_in_callback(struct urb *urb)
/*
* only set a new error if there is no previous error.
* Errors are only cleared during read/open
+* Avoid propagating -EPIPE (stall) to userspace since it is
+* better handled as an empty read
*/
- if (desc->rerr == 0)
+ if (desc->rerr == 0 && status != -EPIPE)
desc->rerr = status;
if (length + desc->length > desc->wMaxCommand) {
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fibocom L831-EAU and cdc_mbim
Andreas Böhler writes: > As promised I made the new module the default and tested it for a few > days. Although the error messages persist, I did not notice any negative > side effects. In fact, it also fixes a problem where mbim-proxy was at > 100% load. > > Two other users also reported success [1], so I'm looking forward to > having it fixed in the default kernel tree. In the meantime, I'll keep > the module patched using DKMS. Sounds good. Ideally we should also get it tested with lots of other devices. But the only way I know of to make that happen is by pushing the patch into mainline and stable kernels. I'll do some minimal testing myself for a while and then submit. Thanks for your patience. I know this issue has been around for way too long already. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped"
Douglas Anderson writes: > Every once in a while when my system is under a bit of stress I see > some spammy messages show up in my logs that say: > > kevent X may have been dropped > > As far as I can tell these messages aren't terribly useful. I agree, FWIW. These messages just confuse users for no purpose at all. > + /* If work is already started this will mark it to run again when it > + * finishes; if we already had work pending and it hadn't started > + * yet then that's fine too. > + */ > + schedule_work (&dev->kevent); > + netdev_dbg(dev->net, "kevent %d scheduled\n", work); Or maybe if (schedule_work (&dev->kevent)) netdev_dbg(dev->net, "kevent %d scheduled\n", work); ? Not that I think it matters much. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/3] usbnet: Fix memory leak when rx_submit() fails
Douglas Anderson writes:
> If rx_submit() returns an error code then nobody calls usb_free_urb().
> That means it's leaked.
Nope. rx_submit() will call usb_free_urb() before returning an error:
static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
..
if (!skb) {
netif_dbg(dev, rx_err, dev->net, "no rx skb\n");
usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
usb_free_urb (urb);
return -ENOMEM;
}
..
if (retval) {
dev_kfree_skb_any (skb);
usb_free_urb (urb);
}
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fibocom L831-EAU and cdc_mbim
Andreas Böhler writes:
> On 19/09/17 14:25, Bjørn Mork wrote:
>> Bjørn Mork writes:
>>
>> What happens if you kill the mbim-proxy process? Are you then able to
>> use the modem again without resetting it?
>
> I was unable to test that: It can only be killed using SIGKILL, any new
> mbim-proxy processes are then unable to open the device.
>
>>
>> Maybe we should simply ignore stalls completely? They should not happen
>> with any real WDM device anyway.
>>
>> What happens if you e.g. change the if-test here:
>>
>> /*
>> * only set a new error if there is no previous error.
>> * Errors are only cleared during read/open
>> */
>> if (desc->rerr == 0)
>> desc->rerr = status;
>>
>>
>> to
>>
>> if (desc->rerr == 0 && status != -EPIPE)
>
> Well, this looks very promising: Although the error messages persist
> every now and then, the device now works!
>
> I'll make the new module the default and I'll test it more thouroughly
> during the next few days.
>
> Are you planning on adding a more "proper" fix, i.e. without ignoring
> the error? Or is this something that should be "fixed" in libmbim?
If I only knew what the proper fix was... Ignoring the "error" as such
should not be a big problem. It's really just a signal from the
firmware that there is no data available to read. There are many
similar examples in the Linux USB code. For example from the core code
in drivers/usb/core/message.c:
static int usb_get_string(struct usb_device *dev, unsigned short langid,
unsigned char index, void *buf, int size)
{
int i;
int result;
for (i = 0; i < 3; ++i) {
/* retry on length 0 or stall; some devices are flakey */
result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
(USB_DT_STRING << 8) + index, langid, buf, size,
USB_CTRL_GET_TIMEOUT);
if (result == 0 || result == -EPIPE)
continue;
if (result > 1 && ((u8 *) buf)[1] != USB_DT_STRING) {
result = -ENODATA;
continue;
}
break;
}
return result;
}
We cannot do the same looping since we are using async requests of
course. We could add a counter to catch repeated errors, but I feel that
is overkill. Ignoring will do.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fibocom L831-EAU and cdc_mbim
Bjørn Mork writes:
> Thanks a lot. These are very useful. The thing is that I really cannot
> see anything going wrong there. The "failed" capture just ends with a
> sequence of successful MBIM indications being read from the firmware.
>
> So whatever goes wrong looks like something entirely local to the
> driver. I'll try to poke around some more.
I am wondering if Robert added a hint for us here:
if (desc->rerr) {
/*
* Since there was an error, userspace may decide to not read
* any data after poll'ing.
* We should respond to further attempts from the device to send
* data, so that we can get unstuck.
*/
service_outstanding_interrupt(desc);
}
This might solve only half the problem. The driver does not stop
reading from the firmware as we can see in your pcaps. But if userspace
decided to not read any more data from the driver, then that doesn't
help much, does it?
What happens if you kill the mbim-proxy process? Are you then able to
use the modem again without resetting it?
Maybe we should simply ignore stalls completely? They should not happen
with any real WDM device anyway.
What happens if you e.g. change the if-test here:
/*
* only set a new error if there is no previous error.
* Errors are only cleared during read/open
*/
if (desc->rerr == 0)
desc->rerr = status;
to
if (desc->rerr == 0 && status != -EPIPE)
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fibocom L831-EAU and cdc_mbim
Andreas Böhler writes: > On 18/09/17 22:18, Bjørn Mork wrote: >> Andreas Böhler writes: >>> >>> I can also provide you with Wireshark USB traces, if that helps? >> >> It might help. We are obviously interacting badly with the firmware. >> Seeing where and how it stops would be useful. > > Attached are two Wireshark traces, I hope that I captured everything > relevant: > > 1. wwan_init_connect_failed.pcapng.gz > 2. wwan_reset_init_connect_success.pcapng.gz > > I disabled everything Modem-related (i.e. ModemManager), I blacklisted > the cdc* modules then I restarted the machine. The first trace was taken > directly after boot. I loaded the modules and I tried to enable > ModemManager and to connect to the Internet - it failed. > > The second trace was taken after disabling ModemManager and sending > "AT+CFUN=15" to the device, thus resetting it. I then enabled > ModemManager and connected to the Internet - which succeeded. Thanks a lot. These are very useful. The thing is that I really cannot see anything going wrong there. The "failed" capture just ends with a sequence of successful MBIM indications being read from the firmware. So whatever goes wrong looks like something entirely local to the driver. I'll try to poke around some more. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fibocom L831-EAU and cdc_mbim
Andreas Böhler writes: > Unfortunately, it doesn't change anything. I applied the patch, > recompiled the kernel (it updated to 4.13.2 at the same time), rebooted > and upon connecting, I can see the 'cdc_mbim 1-6:1.0: nonzero urb status > received: -EPIPE' message again. Thanks for testing. Then I'll rule out that theory. I guess that's mostly good, since it didn't make much sense :-) > What makes it work for this session only is the following (also works > without your patch, though): > > * connect to /dev/ttyACM0 using a terminal emulator (PuTTY in my case) > * send AT+CFUN=15 to reset the modem > > Then, the device re-appears as /dev/cdc-wdm1 (instead of /dev/cdc-wdm0) > and connecting to the web is possible. If I then disable and re-enable > the modem (in this case, using NetworkManager/ModemManager), the nonzero > urb status message reappears. I can make it work again by resetting the > modem a second time (and stopping ModemManager before the reset, > starting it again after the reset). > > I can also provide you with Wireshark USB traces, if that helps? It might help. We are obviously interacting badly with the firmware. Seeing where and how it stops would be useful. What I don't understand is why the -EPIPE error is permanent. It is an indication from the firmware that there was no data to read (which I believe is a frmware bug, but whatever..). But the stall should be cleared on the next read. Unless the firmware decides to stop sending notifications too? Don't know why it would do that, though. I am just so lost here... Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fibocom L831-EAU and cdc_mbim
Andreas Böhler writes:
> I recently bought a Thinkpad T470 notebook and added the Fibocom
> L831-EAU WWAN card. It's mbim-based, but unfortunately I can't get it
> to work correctly. In fact I'm experiencing similar issues to Patrick
> Chilton, who reported this behaviour a few months ago also on this
> list: https://www.spinics.net/lists/linux-usb/msg158286.html
>
> After a new boot, the card works for me for a few minutes, then all
> transfers stall. Finally, I receive the infamous 'cdc_mbim 1-6:1.0:
> nonzero urb status received: -EPIPE' message in my system log.
>
> I also tried to disable MBIM by adding an option to modprobe. During
> connect, pppd then just hangs up (again similar to what Patrick
> Chilton experienced).
>
> I'm on Arch Linux and Kernel 4.12.13, I tried with and without
> NetworkManager/ModemManager.
>
> Any ideas how I might be able to debug this?
Are you able to test the attached patch? It should apply cleanly to any
of the 4.9, 4.12 and 4.13 stable trees.
Let me just admit that this is a long shot. I don't actually understand
what happens here. But I believe the problem started appearing with
v4.9, which had two patches touching this part of the driver in
suspicious ways. One of those patches have already been reverted due to
other issues. But the second is still present. And I can't see
anything else which could possibly explain the problems.
If you can test this patch, then we can either confirm or refute this
theory.
Bjørn
>From 8fade48507bf4a017420acbe00b96ee2dc003efe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?=
Date: Mon, 18 Sep 2017 12:07:21 +0200
Subject: [PATCH] Revert "cdc-wdm: Clear read pipeline in case of error"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This reverts commit c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829.
Some modem firmwares got stuck in an error state with the queued
error handling, never sending any data which would clear the
driver internal error state.
Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Signed-off-by: Bjørn Mork
---
drivers/usb/class/cdc-wdm.c | 39 ++-
1 file changed, 10 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 5aacea1978a5..2b88eaa4089d 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -150,9 +150,6 @@ static void wdm_out_callback(struct urb *urb)
wake_up(&desc->wait);
}
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
static void wdm_in_callback(struct urb *urb)
{
struct wdm_device *desc = urb->context;
@@ -187,13 +184,7 @@ static void wdm_in_callback(struct urb *urb)
}
}
- /*
- * only set a new error if there is no previous error.
- * Errors are only cleared during read/open
- */
- if (desc->rerr == 0)
- desc->rerr = status;
-
+ desc->rerr = status;
if (length + desc->length > desc->wMaxCommand) {
/* The buffer would overflow */
set_bit(WDM_OVERFLOW, &desc->flags);
@@ -206,19 +197,9 @@ static void wdm_in_callback(struct urb *urb)
}
}
skip_error:
- set_bit(WDM_READ, &desc->flags);
wake_up(&desc->wait);
- if (desc->rerr) {
- /*
- * Since there was an error, userspace may decide to not read
- * any data after poll'ing.
- * We should respond to further attempts from the device to send
- * data, so that we can get unstuck.
- */
- service_outstanding_interrupt(desc);
- }
-
+ set_bit(WDM_READ, &desc->flags);
spin_unlock(&desc->iuspin);
}
@@ -441,14 +422,17 @@ static ssize_t wdm_write
}
/*
- * Submit the read urb if resp_count is non-zero.
+ * clear WDM_READ flag and possibly submit the read urb if resp_count
+ * is non-zero.
*
* Called with desc->iuspin locked
*/
-static int service_outstanding_interrupt(struct wdm_device *desc)
+static int clear_wdm_read_flag(struct wdm_device *desc)
{
int rv = 0;
+ clear_bit(WDM_READ, &desc->flags);
+
/* submit read urb only if the device is waiting for it */
if (!desc->resp_count || !--desc->resp_count)
goto out;
@@ -540,8 +524,7 @@ static ssize_t wdm_read
if (!desc->reslength) { /* zero length read */
dev_dbg(&desc->intf->dev, "zero length - clearing WDM_READ\n");
- clear_bit(WDM_READ, &desc->flags);
- rv = service_outstanding_interrupt(desc);
+ rv = clear_wdm_read_flag(desc);
spin_unlock_irq(&desc->iuspin);
if (rv < 0)
goto err;
@@ -566,10 +549,8 @@ static ssize_t wdm_read
desc->length -= cntr;
/* in case we had outstanding data */
- if (!desc->length) {
- clear_bit(WDM_READ, &desc->flags);
- service_outstanding_interrupt(desc);
- }
+ if (!desc->length)
+ clear_wdm_read_flag(desc);
spin_unlock_irq(&desc->iuspin);
rv = cntr;
--
2.11.0
Re: [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer
Anna-Maria Gleixner writes:
> From: Thomas Gleixner
>
> The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in
> softirq context. This can be also achieved without the tasklet but with
> CLOCK_MONOTONIC_SOFT as hrtimer base.
>
> Signed-off-by: Thomas Gleixner
> Signed-off-by: Anna-Maria Gleixner
> Cc: Oliver Neukum
> Cc: Greg Kroah-Hartman
> Cc: linux-usb@vger.kernel.org
> Cc: net...@vger.kernel.org
> ---
> drivers/net/usb/cdc_ncm.c | 37 -
> include/linux/usb/cdc_ncm.h |2 +-
> 2 files changed, 17 insertions(+), 22 deletions(-)
>
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -61,7 +61,6 @@ static bool prefer_mbim;
> module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM
> functions");
>
> -static void cdc_ncm_txpath_bh(unsigned long param);
> static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
> static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
> static struct usb_driver cdc_ncm_driver;
> @@ -777,10 +776,9 @@ int cdc_ncm_bind_common(struct usbnet *d
> if (!ctx)
> return -ENOMEM;
>
> - hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
> ctx->tx_timer.function = &cdc_ncm_tx_timer_cb;
> - ctx->bh.data = (unsigned long)dev;
> - ctx->bh.func = cdc_ncm_txpath_bh;
> + ctx->usbnet = dev;
> atomic_set(&ctx->stop, 0);
> spin_lock_init(&ctx->mtx);
>
> @@ -967,10 +965,7 @@ void cdc_ncm_unbind(struct usbnet *dev,
>
> atomic_set(&ctx->stop, 1);
>
> - if (hrtimer_active(&ctx->tx_timer))
> - hrtimer_cancel(&ctx->tx_timer);
> -
> - tasklet_kill(&ctx->bh);
> + hrtimer_cancel(&ctx->tx_timer);
>
> /* handle devices with combined control and data interface */
> if (ctx->control == ctx->data)
> @@ -1348,20 +1343,9 @@ static void cdc_ncm_tx_timeout_start(str
> HRTIMER_MODE_REL);
> }
>
> -static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
> +static void cdc_ncm_txpath_bh(struct cdc_ncm_ctx *ctx)
> {
> - struct cdc_ncm_ctx *ctx =
> - container_of(timer, struct cdc_ncm_ctx, tx_timer);
> -
> - if (!atomic_read(&ctx->stop))
> - tasklet_schedule(&ctx->bh);
> - return HRTIMER_NORESTART;
> -}
> -
> -static void cdc_ncm_txpath_bh(unsigned long param)
> -{
> - struct usbnet *dev = (struct usbnet *)param;
> - struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> + struct usbnet *dev = ctx->usbnet;
>
> spin_lock_bh(&ctx->mtx);
> if (ctx->tx_timer_pending != 0) {
> @@ -1379,6 +1363,17 @@ static void cdc_ncm_txpath_bh(unsigned l
> }
> }
>
> +static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
> +{
> + struct cdc_ncm_ctx *ctx =
> + container_of(timer, struct cdc_ncm_ctx, tx_timer);
> +
> + if (!atomic_read(&ctx->stop))
> + cdc_ncm_txpath_bh(ctx);
> +
> + return HRTIMER_NORESTART;
> +}
> +
> struct sk_buff *
> cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
> {
> --- a/include/linux/usb/cdc_ncm.h
> +++ b/include/linux/usb/cdc_ncm.h
> @@ -92,7 +92,6 @@
> struct cdc_ncm_ctx {
> struct usb_cdc_ncm_ntb_parameters ncm_parm;
> struct hrtimer tx_timer;
> - struct tasklet_struct bh;
>
> const struct usb_cdc_ncm_desc *func_desc;
> const struct usb_cdc_mbim_desc *mbim_desc;
> @@ -101,6 +100,7 @@ struct cdc_ncm_ctx {
>
> struct usb_interface *control;
> struct usb_interface *data;
> + struct usbnet *usbnet;
>
> struct sk_buff *tx_curr_skb;
> struct sk_buff *tx_rem_skb;
I believe the struct usbnet pointer is redundant. We already have lots
of pointers back and forth here. This should work, but is not tested:
struct usbnet *dev = usb_get_intfdata(ctx->control):
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: serial: option: simplify 3 D-Link device entries
All the vendor specific interfaces on these devices are serial
functions handled by this driver, so we can use a single class
match entry for each.
P: Vendor=2001 ProdID=7d01 Rev= 3.00
S: Manufacturer=D-Link,Inc
S: Product=D-Link DWM-156
C:* #Ifs= 7 Cfg#= 1 Atr=a0 MxPwr=500mA
A: FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=0e Prot=00
I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=cdc_mbim
E: Ad=88(I) Atr=03(Int.) MxPS= 64 Ivl=125us
I: If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim
I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim
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= 3 Cls=ff(vend.) Sub=02 Prot=01 Driver=option
E: Ad=87(I) Atr=03(Int.) MxPS= 64 Ivl=500us
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=00 Prot=00 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=00 Prot=00 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=00 Prot=00 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=08(stor.) Sub=06 Prot=50 Driver=usb-storage
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: Bjørn Mork
---
Johan Hovold writes:
> Do you want to submit a patch or should I do it?
Well, I can save you that job if this is fine with you. Feel
free to add a stable cc if you think it is appropriate. I am not
sure...
Bjørn
drivers/usb/serial/option.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index fe123153b1a5..05862b75f895 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -2016,12 +2016,9 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(TPLINK_VENDOR_ID, 0x9000),
/* TP-Link MA260 */
.driver_info = (kernel_ulong_t)&net_intf4_blacklist },
{ USB_DEVICE(CHANGHONG_VENDOR_ID, CHANGHONG_PRODUCT_CH690) },
- { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d01, 0xff, 0x02, 0x01) },
/* D-Link DWM-156 (variant) */
- { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d01, 0xff, 0x00, 0x00) },
/* D-Link DWM-156 (variant) */
- { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d02, 0xff, 0x02, 0x01) },
- { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d02, 0xff, 0x00, 0x00) },
- { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x02, 0x01) },
- { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x00, 0x00) },
+ { USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7d01, 0xff) },
/* D-Link DWM-156 (variant) */
+ { USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7d02, 0xff) },
+ { USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7d03, 0xff) },
{ USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7d04, 0xff) },
/* D-Link DWM-158 */
{ USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7e19, 0xff),
/* D-Link DWM-221 B1 */
.driver_info = (kernel_ulong_t)&net_intf4_blacklist },
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: option: add support for D-Link DWM-157 C1
Johan Hovold writes: > On Mon, Aug 28, 2017 at 04:32:53PM +0200, Maciej S. Szmigiero wrote: > >> Also, three other D-Link modems few lines above are using the same >> USB_DEVICE_AND_INTERFACE_INFO() selectors. > > Yeah, I noticed those, and I'm not sure why they're not using > class-matching only either (and note that we have some entries that do). I see that I'm responsible for those. They can probably be compressed to USB_DEVICE_INTERFACE_CLASS(). I assume I just didn't think about it... FWIW, I found this 'devices' listing for the 2001:7d01 device. I believe the 7d02 and 7d03 are similar: P: Vendor=2001 ProdID=7d01 Rev= 3.00 S: Manufacturer=D-Link,Inc S: Product=D-Link DWM-156 C:* #Ifs= 7 Cfg#= 1 Atr=a0 MxPwr=500mA A: FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=0e Prot=00 I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=cdc_mbim E: Ad=88(I) Atr=03(Int.) MxPS= 64 Ivl=125us I: If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim 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= 3 Cls=ff(vend.) Sub=02 Prot=01 Driver=option E: Ad=87(I) Atr=03(Int.) MxPS= 64 Ivl=500us 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=00 Prot=00 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=00 Prot=00 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=00 Prot=00 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=08(stor.) Sub=06 Prot=50 Driver=usb-storage E: Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms E: Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new usb LTE modem device
On August 8, 2017 9:22:29 PM CEST, Giuseppe Lippolis wrote: >> But we already have many Sierra devices with 2 QMI interfaces (the >3rd one >> is documented and verified non-functional for unknown reasons). And >these >> tend to come with multiple OEM device IDs. So a whitelist method >could >> reduce the number of matching entries considerably. Feel free to >submit >> patches if you like :-) > >Dear Bjorg, >I'm going to prepare the following patch. >If you think it is ok, I will build and test it and if working I will >submit it. The qmi_wwan part looks fine to me. But you will need to split it in two patches since the two drivers are parts of different subsystems. The option driver use interface blacklists instead of multiple match entries. You should probably follow the same style there. But this is up to Johan... Use the get_maintainer script to figure out where the patches should be sent. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net,stable] qmi_wwan: fix NULL deref on disconnect
qmi_wwan_disconnect is called twice when disconnecting devices with
separate control and data interfaces. The first invocation will set
the interface data to NULL for both interfaces to flag that the
disconnect has been handled. But the matching NULL check was left
out when qmi_wwan_disconnect was added, resulting in this oops:
usb 2-1.4: USB disconnect, device number 4
qmi_wwan 2-1.4:1.6 wwp0s29u1u4i6: unregister 'qmi_wwan' usb-:00:1d.0-1.4,
WWAN/QMI device
BUG: unable to handle kernel NULL pointer dereference at 00e0
IP: qmi_wwan_disconnect+0x25/0xc0 [qmi_wwan]
PGD 0
P4D 0
Oops: [#1] SMP
Modules linked in:
CPU: 2 PID: 33 Comm: kworker/2:1 Tainted: GE
4.12.3-nr44-normandy-r1500619820+ #1
Hardware name: LENOVO 4291LR7/4291LR7, BIOS CBET4000 4.6-810-g50522254fb
07/21/2017
Workqueue: usb_hub_wq hub_event [usbcore]
task: 8c882b716040 task.stack: b8e800d84000
RIP: 0010:qmi_wwan_disconnect+0x25/0xc0 [qmi_wwan]
RSP: 0018:b8e800d87b38 EFLAGS: 00010246
RAX: RBX: RCX:
RDX: 0001 RSI: 8c8824f3f1d0 RDI: 8c8824ef6400
RBP: 8c8824ef6400 R08: R09:
R10: b8e800d87780 R11: 0011 R12: c07ea0e8
R13: 8c8824e2e000 R14: 8c8824e2e098 R15:
FS: () GS:8c883530() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 00e0 CR3: 000229ca5000 CR4: 000406e0
Call Trace:
? usb_unbind_interface+0x71/0x270 [usbcore]
? device_release_driver_internal+0x154/0x210
? qmi_wwan_unbind+0x6d/0xc0 [qmi_wwan]
? usbnet_disconnect+0x6c/0xf0 [usbnet]
? qmi_wwan_disconnect+0x87/0xc0 [qmi_wwan]
? usb_unbind_interface+0x71/0x270 [usbcore]
? device_release_driver_internal+0x154/0x210
Reported-and-tested-by: Nathaniel Roach
Fixes: c6adf77953bc ("net: usb: qmi_wwan: add qmap mux protocol support")
Cc: Daniele Palmas
Signed-off-by: Bjørn Mork
---
Needed for v4.12 and later
drivers/net/usb/qmi_wwan.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index ff6f39fe6c00..8c3733608271 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -1341,10 +1341,14 @@ static int qmi_wwan_probe(struct usb_interface *intf,
static void qmi_wwan_disconnect(struct usb_interface *intf)
{
struct usbnet *dev = usb_get_intfdata(intf);
- struct qmi_wwan_state *info = (void *)&dev->data;
+ struct qmi_wwan_state *info;
struct list_head *iter;
struct net_device *ldev;
+ /* called twice if separate control and data intf */
+ if (!dev)
+ return;
+ info = (void *)&dev->data;
if (info->flags & QMI_WWAN_FLAG_MUX) {
if (!rtnl_trylock()) {
restart_syscall();
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] append Qualcomm GOBI 2K chipset ID for Panasonic CF-U1 Toughbook
Johan Hovold writes:
> On Mon, Aug 07, 2017 at 04:22:27PM +0200, Oleg Kokorin wrote:
>> From 336f8adb0292a25ea41f0515a80850031f814b9b Mon Sep 17 00:00:00 2001
>> From: Oleg Kokorin
>> Date: Mon, 7 Aug 2017 15:35:53 +0200
>> Subject: [PATCH] append Qualcomm GOBI 2K chipset ID for Panasonic CF-U1
>> Toughbook
>
> Please use
>
> USB: serial: qcserial: add support for Panasonic CF-U1 Toughbook
>
> as Subject (patch summary), and also add a commit message here (e.g.
> your current summary).
>
>> Signed-off-by: Oleg Kokorin
>> ---
>> drivers/usb/serial/qcserial.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
>> index ebc0bee..53786ce 100644
>> --- a/drivers/usb/serial/qcserial.c
>> +++ b/drivers/usb/serial/qcserial.c
>> @@ -76,6 +76,8 @@ static const struct usb_device_id id_table[] = {
>> {DEVICE_G1K(0x1bc7, 0x900e)}, /* Telit Gobi QDL device */
>>
>> /* Gobi 2000 devices */
>> + {USB_DEVICE(0x04da, 0x250e)}, /* Panasonic Gobi 2000 QDL device */
>> + {USB_DEVICE(0x04da, 0x250f)}, /* Panasonic Gobi 2000 Modem device
>> */
>
> This patch is whitespace-damaged and does not apply. Please try sending
> the patch to yourself first (e.g. using git-send-email) and make sure
> you can apply it it with git am. Running checkpatch on the result is
> also a good idea.
And please send a patch for qmi_wwan too, assuming this really is a Gobi
2000 modem. Thanks.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new usb LTE modem device
"Giuseppe Lippolis" writes: > Hi all, > I'm working to port OpenWRT/LEDE on a new router with embedded usb LTE > modem. > The modem have 3 qmi_wwan interfaces and 2 option. > > I would like to prepare the patch, but before I would like to know if using > the QMI_FIXED_INTF macro is the best way to identify the interface or if > there is a better way. That is currently the only implemented way, so it's definitely simplest. And most likely the only method suitable for stable backports. I agree that it is very inefficient to add 3 match lines for a single device. We could consider adding an alternative matching method for this kind of device, using an interface whitelist or blacklist. I guess we should if there are many more of these. But we already have many Sierra devices with 2 QMI interfaces (the 3rd one is documented and verified non-functional for unknown reasons). And these tend to come with multiple OEM device IDs. So a whitelist method could reduce the number of matching entries considerably. Feel free to submit patches if you like :-) > Here the info grapped from cat /proc/bus/usb/devices using the stock > firmware: > > T: Bus=02 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 2 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=1435 ProdID=0918 Rev= 2.32 > S: Manufacturer=Android > S: Product=Android > S: SerialNumber=0123456789ABCDEF > C:* #Ifs= 7 Cfg#= 1 Atr=80 MxPwr=500mA > I:* If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option > E: Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=(none) > E: Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option > E: Ad=84(I) Atr=03(Int.) MxPS= 64 Ivl=32ms > E: Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > E: Ad=86(I) Atr=03(Int.) MxPS= 64 Ivl=32ms > E: Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > E: Ad=88(I) Atr=03(Int.) MxPS= 64 Ivl=32ms > E: Ad=87(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 5 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > E: Ad=8a(I) Atr=03(Int.) MxPS= 64 Ivl=32ms > E: Ad=89(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms > I:* If#= 6 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=(none) > E: Ad=8b(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms > E: Ad=07(O) Atr=02(Bulk) MxPS= 512 Ivl=125us Two questions: Did you verify that all 3 QMI interfaces work as expected? What is the storage function (last interface) good for on an embedded USB modem? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mfd: Add support for FTDI FT232H devices
Johan Hovold writes: > On Tue, Jul 11, 2017 at 08:52:37AM +0200, Anatolij Gustschin wrote: > >> For devices with connected EEPROM some modes (including UART) are >> configurable in the EEPROM. For devices without EEPROM the default >> mode is always UART, but FIFO-, Bitbang- and MPSSE-mode can be >> switched via commands to the the chip. > > IIRC we should be able read from the EEPROM, and I would at least expect > there to be a way to retrieve the current mode as well. Stupid question, I know, but I cannot help thinking: If you have an EEPROM then why the h... don't you use an application specific device ID? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] Set NTB format again after altsetting switch for Huawei devices
Oliver Neukum writes: > Am Dienstag, den 11.07.2017, 17:21 +0200 schrieb Enrico Mioso: >> Some firmwares in Huawei E3372H devices have been observed to switch back >> to NTB 32-bit format after altsetting switch. >> This patch implements a driver flag to check for the device settings and >> set NTB format to 16-bit again if needed. >> The flag has been activated for devices controlled by the huawei_cdc_ncm.c >> driver. > > Hi, > > does this cover reset_resume()? AFAIK reset_resume doesn't really work for 3G/LTE modems. Too much state to restore both in firmware and network, with no well defined protocol to restore it. The only reason for having reset_resume in the modem drivers is that quite a few modems have an SD reader. We need to support persist for the usb-storage function, even if the modem functions fail. There is no reason to mess up the file system too. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC PATCH] Set NTB format again after altsetting switch for Huawei devices
Enrico Mioso writes: > Some firmwares in Huawei E3372H devices have been observed to switch back > to NTB 32-bit format after altsetting switch. > This patch implement a driver flag to check for the device settings and > set NTB format to 16-bit again if needed. > The flag has been activated for devices controlled by the huawei_cdc_ncm > driver. > I am not in a confortable position to test this code, so it may containg > serious defects. > Let me know. This looks good to me if it solves the problem for Christian. Note that you will have to resend it to netdev to have it applied, but I guess you know that :-) Feel free to add Reviewed-by: Bjørn Mork when you do so, if you want. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: cdc_ncm: Specific Huawei E3372h firmware version stuck in NTB-32
[CCing Enrico]
Christian Panton writes:
> Found a solution.
>
>>> I have two mobile broadband Huawei E3372h devices, one with firmware
>>> 21.200.07.01.26 (aka the 200-version) and one with firmware
>>> 21.318.01.00.541 (aka the 318-version).
>>>
>>> Whereas the 200-version works perfectly with a recent kernel (4.10),
>>> the latter never manages to exchange any IP-packets. Upon debugging
>>> USB-traces, I found that the 200-version correctly switches to NTB-16,
>>> whereas the 318-version stays in NTB-32 mode.
>>
>> And both these firmwares use the cdc_ncm class driver directly, and not
>> the huawei_cdc_ncm driver? I.e. they appear as true CDC NCM class
>> devices
>
> They appear to use the huawei_cdc_ncm driver. I am not at all familiar with
> driver nor kernel architecture, so I am sorry if I am being a bit vague.
OK, then all bets regarding specs are off since this is a vendor
specific function. They can do whatever they want with it.
>> Could you test if
>>
>> echo Y >/sys/class/net/xxx/cdc_ncm/ndp_to_end
>>
>> makes any difference, where xxx is the name of your wwan netdev?
>
> It did not make any difference. See root cause below.
No, that is expected in this case. The huawei_cdc_ncm driver already
enforce this.
>> Section 7.2 "Using Alternate Settings to Reset an NCM Function":
>>
>> "Whenever alternate setting 0 is selected by the host, the function
>> shall:
>> ..
>> - reset the NTB format to NTB-16
>> “
>
> I downloaded the spec, and started to investigate. Comparing Windows driver
> traces to the spec, I noticed that the Windows driver queried GET_NTB_FORMAT,
> prior to setting it to NTB-32. It was always 0x0001h (aka. NTB-32), directly
> after coming out of alternate setting = 0.
>
> So I threw together a small python-usb script, to see what was different
> between the two devices I had.
>
> My testing sequence is:
>
> alt_setting = 1
> alt_setting = 0
> GET_NTB_PARAMETERS
> GET_NTB_FORMAT (#A)
> SET_CRC_MODE
> SET_NTB_FORMAT
> GET_NTB_FORMAT (#B)
> alt_setting = 1
> GET_NTB_FORMAT (#C)
> SET_NTB_FORMAT
> GET_NTB_FORMAT (#D)
>
> BOTH devices come out of alt_setting = 0 (after step #A) with GET_NTB_FORMAT
> = 0x0001h!
>
> Readout of GET_NTB_FORMAT:
>
> #A200: 0x0001 318: 0x0001 <— bug in both
> #B200: 0x 318: 0x
> #C200: 0x 318: 0x0001 <— bug in 318
> #D200: 0x 318: 0x
>
> So it is clear, that the 318-firmware resets to NTB-32 after alt_setting = 1
>
> It is clearly non-standard behaviour.
>
> The spec concerning SET_NTB_FORMAT reads:
> "The host shall only send this command while the NCM Data Interface is in
> alternate setting 0.”
Yes, and violating this is known to cause issues with other devices.
> So I blindly added:
>
> usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
> USB_TYPE_CLASS | USB_DIR_OUT
> | USB_RECIP_INTERFACE,
> USB_CDC_NCM_NTB16_FORMAT,
> iface_no, NULL, 0);
>
> in cdc_ncm_bind_common after the interface was switched back with
> alt_setting = 1. Now the device works.
>
> I have zero experience whether this should or could be implemented in the
> driver, nor do I have any kernel development experience.
>
> A solution in pseudocode:
>
> If GET_NTB_FORMAT == 0x0001 { // no restraints on alt setting according to
> spec
> // device is still in NTB-32 mode, which clearly indicates
> // a firmware that is not in the accordance with the spec
> Run SET_NTB_FORMAT(0x)
> }
Maybe we can do that? GET_NTB_FORMAT is allowed in altsetting 1 if the
device supports multiple formats. And as it should always return
USB_CDC_NCM_NTB16_FORMAT (0) here, the SET_NTB_FORMAT will never be
sent unless the device is buggy. So we do not violate the spec.
What do you think, Enrico?
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
