| Message ID | 1329317261-3406-3-git-send-email-toby.gray@realvnc.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Feb 15, 2012 at 3:47 PM, Toby Gray <toby.gray@realvnc.com> 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/02/12 09:57, Alexey Orishko wrote: > On Wed, Feb 15, 2012 at 3:47 PM, Toby Gray<toby.gray@realvnc.com> 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = {
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@realvnc.com> --- drivers/net/usb/cdc_ncm.c | 36 ++++++++++++++++++++++++++++-------- 1 files changed, 28 insertions(+), 8 deletions(-)