Patchwork TODO FLAG_POINTTOPOINT => FLAG_WWAN? usbnet/cdc_ncm: mark ncm devices as "mobile broadband devices" with FLAG_WWAN

login
register
mail settings
Submitter Stefan Metzmacher
Date June 1, 2011, 10:08 a.m.
Message ID <1306922913-17803-2-git-send-email-metze@samba.org>
Download mbox | patch
Permalink /patch/98162/
State Rejected
Delegated to: David Miller
Headers show

Comments

Stefan Metzmacher - June 1, 2011, 10:08 a.m.
Commit e1e499eef2200c2a7120c9ebf297d48b195cf887 does the same for
cdc_ether (with lets the Ericsson F3507g modem appear as wwanX device).

With this commit also the Ericsson F5521gw modem becomes a wwanX device.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 drivers/net/usb/cdc_ncm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Alexey ORISHKO - June 1, 2011, 10:20 a.m.
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Stefan Metzmacher
> Sent: Wednesday, June 01, 2011 12:09 PM


> -	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> +	.flags = FLAG_WWAN | FLAG_NO_SETINT | FLAG_MULTI_PACKET,

This patch will introduce incompatibility with already existing
applications, which track usbX devices. As a result, end user
application will stop working.

cdc_ncm driver can also be used for local link terminated in device,
so wwan would be inappropriate name.

Regards,
Alexey


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Metzmacher - June 1, 2011, 10:47 a.m.
Am 01.06.2011 12:20, schrieb Alexey ORISHKO:
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Stefan Metzmacher
>> Sent: Wednesday, June 01, 2011 12:09 PM
> 
> 
>> -	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
>> +	.flags = FLAG_WWAN | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> 
> This patch will introduce incompatibility with already existing
> applications, which track usbX devices. As a result, end user
> application will stop working.

The same argument would apply to commits
e1e499eef2200c2a7120c9ebf297d48b195cf887 and
c261344d3ce3edac781f9d3c7eabe2e96d8e8fe8,
while they made it into the upstream kernel.

So I would think that such a change should be ok for a next release,
but without backport to stable branches.

> cdc_ncm driver can also be used for local link terminated in device,
> so wwan would be inappropriate name.

Would it be possible to add some autodetection for known devices,
in a similar way the cdc_ether driver does it.

It's really strange that Ericsson F3507g and Ericsson F5521gw appear as
different kind of devices, while using the same kernel version.

And Ericsson F3507g already changed from usbX to wwanX between 2.6.32
(ubuntu 10.04)
and 2.6.38 (ubuntu 11.04).

metze
Alexey ORISHKO - June 1, 2011, 11:39 a.m.
> -----Original Message-----
> From: Stefan (metze) Metzmacher [mailto:metze@samba.org]
> Sent: Wednesday, June 01, 2011 12:47 PM
> 
> Would it be possible to add some autodetection for known devices, in a
> similar way the cdc_ether driver does it.

If you use udev rules and create an alias for your device, you won't
be dependent on generic driver code.

Regards,
alexey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Metzmacher - June 1, 2011, 11:54 a.m.
Am 01.06.2011 13:39, schrieb Alexey ORISHKO:
>> -----Original Message-----
>> From: Stefan (metze) Metzmacher [mailto:metze@samba.org]
>> Sent: Wednesday, June 01, 2011 12:47 PM
>>
>> Would it be possible to add some autodetection for known devices, in a
>> similar way the cdc_ether driver does it.
> 
> If you use udev rules and create an alias for your device, you won't
> be dependent on generic driver code.

Ok, do you have examples for that?

metze
Alexey ORISHKO - June 1, 2011, 12:27 p.m.
> -----Original Message-----
> From: Stefan (metze) Metzmacher [mailto:metze@samba.org]
> Sent: Wednesday, June 01, 2011 1:55 PM

> > If you use udev rules and create an alias for your device, you won't
> > be dependent on generic driver code.
> 
> Ok, do you have examples for that?

Please, read udev man pages. Rule might be as follows
KERNEL=="usb*", ATTRS{idVendor}=="04cc", ATTRS{idProduct}=="xxxx", NAME="ste_bridge0"

Regards,
alexey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - June 2, 2011, 10:29 p.m.
On Wed, 2011-06-01 at 12:20 +0200, Alexey ORISHKO wrote:
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > owner@vger.kernel.org] On Behalf Of Stefan Metzmacher
> > Sent: Wednesday, June 01, 2011 12:09 PM
> 
> 
> > -	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> > +	.flags = FLAG_WWAN | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> 
> This patch will introduce incompatibility with already existing
> applications, which track usbX devices. As a result, end user
> application will stop working.

Applications should *never* track devices based solely on a device name
prefix.  What do they do when the device gets renamed either by udev
rules or the user?  It's simply broken.  Device names are not stable API
and they can and do change at will.  Applications that expect them to
have a stable prefix are simply broken.

Dan


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - June 2, 2011, 10:35 p.m.
On Thu, 2011-06-02 at 17:29 -0500, Dan Williams wrote:
> On Wed, 2011-06-01 at 12:20 +0200, Alexey ORISHKO wrote:
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > > owner@vger.kernel.org] On Behalf Of Stefan Metzmacher
> > > Sent: Wednesday, June 01, 2011 12:09 PM
> > 
> > 
> > > -	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> > > +	.flags = FLAG_WWAN | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> > 
> > This patch will introduce incompatibility with already existing
> > applications, which track usbX devices. As a result, end user
> > application will stop working.
> 
> Applications should *never* track devices based solely on a device name
> prefix.  What do they do when the device gets renamed either by udev
> rules or the user?  It's simply broken.  Device names are not stable API
> and they can and do change at will.  Applications that expect them to
> have a stable prefix are simply broken.

A follow-on to this is that if you really care about specific devices,
your application can use udev rules to "tag" specific interfaces based
on USB VID/PID/GUID or other device attributes, and check for those tags
in your program.  Use udev (good) or netlink (good) or SIOCGIFCONF (bad)
to enumerate the various network interfaces on the system and pick the
one you want using the udev tags instead of hardcoding stuff that will
inevitably break, as you've seen here.  Yeah, it's more code.  But hey,
it doesn't break when people do something you don't expect!

Dan


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Valdis.Kletnieks@vt.edu - June 3, 2011, 12:58 a.m.
On Thu, 02 Jun 2011 17:35:31 CDT, Dan Williams said:
> On Thu, 2011-06-02 at 17:29 -0500, Dan Williams wrote:
> > On Wed, 2011-06-01 at 12:20 +0200, Alexey ORISHKO wrote:
> > > > -----Original Message-----
> > > > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > > > owner@vger.kernel.org] On Behalf Of Stefan Metzmacher
> > > > Sent: Wednesday, June 01, 2011 12:09 PM
> > > 
> > > 
> > > > -	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> > > > +	.flags = FLAG_WWAN | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
> > > 
> > > This patch will introduce incompatibility with already existing
> > > applications, which track usbX devices. As a result, end user
> > > application will stop working.

I'm interpreting this as "devices which currently often get named usbX by udev,
and currently have FLAG_POINTTOPOINT set".  People are getting hung up
on the usbX part, not what the *real* problem is.

> A follow-on to this is that if you really care about specific devices,
> your application can use udev rules to "tag" specific interfaces based
> on USB VID/PID/GUID or other device attributes, and check for those tags
> in your program.  Use udev (good) or netlink (good) or SIOCGIFCONF (bad)
> to enumerate the various network interfaces on the system and pick the

I think Alexey's point was that the patch will hose up programs that
currently do the netlink or  SIOCGIFCONF thing and look for FLAG_POINTTOPOINT.
Stefan Metzmacher - June 3, 2011, 9:35 a.m.
Am 01.06.2011 14:27, schrieb Alexey ORISHKO:
>> -----Original Message-----
>> From: Stefan (metze) Metzmacher [mailto:metze@samba.org]
>> Sent: Wednesday, June 01, 2011 1:55 PM
> 
>>> If you use udev rules and create an alias for your device, you won't
>>> be dependent on generic driver code.
>>
>> Ok, do you have examples for that?
> 
> Please, read udev man pages. Rule might be as follows
> KERNEL=="usb*", ATTRS{idVendor}=="04cc", ATTRS{idProduct}=="xxxx", NAME="ste_bridge0"

Thanks! I got something that works.

metze
Alexey ORISHKO - June 3, 2011, 9:45 a.m.
> -----Original Message-----
> From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks@vt.edu]
> Sent: Friday, June 03, 2011 2:58 AM
> 
> > A follow-on to this is that if you really care about specific
> devices,
> > your application can use udev rules to "tag" specific interfaces
> based
> > on USB VID/PID/GUID or other device attributes, and check for those
> > tags in your program.  Use udev (good) or netlink (good) or
> > SIOCGIFCONF (bad) to enumerate the various network interfaces on the
> > system and pick the
> 
> I think Alexey's point was that the patch will hose up programs that
> currently do the netlink or  SIOCGIFCONF thing and look for
> FLAG_POINTTOPOINT.

Just to clarify, I was objecting to renaming interface name mostly because
devices which use CDC NCM function might be something different from wwan
devices. I would prefer to keep a generic name of interface (usbX or ethX).

As an option anyone can use udev rules to set interface name they want 
for their device based on VID/PID or MAC address or something else.
I've already provided udev rule example earlier in this thread.

Dan, is it in line with your statement?

/alexey

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - June 3, 2011, 10:01 a.m.
Am Freitag, 3. Juni 2011, 11:45:38 schrieb Alexey ORISHKO:
> > -----Original Message-----
> > From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks@vt.edu]
> > Sent: Friday, June 03, 2011 2:58 AM
> > 
> > > A follow-on to this is that if you really care about specific
> > devices,
> > > your application can use udev rules to "tag" specific interfaces
> > based
> > > on USB VID/PID/GUID or other device attributes, and check for those
> > > tags in your program.  Use udev (good) or netlink (good) or
> > > SIOCGIFCONF (bad) to enumerate the various network interfaces on the
> > > system and pick the
> > 
> > I think Alexey's point was that the patch will hose up programs that
> > currently do the netlink or  SIOCGIFCONF thing and look for
> > FLAG_POINTTOPOINT.
> 
> Just to clarify, I was objecting to renaming interface name mostly because
> devices which use CDC NCM function might be something different from wwan
> devices. I would prefer to keep a generic name of interface (usbX or ethX).
> 
> As an option anyone can use udev rules to set interface name they want 
> for their device based on VID/PID or MAC address or something else.
> I've already provided udev rule example earlier in this thread.

This is not ideal. Distributions cannot care about every VIP:PID value.
If a device with an NCM interface needs to be treated in a special
manner we'd better have a special name for such interfaces.

	Regards
		Oliver
Alexey ORISHKO - June 3, 2011, 10:23 a.m.
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.de]
> Sent: Friday, June 03, 2011 12:01 PM
> 
> This is not ideal. Distributions cannot care about every VIP:PID value.
> If a device with an NCM interface needs to be treated in a special
> manner we'd better have a special name for such interfaces.

There is no difference between cdc_ncm and cdc_ether devices, besides
the fact cdc_ncm is more efficient in data transfer and cpu load.
The problem is that you cannot say much about device functionality based
just on interface name. As example existing devices with cdc_ether 
interface could be mobile phone which provides connectivity to 3G network,
or could be a phone in the mode "over PC", where phone applications
access internet via broadband connection on PC (to save money).

Network manager cannot simply make decision which devices can be used
for mobile broadband access based on interface name. It needs an additional
info, which does not exist in device descriptor. 

So, back to original question, is there any point to rename an interface name,
if it can't be guaranteed that any device would be wwan device?

/alexey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - June 3, 2011, 10:50 a.m.
Am Freitag, 3. Juni 2011, 12:23:21 schrieb Alexey ORISHKO:

> There is no difference between cdc_ncm and cdc_ether devices, besides
> the fact cdc_ncm is more efficient in data transfer and cpu load.
> The problem is that you cannot say much about device functionality based
> just on interface name. As example existing devices with cdc_ether 
> interface could be mobile phone which provides connectivity to 3G network,
> or could be a phone in the mode "over PC", where phone applications
> access internet via broadband connection on PC (to save money).
> 
> Network manager cannot simply make decision which devices can be used
> for mobile broadband access based on interface name. It needs an additional
> info, which does not exist in device descriptor. 

Well, but cdc-ether usually means that you can start up dhcp and use the
interface as a network card. Can the same be done with cdc-ncm or do you always
need to establish a connection through a secondary interface?

	Regards
		Oliver
Alexey ORISHKO - June 3, 2011, 11:47 a.m.
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.de]
> Sent: Friday, June 03, 2011 12:50 PM
> 
> Well, but cdc-ether usually means that you can start up dhcp and use
> the
> interface as a network card. Can the same be done with cdc-ncm or do
> you always
> need to establish a connection through a secondary interface?
> 

Some solutions (also based on cdc_ether driver) present IP address assigned
by 3G network. Initially device carrier is OFF. As soon as 3G network PDP
context is established, device send notification 
USB_CDC_NOTIFY_NETWORK_CONNECTION and host driver set carrier ON.

In such a case the problem is the lack of control channel definition in both
CDC ECM and CDC NCM. In order for mobile device to setup 3G connection
some application from PC must setup PDP context. The usual way to do it via
modem by using AT commands. So, it might be CDC ACM or some proprietary
solution. 
As a result you have to have vendor specific solution to find "real" control
channel (/dev/ttyACMx or other) and setup connection.
The need to know interface name is needed if you want to set default
route to that interface. Do you want to do it while you have pc broadband 
connection at hand? Probably not. 
I've tried to run 3g modem with cdc_ncm on Ubuntu 8 and later without any
need to specify interface names, same for Fedora.

So connection manager has to know: control interface, vendor- 
specific (or ever product specific) AT command sequence and optionally 
network interface name.
To sort this out, someone need either to track VID/PID or get
information by other means, for example via AT channel by guessing the
right tty device name.

Anyway, all this discussion is user space application problems.
Solution would be different from vendor to vendor.

/alexey

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork - June 3, 2011, 12:31 p.m.
Oliver Neukum <oneukum@suse.de> writes:

> Well, but cdc-ether usually means that you can start up dhcp and use the
> interface as a network card. Can the same be done with cdc-ncm or do you always
> need to establish a connection through a secondary interface?

Is there anything in the class definition that prevents either option,
for either protocol? No?

So, could we please kill the device guessing game?  Making decisions
based on absolute measures like "do we have a global mac address?" do
make some sense.  But guessing based on USB class does not.  That will
only tell you the protocol for the USB link. If you want more specific
device information, then you will have to look at the vid/pid. And I
assume you don't want to put all of those into the kernel when a class
driver will do, and whatever additional device specific information just
as well can be added by udev rules. Preferably created by whatever
application wishing to support the device.

But I do also assume you know all this already...



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - June 3, 2011, 7:24 p.m.
On Fri, 2011-06-03 at 11:45 +0200, Alexey ORISHKO wrote:
> > -----Original Message-----
> > From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks@vt.edu]
> > Sent: Friday, June 03, 2011 2:58 AM
> > 
> > > A follow-on to this is that if you really care about specific
> > devices,
> > > your application can use udev rules to "tag" specific interfaces
> > based
> > > on USB VID/PID/GUID or other device attributes, and check for those
> > > tags in your program.  Use udev (good) or netlink (good) or
> > > SIOCGIFCONF (bad) to enumerate the various network interfaces on the
> > > system and pick the
> > 
> > I think Alexey's point was that the patch will hose up programs that
> > currently do the netlink or  SIOCGIFCONF thing and look for
> > FLAG_POINTTOPOINT.
> 
> Just to clarify, I was objecting to renaming interface name mostly because
> devices which use CDC NCM function might be something different from wwan
> devices. I would prefer to keep a generic name of interface (usbX or ethX).
> 
> As an option anyone can use udev rules to set interface name they want 
> for their device based on VID/PID or MAC address or something else.
> I've already provided udev rule example earlier in this thread.
> 
> Dan, is it in line with your statement?

Sort of; the point of 'wwan' is to detect when we need some auxiliary
configuration to make the net interface actually do something.  It
allows userspace tools (like NetworkManager) to treat ethernet
interfaces that do require aux config specially.  Matching driver name
is not the solution here since as you said the driver can drive a wide
variety of hardware not all of which may be WWAN.

But there are a bunch of devices we *know* are 3G devices, and for those
we should mark them as WWAN.  And the best place to do that is in the
driver where that device's USB IDs and/or workarounds may exist, just
like the existing cdc-wdm stuff for Ericsson MBM minicards.  This
ensures that we keep this information in *one* place, instead of
sprinkling pieces around between userspace udev rules, apps, and the
driver.

Dan


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

Patch

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 3257aaa..21f44ab 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1217,7 +1217,7 @@  static int cdc_ncm_manage_power(struct usbnet *dev, int status)
 
 static const struct driver_info cdc_ncm_info = {
 	.description = "CDC NCM",
-	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
+	.flags = FLAG_WWAN | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
 	.bind = cdc_ncm_bind,
 	.unbind = cdc_ncm_unbind,
 	.check_connect = cdc_ncm_check_connect,