[PATCH 0/5] Delay selecting alternate setting in CDC NCM until network interface is raised

[PATCH 0/5] Delay selecting alternate setting in CDC NCM until network interface is raised

[Toby Gray]

This patch series moves the setting of the alternate setting used
for the CDC NCM data interface until the network interface is
raised. Before this patch series the CDC NCM driver would select
the alternate setting which allows data to be sent and received
as soon as the device was probed.

While the previous behaviour was not against the CDC NCM
specification it is problematic with some CDC NCM devices,
e.g. Nokia 701 Mobile Telephone. These devices will only send the
network connection status notification to the host if the control
interrupt endpoint is read from within a few seconds of the data
enabled alternate setting being selected.

This behaviour means that the network connection notification
would never be received and so the network interface would stay
in a disconnected state indefinitely, unless the interface was
marked as 'up' almost immediately after the CDC NCM device was
probed.

Toby Gray (5):
  usb: cdc-ncm: Change alternate setting magic numbers into #defines
  usb: cdc-ncm: Set altsetting only when network interface is opened
  usb: usbnet: Allow drivers using usbnet to specify maximum packet
    size
  usb: usbnet: Add validation of dev->maxpacket to usbnet
  usb: cdc-ncm: Allow NCM driver to determine dev->maxpacket

 drivers/net/usb/cdc_ncm.c |   46 ++++++++++++++++++++++++++++++++++++--------
 drivers/net/usb/usbnet.c  |    7 +++++-
 2 files changed, 43 insertions(+), 10 deletions(-)

[PATCH 1/5] usb: cdc-ncm: Change alternate setting magic numbers into #defines

[Toby Gray]

The CDC NCM specification describes the alternate settings that a CDC
NCM interface must support. Alternate setting 0 does not allow any
network traffic but allows configuration of the interface. Alternate 1
allows network traffic but doesn't allow configuration of some of the
interface properties.

This change provides #defines for there two numbers to clarify why
usb_set_interface is being called.

Signed-off-by: Toby Gray <toby.gray@realvnc.com>
---
 drivers/net/usb/cdc_ncm.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 3a539a9..3df51ee 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -87,6 +87,13 @@
 	(sizeof(struct usb_cdc_ncm_nth16) + sizeof(struct usb_cdc_ncm_ndp16) + \
 	(CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
 
+/* CDC NCM ch. 5.3 describes alternate setting 0 as having no
+ * endpoints and therefore not allowing any networking traffic. */
+#define	CDC_NCM_ALTSETTING_RESET	0
+/* CDC NCM ch. 5.3 describes alternate setting 1 as having the
+ * required bulk endpoints for normal operation. */
+#define	CDC_NCM_ALTSETTING_DATA		1
+
 struct cdc_ncm_data {
 	struct usb_cdc_ncm_nth16 nth16;
 	struct usb_cdc_ncm_ndp16 ndp16;
@@ -549,7 +556,7 @@ advance:
 	iface_no = ctx->data->cur_altsetting->desc.bInterfaceNumber;
 
 	/* reset data interface */
-	temp = usb_set_interface(dev->udev, iface_no, 0);
+	temp = usb_set_interface(dev->udev, iface_no, CDC_NCM_ALTSETTING_RESET);
 	if (temp)
 		goto error2;
 
@@ -558,7 +565,7 @@ advance:
 		goto error2;
 
 	/* configure data interface */
-	temp = usb_set_interface(dev->udev, iface_no, 1);
+	temp = usb_set_interface(dev->udev, iface_no, CDC_NCM_ALTSETTING_DATA);
 	if (temp)
 		goto error2;
 
-- 
1.7.0.4

Re: [PATCH 1/5] usb: cdc-ncm: Change alternate setting magic numbers into #defines

[Alexey Orishko]

On Wed, Feb 15, 2012 at 3:47 PM, Toby Gray <toby.gray-BiNz9QiKYoNBDgjK7y7TUQ@public.gmane.org> wrote:

> +/* CDC NCM ch. 5.3 describes alternate setting 0 as having no
> + * endpoints and therefore not allowing any networking traffic. */
> +#define        CDC_NCM_ALTSETTING_RESET        0

Please, use bulk in the both define names to distinguish from control
ep alt settings in the next version of the specification, i.e.
something like CDC_NCM_BULK_ALTSETTING_NO_DATA
This is the setting with no endpoints (no data).

> +/* CDC NCM ch. 5.3 describes alternate setting 1 as having the
> + * required bulk endpoints for normal operation. */
> +#define        CDC_NCM_ALTSETTING_DATA         1

Regards,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] usb: cdc-ncm: Set altsetting only when network interface is opened

[Toby Gray]

CDC NCM devices have two alternate settings for the data interface,
one without any endpoints and one with endpoints. Selecting the
alternate setting with endpoints is used to signal to the function
that the host is interested in the network connectivity and has
finished setting NCM operational parameters.

Some NCM devices fail to send the NetworkConnection status if the host
is not trying to read from the control interrupt endpoint in the first
few seconds after the data interface alternate setting is selected.

This change moves the selection of the data interface alternate
setting to when the network interface is opened.

Signed-off-by: Toby Gray <toby.gray-BiNz9QiKYoNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c |   36 ++++++++++++++++++++++++++++--------
 1 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 3df51ee..e1b2d5d 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -411,14 +411,14 @@ max_dgram_err:
 }
 
 static void
-cdc_ncm_find_endpoints(struct cdc_ncm_ctx *ctx, struct usb_interface *intf)
+cdc_ncm_find_endpoints(struct cdc_ncm_ctx *ctx, struct usb_host_interface *intf)
 {
 	struct usb_host_endpoint *e;
 	u8 ep;
 
-	for (ep = 0; ep < intf->cur_altsetting->desc.bNumEndpoints; ep++) {
+	for (ep = 0; ep < intf->desc.bNumEndpoints; ep++) {
 
-		e = intf->cur_altsetting->endpoint + ep;
+		e = intf->endpoint + ep;
 		switch (e->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
 		case USB_ENDPOINT_XFER_INT:
 			if (usb_endpoint_dir_in(&e->desc)) {
@@ -467,6 +467,7 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct cdc_ncm_ctx *ctx;
 	struct usb_driver *driver;
+	struct usb_host_interface *data_altsetting;
 	u8 *buf;
 	int len;
 	int temp;
@@ -564,13 +565,14 @@ advance:
 	if (cdc_ncm_setup(ctx))
 		goto error2;
 
-	/* configure data interface */
-	temp = usb_set_interface(dev->udev, iface_no, CDC_NCM_ALTSETTING_DATA);
-	if (temp)
+	/* find the data interface altsetting */
+	data_altsetting =
+		usb_altnum_to_altsetting(ctx->data, CDC_NCM_ALTSETTING_DATA);
+	if (data_altsetting == NULL)
 		goto error2;
 
-	cdc_ncm_find_endpoints(ctx, ctx->data);
-	cdc_ncm_find_endpoints(ctx, ctx->control);
+	cdc_ncm_find_endpoints(ctx, data_altsetting);
+	cdc_ncm_find_endpoints(ctx, ctx->control->cur_altsetting);
 
 	if ((ctx->in_ep == NULL) || (ctx->out_ep == NULL) ||
 	    (ctx->status_ep == NULL))
@@ -1082,6 +1084,23 @@ error:
 	return 0;
 }
 
+static int cdc_ncm_reset(struct usbnet *dev)
+{
+	struct cdc_ncm_ctx *ctx;
+	int temp;
+	u8 iface_no;
+
+	ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	iface_no = ctx->data->cur_altsetting->desc.bInterfaceNumber;
+
+	/* configure data interface */
+	temp = usb_set_interface(dev->udev, iface_no, CDC_NCM_ALTSETTING_DATA);
+	if (temp)
+		return temp;
+
+	return 0;
+}
+
 static void
 cdc_ncm_speed_change(struct cdc_ncm_ctx *ctx,
 		     struct usb_cdc_speed_change *data)
@@ -1214,6 +1233,7 @@ static const struct driver_info cdc_ncm_info = {
 	.status = cdc_ncm_status,
 	.rx_fixup = cdc_ncm_rx_fixup,
 	.tx_fixup = cdc_ncm_tx_fixup,
+	.reset = cdc_ncm_reset,
 };
 
 static struct usb_driver cdc_ncm_driver = {
-- 
1.7.0.4

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

Re: [PATCH 2/5] usb: cdc-ncm: Set altsetting only when network interface is opened

[Alexey Orishko]

On Wed, Feb 15, 2012 at 3:47 PM, Toby Gray <toby.gray-BiNz9QiKYoNBDgjK7y7TUQ@public.gmane.org> wrote:

> Some NCM devices fail to send the NetworkConnection status if the host
> is not trying to read from the control interrupt endpoint in the first
> few seconds after the data interface alternate setting is selected.

I have a feeling that the problem description above is not correct:
more likely the fault is related to device initialization or other
state machine problem in device.

Looking at the trace, the patch serves its purpose, however there
might be other devices which can't get network status in time or get
similar problem. If this would be the case, driver could set a timer
after selecting alt1; in the absence of the connect message set alt0
to reset a function when timer expires and set alt1 again.

Regards,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] usb: cdc-ncm: Set altsetting only when network interface is opened

[Toby Gray]

On 17/02/12 09:57, Alexey Orishko wrote:
> On Wed, Feb 15, 2012 at 3:47 PM, Toby Gray<toby.gray-BiNz9QiKYoNBDgjK7y7TUQ@public.gmane.org>  wrote:
>
>> Some NCM devices fail to send the NetworkConnection status if the host
>> is not trying to read from the control interrupt endpoint in the first
>> few seconds after the data interface alternate setting is selected.
> I have a feeling that the problem description above is not correct:
> more likely the fault is related to device initialization or other
> state machine problem in device.

I think I've been explaining my understanding badly and we actually 
agree :) I imagine that the device has code something like the following 
in it's initialization code:

void handle_set_alternate_setting(int altsetting) {
     if (altsetting == 0) {
         reset_interface_configuration();
     } else if (altsetting == 1) {
         queue_networkconnection_notification(DISCONNECTED, 1000MS_TIMEOUT);
         //ignore timeout in sending notification
         queue_speed_configuration_notification(tx_speed, rx_speed, 
1000MS_TIMEOUT);
         //ignore timeout in sending speed configuration
         queue_networkconnection_notification(CONNECTED, 1000MS_TIMEOUT);
         // ignore timeout in sending notification
     }
}

Is that the sort of behavior you were thinking of?

> Looking at the trace, the patch serves its purpose, however there
> might be other devices which can't get network status in time or get
> similar problem. If this would be the case, driver could set a timer
> after selecting alt1; in the absence of the connect message set alt0
> to reset a function when timer expires and set alt1 again.

I might be overly cautious but this behavior sounds like it could impact 
reliability. All my patch series does is keep the device in the reset 
alternate setting until the network interface comes up. My understanding 
is that this won't impact CDC NCM devices which queue the network 
notifications indefinitely and it solves the issue for the device I'm 
having issues with.

My reading of the CDC NCM specification is that it doesn't give any 
timing constraints on when a NetworkConnection notification can come 
from the device. You mentioned that 3G modems can take 1-2 minutes, so 
if some sort of timer was added after selecting alt1, what value could 
be used for it which would work reliably with all NCM CDC devices? Also 
what devices could we test this timer code on?

Regards,

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

[PATCH 3/5] usb: usbnet: Allow drivers using usbnet to specify maximum packet size

[Toby Gray]

The usbnet driver always attempts to set dev->maxpacket from the out
endpoint. For this to function correctly it relies on the alternate
setting containg the bulk out endpoint to be selected. This isn't
necessarily always the case.

This change allows drivers that make use of usbnet to specify the
value for dev->maxpacket themselves.

Signed-off-by: Toby Gray <toby.gray@realvnc.com>
---
 drivers/net/usb/usbnet.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index fae0fbd..4ccd316 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1425,7 +1425,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	if (!dev->rx_urb_size)
 		dev->rx_urb_size = dev->hard_mtu;
-	dev->maxpacket = usb_maxpacket (dev->udev, dev->out, 1);
+	if (!dev->maxpacket)
+		dev->maxpacket = usb_maxpacket(dev->udev, dev->out, 1);
 
 	if ((dev->driver_info->flags & FLAG_WLAN) != 0)
 		SET_NETDEV_DEVTYPE(net, &wlan_type);
-- 
1.7.0.4

Re: [PATCH 3/5] usb: usbnet: Allow drivers using usbnet to specify maximum packet size

[Oliver Neukum]

Am Mittwoch, 15. Februar 2012, 15:47:39 schrieb Toby Gray:
> The usbnet driver always attempts to set dev->maxpacket from the out
> endpoint. For this to function correctly it relies on the alternate
> setting containg the bulk out endpoint to be selected. This isn't
> necessarily always the case.
> 
> This change allows drivers that make use of usbnet to specify the
> value for dev->maxpacket themselves.
> 
> Signed-off-by: Toby Gray <toby.gray-BiNz9QiKYoNBDgjK7y7TUQ@public.gmane.org>
Acked-by: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] usb: usbnet: Add validation of dev->maxpacket to usbnet

[Toby Gray]

Several parts of usbnet rely on dev->maxpacket not being set to 0 to
prevent division by zero errors.

This adds validation of the dev->maxpacket value being non-zero before
treating the device probe as successful.

Signed-off-by: Toby Gray <toby.gray@realvnc.com>
---
 drivers/net/usb/usbnet.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 4ccd316..1491c90 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1427,6 +1427,10 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 		dev->rx_urb_size = dev->hard_mtu;
 	if (!dev->maxpacket)
 		dev->maxpacket = usb_maxpacket(dev->udev, dev->out, 1);
+	if (!dev->maxpacket) {
+		status = -ENODEV;
+		goto out3;
+	}
 
 	if ((dev->driver_info->flags & FLAG_WLAN) != 0)
 		SET_NETDEV_DEVTYPE(net, &wlan_type);
-- 
1.7.0.4

Re: [PATCH 4/5] usb: usbnet: Add validation of dev->maxpacket to usbnet

[Oliver Neukum]

Am Mittwoch, 15. Februar 2012, 15:47:40 schrieb Toby Gray:
> Several parts of usbnet rely on dev->maxpacket not being set to 0 to
> prevent division by zero errors.
> 
> This adds validation of the dev->maxpacket value being non-zero before
> treating the device probe as successful.
> 
> Signed-off-by: Toby Gray <toby.gray@realvnc.com>
> ---
>  drivers/net/usb/usbnet.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 4ccd316..1491c90 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1427,6 +1427,10 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  		dev->rx_urb_size = dev->hard_mtu;
>  	if (!dev->maxpacket)
>  		dev->maxpacket = usb_maxpacket(dev->udev, dev->out, 1);
> +	if (!dev->maxpacket) {
> +		status = -ENODEV;
> +		goto out3;

Hm. I am sceptical. If this happens a subdriver is buggy. We should
not hide that. I am afraid I have to reject this patch.

	Regards
		Oliver

Re: [PATCH 4/5] usb: usbnet: Add validation of dev->maxpacket to usbnet

[Toby Gray]

On 15/02/12 19:34, Oliver Neukum wrote:
> Am Mittwoch, 15. Februar 2012, 15:47:40 schrieb Toby Gray:
>> Several parts of usbnet rely on dev->maxpacket not being set to 0 to
>> prevent division by zero errors.
>>
>> This adds validation of the dev->maxpacket value being non-zero before
>> treating the device probe as successful.
>>
>> Signed-off-by: Toby Gray<toby.gray-BiNz9QiKYoNBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/net/usb/usbnet.c |    4 ++++
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 4ccd316..1491c90 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -1427,6 +1427,10 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>>   		dev->rx_urb_size = dev->hard_mtu;
>>   	if (!dev->maxpacket)
>>   		dev->maxpacket = usb_maxpacket(dev->udev, dev->out, 1);
>> +	if (!dev->maxpacket) {
>> +		status = -ENODEV;
>> +		goto out3;
> Hm. I am sceptical. If this happens a subdriver is buggy. We should
> not hide that. I am afraid I have to reject this patch.

That's understandable, I almost didn't include it in the series. The 
only reason I added this to the patch series was because I spent a while 
trying to track down a division by zero, when it turns out it was 
actually due to dev->maxpacket being zero when usbnet_start_xmit tried 
to calculate length % dev->maxpacket.

Would you prefer that I drop this patch entirely or just change it to 
something like a BUG_ON?

Regards,

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

[PATCH 5/5] usb: cdc-ncm: Allow NCM driver to determine dev->maxpacket

[Toby Gray]

As the NCM driver does not necessarily select the alternate setting
which presents the data bulk endpoints, dev->maxpacket can't be set
correctly by usbnet.

This fixes this by getting the NCM driver to set the dev->maxpacket itself.

Signed-off-by: Toby Gray <toby.gray@realvnc.com>
---
 drivers/net/usb/cdc_ncm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e1b2d5d..158c3d8 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -600,6 +600,7 @@ advance:
 		ctx->out_ep->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
 	dev->status = ctx->status_ep;
 	dev->rx_urb_size = ctx->rx_max;
+	dev->maxpacket = usb_endpoint_maxp(&ctx->out_ep->desc);
 
 	/*
 	 * We should get an event when network connection is "connected" or
-- 
1.7.0.4