* [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility
@ 2013-03-14 11:05 Bjørn Mork
[not found] ` <1363259113-6909-1-git-send-email-bjorn-yOkvZcmFvRU@public.gmane.org>
2013-03-17 16:00 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Bjørn Mork @ 2013-03-14 11:05 UTC (permalink / raw)
To: netdev; +Cc: linux-usb, Bjørn Mork, Greg Suarez, Alexey Orishko
commit bd329e1 ("net: cdc_ncm: do not bind to NCM compatible MBIM devices")
introduced a new policy, preferring MBIM for dual NCM/MBIM functions if
the cdc_mbim driver was enabled. This caused a regression for users
wanting to use NCM.
Devices implementing NCM backwards compatibility according to section
3.2 of the MBIM v1.0 specification allow either NCM or MBIM on a single
USB function, using different altsettings. The cdc_ncm and cdc_mbim
drivers will both probe such functions, and must agree on a common
policy for selecting either MBIM or NCM. Until now, this policy has
been set at build time based on CONFIG_USB_NET_CDC_MBIM.
Use a module parameter to set the system policy at runtime, allowing the
user to prefer NCM on systems with the cdc_mbim driver.
Cc: Greg Suarez <gsuarez@smithmicro•com>
Cc: Alexey Orishko <alexey.orishko@stericsson•com>
Reported-by: Geir Haatveit <nospam@haatveit•nu>
Reported-by: Tommi Kyntola <kynde@ts•ray.fi>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=54791
Signed-off-by: Bjørn Mork <bjorn@mork•no>
---
We now have two users independently reporting this as a 3.8 regression,
so something needs to be done. I am not sure if adding a new module
parameter is acceptable for stable, but this problem is definitely a
regression and no other solutions came up in response to my RFC.
The only real alternative I see for stable, is disabling MBIM support
on any dual NCM/MBIM function. Which of course will be a regression
for any user wanting MBIM, making it unacceptable.
drivers/net/usb/cdc_mbim.c | 11 +---------
drivers/net/usb/cdc_ncm.c | 49 ++++++++++++++++++++++++++++---------------
include/linux/usb/cdc_ncm.h | 1 +
3 files changed, 34 insertions(+), 27 deletions(-)
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 248d2dc..16c8429 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -68,18 +68,9 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
struct cdc_ncm_ctx *ctx;
struct usb_driver *subdriver = ERR_PTR(-ENODEV);
int ret = -ENODEV;
- u8 data_altsetting = CDC_NCM_DATA_ALTSETTING_NCM;
+ u8 data_altsetting = cdc_ncm_select_altsetting(dev, intf);
struct cdc_mbim_state *info = (void *)&dev->data;
- /* see if interface supports MBIM alternate setting */
- if (intf->num_altsetting == 2) {
- if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
- usb_set_interface(dev->udev,
- intf->cur_altsetting->desc.bInterfaceNumber,
- CDC_NCM_COMM_ALTSETTING_MBIM);
- data_altsetting = CDC_NCM_DATA_ALTSETTING_MBIM;
- }
-
/* Probably NCM, defer for cdc_ncm_bind */
if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
goto err;
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 61b74a2..4709fa3 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -55,6 +55,14 @@
#define DRIVER_VERSION "14-Mar-2012"
+#if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
+static bool prefer_mbim = true;
+#else
+static bool prefer_mbim;
+#endif
+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);
@@ -550,9 +558,12 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
}
EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
-static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
+/* Select the MBIM altsetting iff it is preferred and available,
+ * returning the number of the corresponding data interface altsetting
+ */
+u8 cdc_ncm_select_altsetting(struct usbnet *dev, struct usb_interface *intf)
{
- int ret;
+ struct usb_host_interface *alt;
/* The MBIM spec defines a NCM compatible default altsetting,
* which we may have matched:
@@ -568,23 +579,27 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
* endpoint descriptors, shall be constructed according to
* the rules given in section 6 (USB Device Model) of this
* specification."
- *
- * Do not bind to such interfaces, allowing cdc_mbim to handle
- * them
*/
-#if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
- if ((intf->num_altsetting == 2) &&
- !usb_set_interface(dev->udev,
- intf->cur_altsetting->desc.bInterfaceNumber,
- CDC_NCM_COMM_ALTSETTING_MBIM)) {
- if (cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
- return -ENODEV;
- else
- usb_set_interface(dev->udev,
- intf->cur_altsetting->desc.bInterfaceNumber,
- CDC_NCM_COMM_ALTSETTING_NCM);
+ if (prefer_mbim && intf->num_altsetting == 2) {
+ alt = usb_altnum_to_altsetting(intf, CDC_NCM_COMM_ALTSETTING_MBIM);
+ if (alt && cdc_ncm_comm_intf_is_mbim(alt) &&
+ !usb_set_interface(dev->udev,
+ intf->cur_altsetting->desc.bInterfaceNumber,
+ CDC_NCM_COMM_ALTSETTING_MBIM))
+ return CDC_NCM_DATA_ALTSETTING_MBIM;
}
-#endif
+ return CDC_NCM_DATA_ALTSETTING_NCM;
+}
+EXPORT_SYMBOL_GPL(cdc_ncm_select_altsetting);
+
+static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+ int ret;
+
+ /* MBIM backwards compatible function? */
+ cdc_ncm_select_altsetting(dev, intf);
+ if (cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
+ return -ENODEV;
/* NCM data altsetting is always 1 */
ret = cdc_ncm_bind_common(dev, intf, 1);
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 3b8f9d4..cc25b70 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -127,6 +127,7 @@ struct cdc_ncm_ctx {
u16 connected;
};
+extern u8 cdc_ncm_select_altsetting(struct usbnet *dev, struct usb_interface *intf);
extern int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting);
extern void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf);
extern struct sk_buff *cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility
[not found] ` <1363259113-6909-1-git-send-email-bjorn-yOkvZcmFvRU@public.gmane.org>
@ 2013-03-15 5:22 ` Ben Hutchings
2013-03-15 7:02 ` Bjørn Mork
0 siblings, 1 reply; 5+ messages in thread
From: Ben Hutchings @ 2013-03-15 5:22 UTC (permalink / raw)
To: Bjørn Mork
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Greg Suarez, Alexey Orishko
[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]
On Thu, 2013-03-14 at 12:05 +0100, Bjørn Mork wrote:
> commit bd329e1 ("net: cdc_ncm: do not bind to NCM compatible MBIM devices")
> introduced a new policy, preferring MBIM for dual NCM/MBIM functions if
> the cdc_mbim driver was enabled. This caused a regression for users
> wanting to use NCM.
>
> Devices implementing NCM backwards compatibility according to section
> 3.2 of the MBIM v1.0 specification allow either NCM or MBIM on a single
> USB function, using different altsettings. The cdc_ncm and cdc_mbim
> drivers will both probe such functions, and must agree on a common
> policy for selecting either MBIM or NCM. Until now, this policy has
> been set at build time based on CONFIG_USB_NET_CDC_MBIM.
>
> Use a module parameter to set the system policy at runtime, allowing the
> user to prefer NCM on systems with the cdc_mbim driver.
>
> Cc: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public•gmane.org>
> Cc: Alexey Orishko <alexey.orishko-0IS4wlFg1OjSUeElwK9/Pw@public•gmane.org>
> Reported-by: Geir Haatveit <nospam-br728iSPf5UsCylrc8G9yg@public•gmane.org>
> Reported-by: Tommi Kyntola <kynde-oSY6wnT0nbj1KXRcyAk9cg@public•gmane.org>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=54791
> Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public•gmane.org>
> ---
>
> We now have two users independently reporting this as a 3.8 regression,
> so something needs to be done. I am not sure if adding a new module
> parameter is acceptable for stable, but this problem is definitely a
> regression and no other solutions came up in response to my RFC.
>
> The only real alternative I see for stable, is disabling MBIM support
> on any dual NCM/MBIM function. Which of course will be a regression
> for any user wanting MBIM, making it unacceptable.
[...]
It definitely makes sense for this to be a run-time parameter. And the
default seems correct for custom kernels.
For a distribution kernel - at least for Debian, where we can't assume
kernel and userland are always updated together - I think the
compile-time default should be false, and the userland package
(presumably ModemManager?) can install a modprobe.conf file to override
that once it can handle MBIM. We handled KMS transitions in a similar
way. I don't know that it's worth having a Kconfig option for that,
though.
Ben.
--
Ben Hutchings
Humans are not rational beings; they are rationalising beings.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility
2013-03-15 5:22 ` Ben Hutchings
@ 2013-03-15 7:02 ` Bjørn Mork
2013-03-16 17:51 ` Ben Hutchings
0 siblings, 1 reply; 5+ messages in thread
From: Bjørn Mork @ 2013-03-15 7:02 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, linux-usb, Greg Suarez, Alexey Orishko
Ben Hutchings <ben@decadent•org.uk> writes:
> On Thu, 2013-03-14 at 12:05 +0100, Bjørn Mork wrote:
>> commit bd329e1 ("net: cdc_ncm: do not bind to NCM compatible MBIM devices")
>> introduced a new policy, preferring MBIM for dual NCM/MBIM functions if
>> the cdc_mbim driver was enabled. This caused a regression for users
>> wanting to use NCM.
>>
>> Devices implementing NCM backwards compatibility according to section
>> 3.2 of the MBIM v1.0 specification allow either NCM or MBIM on a single
>> USB function, using different altsettings. The cdc_ncm and cdc_mbim
>> drivers will both probe such functions, and must agree on a common
>> policy for selecting either MBIM or NCM. Until now, this policy has
>> been set at build time based on CONFIG_USB_NET_CDC_MBIM.
>>
>> Use a module parameter to set the system policy at runtime, allowing the
>> user to prefer NCM on systems with the cdc_mbim driver.
>>
>> Cc: Greg Suarez <gsuarez@smithmicro•com>
>> Cc: Alexey Orishko <alexey.orishko@stericsson•com>
>> Reported-by: Geir Haatveit <nospam@haatveit•nu>
>> Reported-by: Tommi Kyntola <kynde@ts•ray.fi>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=54791
>> Signed-off-by: Bjørn Mork <bjorn@mork•no>
>> ---
>>
>> We now have two users independently reporting this as a 3.8 regression,
>> so something needs to be done. I am not sure if adding a new module
>> parameter is acceptable for stable, but this problem is definitely a
>> regression and no other solutions came up in response to my RFC.
>>
>> The only real alternative I see for stable, is disabling MBIM support
>> on any dual NCM/MBIM function. Which of course will be a regression
>> for any user wanting MBIM, making it unacceptable.
> [...]
>
> It definitely makes sense for this to be a run-time parameter. And the
> default seems correct for custom kernels.
>
> For a distribution kernel - at least for Debian, where we can't assume
> kernel and userland are always updated together - I think the
> compile-time default should be false, and the userland package
> (presumably ModemManager?) can install a modprobe.conf file to override
> that once it can handle MBIM. We handled KMS transitions in a similar
> way. I don't know that it's worth having a Kconfig option for that,
> though.
We could consider always defaulting to NCM. That would remove the
ugliest parts of the code as well. Should I send a new version with
such a change?
Bundling a modprobe.conf file to enable MBIM with userland tools
supporting it is a great idea, and sounds feasible for custom built
systems as well.
(BTW, if there is any doubt about it, I feel really bad about my recent
request to enable MBIM in Debian, where I wrote "enabling the driver
will not prevent any existing solution from working.". At the time, I
had not seen any of these dual NCM/MBIM functions and I just didn't
think about the problems such devices would run into. Sorry about
that. I guess I owe you one more...)
Bjørn
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility
2013-03-15 7:02 ` Bjørn Mork
@ 2013-03-16 17:51 ` Ben Hutchings
0 siblings, 0 replies; 5+ messages in thread
From: Ben Hutchings @ 2013-03-16 17:51 UTC (permalink / raw)
To: Bjørn Mork; +Cc: netdev, linux-usb, Greg Suarez, Alexey Orishko
[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]
On Fri, 2013-03-15 at 08:02 +0100, Bjørn Mork wrote:
> Ben Hutchings <ben@decadent•org.uk> writes:
>
> > On Thu, 2013-03-14 at 12:05 +0100, Bjørn Mork wrote:
> >> commit bd329e1 ("net: cdc_ncm: do not bind to NCM compatible MBIM devices")
> >> introduced a new policy, preferring MBIM for dual NCM/MBIM functions if
> >> the cdc_mbim driver was enabled. This caused a regression for users
> >> wanting to use NCM.
> >>
> >> Devices implementing NCM backwards compatibility according to section
> >> 3.2 of the MBIM v1.0 specification allow either NCM or MBIM on a single
> >> USB function, using different altsettings. The cdc_ncm and cdc_mbim
> >> drivers will both probe such functions, and must agree on a common
> >> policy for selecting either MBIM or NCM. Until now, this policy has
> >> been set at build time based on CONFIG_USB_NET_CDC_MBIM.
> >>
> >> Use a module parameter to set the system policy at runtime, allowing the
> >> user to prefer NCM on systems with the cdc_mbim driver.
> >>
> >> Cc: Greg Suarez <gsuarez@smithmicro•com>
> >> Cc: Alexey Orishko <alexey.orishko@stericsson•com>
> >> Reported-by: Geir Haatveit <nospam@haatveit•nu>
> >> Reported-by: Tommi Kyntola <kynde@ts•ray.fi>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=54791
> >> Signed-off-by: Bjørn Mork <bjorn@mork•no>
> >> ---
> >>
> >> We now have two users independently reporting this as a 3.8 regression,
> >> so something needs to be done. I am not sure if adding a new module
> >> parameter is acceptable for stable, but this problem is definitely a
> >> regression and no other solutions came up in response to my RFC.
> >>
> >> The only real alternative I see for stable, is disabling MBIM support
> >> on any dual NCM/MBIM function. Which of course will be a regression
> >> for any user wanting MBIM, making it unacceptable.
> > [...]
> >
> > It definitely makes sense for this to be a run-time parameter. And the
> > default seems correct for custom kernels.
> >
> > For a distribution kernel - at least for Debian, where we can't assume
> > kernel and userland are always updated together - I think the
> > compile-time default should be false, and the userland package
> > (presumably ModemManager?) can install a modprobe.conf file to override
> > that once it can handle MBIM. We handled KMS transitions in a similar
> > way. I don't know that it's worth having a Kconfig option for that,
> > though.
>
> We could consider always defaulting to NCM. That would remove the
> ugliest parts of the code as well. Should I send a new version with
> such a change?
We could, but I fear that would annoy people building custom kernels.
> Bundling a modprobe.conf file to enable MBIM with userland tools
> supporting it is a great idea, and sounds feasible for custom built
> systems as well.
Using unpatched Linux & ModemManager (or other userland) needs to just
work. So if Linux is going to have MBIM disabled by default then
upstream ModemManager needs to ship and install that modprobe.conf file.
> (BTW, if there is any doubt about it, I feel really bad about my recent
> request to enable MBIM in Debian, where I wrote "enabling the driver
> will not prevent any existing solution from working.". At the time, I
> had not seen any of these dual NCM/MBIM functions and I just didn't
> think about the problems such devices would run into. Sorry about
> that. I guess I owe you one more...)
It's called experimental for a reason. :-)
Ben.
--
Ben Hutchings
It is easier to change the specification to fit the program than vice versa.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility
2013-03-14 11:05 [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility Bjørn Mork
[not found] ` <1363259113-6909-1-git-send-email-bjorn-yOkvZcmFvRU@public.gmane.org>
@ 2013-03-17 16:00 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2013-03-17 16:00 UTC (permalink / raw)
To: bjorn; +Cc: netdev, linux-usb, gsuarez, alexey.orishko
From: Bjørn Mork <bjorn@mork•no>
Date: Thu, 14 Mar 2013 12:05:13 +0100
> commit bd329e1 ("net: cdc_ncm: do not bind to NCM compatible MBIM devices")
> introduced a new policy, preferring MBIM for dual NCM/MBIM functions if
> the cdc_mbim driver was enabled. This caused a regression for users
> wanting to use NCM.
>
> Devices implementing NCM backwards compatibility according to section
> 3.2 of the MBIM v1.0 specification allow either NCM or MBIM on a single
> USB function, using different altsettings. The cdc_ncm and cdc_mbim
> drivers will both probe such functions, and must agree on a common
> policy for selecting either MBIM or NCM. Until now, this policy has
> been set at build time based on CONFIG_USB_NET_CDC_MBIM.
>
> Use a module parameter to set the system policy at runtime, allowing the
> user to prefer NCM on systems with the cdc_mbim driver.
>
> Cc: Greg Suarez <gsuarez@smithmicro•com>
> Cc: Alexey Orishko <alexey.orishko@stericsson•com>
> Reported-by: Geir Haatveit <nospam@haatveit•nu>
> Reported-by: Tommi Kyntola <kynde@ts•ray.fi>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=54791
> Signed-off-by: Bjørn Mork <bjorn@mork•no>
Ok, this is fine as a solution for now, applied and queued up for -stable.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-17 16:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-14 11:05 [PATCH net,stable-3.8] net: cdc_ncm, cdc_mbim: allow user to prefer NCM for backwards compatibility Bjørn Mork
[not found] ` <1363259113-6909-1-git-send-email-bjorn-yOkvZcmFvRU@public.gmane.org>
2013-03-15 5:22 ` Ben Hutchings
2013-03-15 7:02 ` Bjørn Mork
2013-03-16 17:51 ` Ben Hutchings
2013-03-17 16:00 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox