Message-ID: <87ehv0eoah.fsf@nemi.mork.no>
Date:	Sun, 15 Jan 2012 20:52:38 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Oliver Neukum <oliver@...kum.org>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Dan Williams <dcbw@...hat.com>,
	Thomas Schäfer <tschaefer@...nline.de>
Subject: Re: [PATCH v2 2/3] net: usb: cdc_enc: New driver exposing USB CDC encapsulated protocols

Bjørn Mork <bjorn@...k.no> writes:
> Oliver Neukum <oliver@...kum.org> writes:
>> Am Sonntag, 15. Januar 2012, 07:40:36 schrieb Bjørn Mork:
>>> USB CDC Ethernet devices may encapsulate vendor specific protocols inside
>>> USB_CDC_SEND_ENCAPSULATED_COMMAND +USB_CDC_GET_ENCAPSULATED_RESPONSE
>>> control messages. Devices based on Qualcomm MSM
>>> chipsets are known to use this for exporting the Qualcomm
>>> MSM Interface (QMI) protocol. Examples of such devices are
>>> the Huawei E392 and E398 LTE modems.
>>
>> The problem is that this is another version of the code already
>> in cdc-wdm, which is tested well.
>
> Huh?  I thought that was something entirely different.  Isn't it?

I should look at things before asking.  I see what you mean.  The
cdc-wdm code is very similar, and should be fairly easy to adapt.  Why
didn't I just research that from the start?  Well, you learn more from
doing such mistakes ;-)


There is one fundamental design issue which worries me though: cdc-wdm
doesn't support multiple simultaneous readers/writers.  This is sort of
a blocker for my use case, as I will need to keep a reader/writer for
the driver internal wwan management at all times.  So two simultaneous
clients are necessary to actually have something available for
userspace at all.

Are there any reasons for this choice, and do you think it's feasible
changing it without destroying the advantage of the well tested code?  I
seem to remember that cdc-wdm allowed simultaneous reading/writing in
the past, but this changed somewhere between 2.6.32 and 3.0. I have an
Ericsson modem with two such AT command speaking interfaces, and I use a
simple shell script to configure it.  At one point I had to switch from
using /dev/cdc-wdm0 to /dev/ttyACM0 because the former started hanging
on constructs like this:

    while read x; do
    case "$x" in
         foo)
              echo -e "AT+bar\r" >/dev/cdc-wdm0
              ;;
         ...
        esac
    done </dev/cdc-wdm0
              

This used to work with 2.6.32, but that might have been just pure luck?
Anyway, that change really made cdc-wdm much less usable for me, and
that sort of proves that supporting simultaneous read/write would be an
improvement even for the existing supported devices.

And then there are a couple of minor issues which I believe needs
sorting out.  If I understand the WDM spec correctly (without bothering
to actually read if, of course), then it only defines the interface but
not the protocol.  Exactly like the embedded CDC Ethernet command
interface.  So I have the Ericsson modem which which speaks an AT
command protocol, of course with a number of vendor specific AT commands
included.  And then I have the Huawei modem which speaks QMI.  Just
mapping these to a number of /dev/cdc-wdmX interfaces isn't going to
help user applications much.  They need to be told *what* protocol to
use on the character device.  And the driver should know this, either by
detection or maybe best: vid/pid based tables and sane fallbacks.


I'm not sure this will do (if the latter was changed to a cdc-wdmX
device):

bjorn@...i:~$ grep . /sys/class/usb/cdc-wdm0/device/*
/sys/class/usb/cdc-wdm0/device/bAlternateSetting: 0
/sys/class/usb/cdc-wdm0/device/bInterfaceClass:02
/sys/class/usb/cdc-wdm0/device/bInterfaceNumber:05
/sys/class/usb/cdc-wdm0/device/bInterfaceProtocol:01
/sys/class/usb/cdc-wdm0/device/bInterfaceSubClass:09
/sys/class/usb/cdc-wdm0/device/bNumEndpoints:01
/sys/class/usb/cdc-wdm0/device/interface:Ericsson F3507g Mobile Broadband Minicard Device Management
/sys/class/usb/cdc-wdm0/device/modalias:usb:v0BDBp1900d0000dc02dsc00dp00ic02isc09ip01
/sys/class/usb/cdc-wdm0/device/supports_autosuspend:1
/sys/class/usb/cdc-wdm0/device/uevent:DEVTYPE=usb_interface
/sys/class/usb/cdc-wdm0/device/uevent:DRIVER=cdc_wdm
/sys/class/usb/cdc-wdm0/device/uevent:DEVICE=/proc/bus/usb/002/056
/sys/class/usb/cdc-wdm0/device/uevent:PRODUCT=bdb/1900/0
/sys/class/usb/cdc-wdm0/device/uevent:TYPE=2/0/0
/sys/class/usb/cdc-wdm0/device/uevent:INTERFACE=2/9/1
/sys/class/usb/cdc-wdm0/device/uevent:MODALIAS=usb:v0BDBp1900d0000dc02dsc00dp00ic02isc09ip01

bjorn@...ardo:~$ grep . /sys/class/cdc_enc/cdc-enc0/device/*
/sys/class/cdc_enc/cdc-enc0/device/bAlternateSetting: 0
/sys/class/cdc_enc/cdc-enc0/device/bInterfaceClass:ff
/sys/class/cdc_enc/cdc-enc0/device/bInterfaceNumber:03
/sys/class/cdc_enc/cdc-enc0/device/bInterfaceProtocol:09
/sys/class/cdc_enc/cdc-enc0/device/bInterfaceSubClass:01
/sys/class/cdc_enc/cdc-enc0/device/bNumEndpoints:01
/sys/class/cdc_enc/cdc-enc0/device/modalias:usb:v12D1p1506d0000dc00dsc00dp00icFFisc01ip09
/sys/class/cdc_enc/cdc-enc0/device/supports_autosuspend:1
/sys/class/cdc_enc/cdc-enc0/device/uevent:DEVTYPE=usb_interface
/sys/class/cdc_enc/cdc-enc0/device/uevent:DRIVER=qmi_wwan
/sys/class/cdc_enc/cdc-enc0/device/uevent:DEVICE=/proc/bus/usb/001/020
/sys/class/cdc_enc/cdc-enc0/device/uevent:PRODUCT=12d1/1506/0
/sys/class/cdc_enc/cdc-enc0/device/uevent:TYPE=0/0/0
/sys/class/cdc_enc/cdc-enc0/device/uevent:INTERFACE=255/1/9
/sys/class/cdc_enc/cdc-enc0/device/uevent:MODALIAS=usb:v12D1p1506d0000dc00dsc00dp00icFFisc01ip09

So my proposal have been something like a "protocol" attribute:

bjorn@...ardo:~$ grep . /sys/class/cdc_enc/cdc-enc0/*
/sys/class/cdc_enc/cdc-enc0/dev:250:0
/sys/class/cdc_enc/cdc-enc0/protocol:qmi
/sys/class/cdc_enc/cdc-enc0/uevent:MAJOR=250
/sys/class/cdc_enc/cdc-enc0/uevent:MINOR=0
/sys/class/cdc_enc/cdc-enc0/uevent:DEVNAME=cdc-enc0


bjorn@...i:~$ grep . /sys/class/usb/cdc-wdm0/*
/sys/class/usb/cdc-wdm0/dev:180:0
/sys/class/usb/cdc-wdm0/uevent:MAJOR=180
/sys/class/usb/cdc-wdm0/uevent:MINOR=0
/sys/class/usb/cdc-wdm0/uevent:DEVNAME=cdc-wdm0

Do you think it would be possible to add that (or similar meta data) to
the cdc-wdmX devices?  Maybe letting it default to "AT"?  Or "unknown"?
Any thoughts on this?  How do actual user applications deal with this?




Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists