[RFC] Revert "sierra_net: keep status interrupt URB active"

[RFC] Revert "sierra_net: keep status interrupt URB active"

[Bjørn Mork]

This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.

It's not easy to create a driver for all the various firmware
bugs out there.

This change caused regressions for a number of devices, which
started to fail link detection and therefore became completely
non-functional. The exact reason is yet unknown, it looks like
the affected firmwares might actually need all or some of the
additional SYNC messages the patch got rid of.

Reverting is not optimal, as it will re-introduce the original
problem, but it is currently the only alternative known to fix
this issue.

Cc: Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=998342
Reported-by: John Henderson <jhen-qw6QB7/foO7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
Hello Dan,

I don't know if you've noticed the bug report referenced above
and the discussions taking place down under.  E.g.:
http://forums.whirlpool.net.au/forum-replies.cfm?t=2135188&p=15#r300

It seems that a number of devices just won't work at all with
the fix for the SYNC problem.  Unfortunately, the only DirectIP
device I've got (MC7710) doesn't have any of these firmware problems,
so I have been unable to come up with any better solution than
reverting.  Which has been confirmed to fix the problem.

My only real alternative proposal, delaying the first SYNC message
to give the firmware some slack, did not work:
http://forums.whirlpool.net.au/forum-replies.cfm?t=2135188&p=16#r312

So unless you or someone else has a better idea, I don't really
see any other option than reverting the patch.  I am posting
this as an RFC initially as I believe we need to discuss the
options.


Bjørn

 drivers/net/usb/sierra_net.c |   38 +++++++-------------------------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index a79e9d3..a923d61 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -426,13 +426,6 @@ static void sierra_net_dosync(struct usbnet *dev)
 
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 
-	/* The SIERRA_NET_HIP_MSYNC_ID command appears to request that the
-	 * firmware restart itself.  After restarting, the modem will respond
-	 * with the SIERRA_NET_HIP_RESTART_ID indication.  The driver continues
-	 * sending MSYNC commands every few seconds until it receives the
-	 * RESTART event from the firmware
-	 */
-
 	/* tell modem we are ready */
 	status = sierra_net_send_sync(dev);
 	if (status < 0)
@@ -711,9 +704,6 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* set context index initially to 0 - prepares tx hdr template */
 	sierra_net_set_ctx_index(priv, 0);
 
-	/* prepare sync message template */
-	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
-
 	/* decrease the rx_urb_size and max_tx_size to 4k on USB 1.1 */
 	dev->rx_urb_size  = SIERRA_NET_RX_URB_SIZE;
 	if (dev->udev->speed != USB_SPEED_HIGH)
@@ -749,6 +739,11 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 		kfree(priv);
 		return -ENODEV;
 	}
+	/* prepare sync message from template */
+	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
+
+	/* initiate the sync sequence */
+	sierra_net_dosync(dev);
 
 	return 0;
 }
@@ -771,9 +766,8 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 		netdev_err(dev->net,
 			"usb_control_msg failed, status %d\n", status);
 
-	usbnet_status_stop(dev);
-
 	sierra_net_set_private(dev, NULL);
+
 	kfree(priv);
 }
 
@@ -914,24 +908,6 @@ static const struct driver_info sierra_net_info_direct_ip = {
 	.tx_fixup = sierra_net_tx_fixup,
 };
 
-static int
-sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
-{
-	int ret;
-
-	ret = usbnet_probe(udev, prod);
-	if (ret == 0) {
-		struct usbnet *dev = usb_get_intfdata(udev);
-
-		ret = usbnet_status_start(dev, GFP_KERNEL);
-		if (ret == 0) {
-			/* Interrupt URB now set up; initiate sync sequence */
-			sierra_net_dosync(dev);
-		}
-	}
-	return ret;
-}

Re: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Dan Williams]

On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> 
> It's not easy to create a driver for all the various firmware
> bugs out there.
> 
> This change caused regressions for a number of devices, which
> started to fail link detection and therefore became completely
> non-functional. The exact reason is yet unknown, it looks like
> the affected firmwares might actually need all or some of the
> additional SYNC messages the patch got rid of.
> 
> Reverting is not optimal, as it will re-introduce the original
> problem, but it is currently the only alternative known to fix
> this issue.

Instead, how does the following patch work for you?

Dan

---
diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index a79e9d3..dd59d97 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -163,18 +163,19 @@ struct lsi_umts {
 #define SIERRA_NET_LSI_UMTS_LEN        (sizeof(struct lsi_umts))
 #define SIERRA_NET_LSI_UMTS_STATUS_LEN \
 	(SIERRA_NET_LSI_UMTS_LEN - SIERRA_NET_LSI_COMMON_LEN)
 
 /* Forward definitions */
 static void sierra_sync_timer(unsigned long syncdata);
 static int sierra_net_change_mtu(struct net_device *net, int new_mtu);
+static int sierra_net_open (struct net_device *net);
 
 /* Our own net device operations structure */
 static const struct net_device_ops sierra_net_device_ops = {
-	.ndo_open               = usbnet_open,
+	.ndo_open               = sierra_net_open,
 	.ndo_stop               = usbnet_stop,
 	.ndo_start_xmit         = usbnet_start_xmit,
 	.ndo_tx_timeout         = usbnet_tx_timeout,
 	.ndo_change_mtu         = sierra_net_change_mtu,
 	.ndo_set_mac_address    = eth_mac_addr,
 	.ndo_validate_addr      = eth_validate_addr,
 };
@@ -439,14 +440,15 @@ static void sierra_net_dosync(struct usbnet *dev)
 		netdev_err(dev->net,
 			"Send SYNC failed, status %d\n", status);
 	status = sierra_net_send_sync(dev);
 	if (status < 0)
 		netdev_err(dev->net,
 			"Send SYNC failed, status %d\n", status);
 
+printk(KERN_INFO "%s: sent two SYNC messages\n", __func__);
 	/* Now, start a timer and make sure we get the Restart Indication */
 	priv->sync_timer.function = sierra_sync_timer;
 	priv->sync_timer.data = (unsigned long) dev;
 	priv->sync_timer.expires = jiffies + SIERRA_NET_SYNCDELAY;
 	add_timer(&priv->sync_timer);
 }
 
@@ -497,31 +499,34 @@ static void sierra_net_kevent(struct work_struct *work)
 				netdev_err(dev->net, "%s: Bad packet, received"
 					" %d, expected %d\n",	__func__, len,
 					hh.hdrlen + hh.payload_len.word);
 				kfree(buf);
 				return;
 			}
 
+printk(KERN_INFO "%s: received msg 0x%02x len %d\n", __func__, hh.msgid.byte, len);
 			/* Switch on received message types */
 			switch (hh.msgid.byte) {
 			case SIERRA_NET_HIP_LSI_UMTSID:
 				dev_dbg(&dev->udev->dev, "LSI for ctx:%d",
 					hh.msgspecific.byte);
 				sierra_net_handle_lsi(dev, buf, &hh);
 				break;
 			case SIERRA_NET_HIP_RESTART_ID:
+printk(KERN_INFO "%s: RESTART received code 0x%02x\n", __func__, hh.msgspecific.byte);
 				dev_dbg(&dev->udev->dev, "Restart reported: %d,"
 						" stopping sync timer",
 						hh.msgspecific.byte);
 				/* Got sync resp - stop timer & clear mask */
 				del_timer_sync(&priv->sync_timer);
 				clear_bit(SIERRA_NET_TIMER_EXPIRY,
 					  &priv->kevent_flags);
 				break;
 			case SIERRA_NET_HIP_HSYNC_ID:
+printk(KERN_INFO "%s: HSYNC received\n", __func__);
 				dev_dbg(&dev->udev->dev, "SYNC received");
 				err = sierra_net_send_sync(dev);
 				if (err < 0)
 					netdev_err(dev->net,
 						"Send SYNC failed %d\n", err);
 				break;
 			case SIERRA_NET_HIP_EXTENDEDID:
@@ -537,14 +542,15 @@ static void sierra_net_kevent(struct work_struct *work)
 				break;
 			}
 		}
 		kfree(buf);
 	}
 	/* The sync timer bit might be set */
 	if (test_bit(SIERRA_NET_TIMER_EXPIRY, &priv->kevent_flags)) {
+printk(KERN_INFO "%s: re-sending SYNC\n", __func__);
 		clear_bit(SIERRA_NET_TIMER_EXPIRY, &priv->kevent_flags);
 		dev_dbg(&dev->udev->dev, "Deferred sync timer expiry");
 		sierra_net_dosync(priv->usbnet);
 	}
 
 	if (priv->kevent_flags)
 		dev_dbg(&dev->udev->dev, "sierra_net_kevent done, "
@@ -562,14 +568,15 @@ static void sierra_net_defer_kevent(struct usbnet *dev, int work)
 /*
  * Sync Retransmit Timer Handler. On expiry, kick the work queue
  */
 static void sierra_sync_timer(unsigned long syncdata)
 {
 	struct usbnet *dev = (struct usbnet *)syncdata;
 
+printk(KERN_INFO "%s: sync timer expired\n", __func__);
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 	/* Kick the tasklet */
 	sierra_net_defer_kevent(dev, SIERRA_NET_TIMER_EXPIRY);
 }
 
 static void sierra_net_status(struct usbnet *dev, struct urb *urb)
 {
@@ -584,14 +591,15 @@ static void sierra_net_status(struct usbnet *dev, struct urb *urb)
 	event = urb->transfer_buffer;
 	switch (event->bNotificationType) {
 	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
 	case USB_CDC_NOTIFY_SPEED_CHANGE:
 		/* USB 305 sends those */
 		break;
 	case USB_CDC_NOTIFY_RESPONSE_AVAILABLE:
+printk(KERN_INFO "%s: firmware indicates response available\n", __func__);
 		sierra_net_defer_kevent(dev, SIERRA_NET_EVENT_RESP_AVAIL);
 		break;
 	default:
 		netdev_err(dev->net, ": unexpected notification %02x!\n",
 				event->bNotificationType);
 		break;
 	}
@@ -767,20 +775,32 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 	/* tell modem we are going away */
 	status = sierra_net_send_cmd(dev, priv->shdwn_msg,
 			sizeof(priv->shdwn_msg), "Shutdown");
 	if (status < 0)
 		netdev_err(dev->net,
 			"usb_control_msg failed, status %d\n", status);
 
-	usbnet_status_stop(dev);
-
 	sierra_net_set_private(dev, NULL);
 	kfree(priv);
 }
 
+static int sierra_net_open (struct net_device *net)
+{
+	int ret;
+
+	ret = usbnet_open(net);
+	if (ret == 0) {
+		struct usbnet *dev = netdev_priv(net);
+
+		/* Interrupt URB now set up; initiate sync sequence */
+		sierra_net_dosync(dev);
+	}
+	return ret;
+}
+
 static struct sk_buff *sierra_net_skb_clone(struct usbnet *dev,
 		struct sk_buff *skb, int len)
 {
 	struct sk_buff *new_skb;
 
 	/* clone skb */
 	new_skb = skb_clone(skb, GFP_ATOMIC);
@@ -910,14 +930,15 @@ static const struct driver_info sierra_net_info_direct_ip = {
 	.bind = sierra_net_bind,
 	.unbind = sierra_net_unbind,
 	.status = sierra_net_status,
 	.rx_fixup = sierra_net_rx_fixup,
 	.tx_fixup = sierra_net_tx_fixup,
 };
 
+#if 0
 static int
 sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
 {
 	int ret;
 
 	ret = usbnet_probe(udev, prod);
 	if (ret == 0) {
@@ -927,14 +948,15 @@ sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
 		if (ret == 0) {
 			/* Interrupt URB now set up; initiate sync sequence */
 			sierra_net_dosync(dev);
 		}
 	}
 	return ret;
 }
+#endif
 
 #define DIRECT_IP_DEVICE(vend, prod) \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 10), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 11), \
@@ -950,15 +972,15 @@ static const struct usb_device_id products[] = {
 };
 MODULE_DEVICE_TABLE(usb, products);
 
 /* We are based on usbnet, so let it handle the USB driver specifics */
 static struct usb_driver sierra_net_driver = {
 	.name = "sierra_net",
 	.id_table = products,
-	.probe = sierra_net_probe,
+	.probe = usbnet_probe,
 	.disconnect = usbnet_disconnect,
 	.suspend = usbnet_suspend,
 	.resume = usbnet_resume,
 	.no_dynamic_id = 1,
 	.disable_hub_initiated_lpm = 1,
 };
 

Re: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Dan Williams]

On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> > 
> > It's not easy to create a driver for all the various firmware
> > bugs out there.
> > 
> > This change caused regressions for a number of devices, which
> > started to fail link detection and therefore became completely
> > non-functional. The exact reason is yet unknown, it looks like
> > the affected firmwares might actually need all or some of the
> > additional SYNC messages the patch got rid of.
> > 
> > Reverting is not optimal, as it will re-introduce the original
> > problem, but it is currently the only alternative known to fix
> > this issue.
> 
> Instead, how does the following patch work for you?

Bjorn, did you have a chance to try this patch out on your devices?

Dan

> Dan
> 
> ---
> diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
> index a79e9d3..dd59d97 100644
> --- a/drivers/net/usb/sierra_net.c
> +++ b/drivers/net/usb/sierra_net.c
> @@ -163,18 +163,19 @@ struct lsi_umts {
>  #define SIERRA_NET_LSI_UMTS_LEN        (sizeof(struct lsi_umts))
>  #define SIERRA_NET_LSI_UMTS_STATUS_LEN \
>  	(SIERRA_NET_LSI_UMTS_LEN - SIERRA_NET_LSI_COMMON_LEN)
>  
>  /* Forward definitions */
>  static void sierra_sync_timer(unsigned long syncdata);
>  static int sierra_net_change_mtu(struct net_device *net, int new_mtu);
> +static int sierra_net_open (struct net_device *net);
>  
>  /* Our own net device operations structure */
>  static const struct net_device_ops sierra_net_device_ops = {
> -	.ndo_open               = usbnet_open,
> +	.ndo_open               = sierra_net_open,
>  	.ndo_stop               = usbnet_stop,
>  	.ndo_start_xmit         = usbnet_start_xmit,
>  	.ndo_tx_timeout         = usbnet_tx_timeout,
>  	.ndo_change_mtu         = sierra_net_change_mtu,
>  	.ndo_set_mac_address    = eth_mac_addr,
>  	.ndo_validate_addr      = eth_validate_addr,
>  };
> @@ -439,14 +440,15 @@ static void sierra_net_dosync(struct usbnet *dev)
>  		netdev_err(dev->net,
>  			"Send SYNC failed, status %d\n", status);
>  	status = sierra_net_send_sync(dev);
>  	if (status < 0)
>  		netdev_err(dev->net,
>  			"Send SYNC failed, status %d\n", status);
>  
> +printk(KERN_INFO "%s: sent two SYNC messages\n", __func__);
>  	/* Now, start a timer and make sure we get the Restart Indication */
>  	priv->sync_timer.function = sierra_sync_timer;
>  	priv->sync_timer.data = (unsigned long) dev;
>  	priv->sync_timer.expires = jiffies + SIERRA_NET_SYNCDELAY;
>  	add_timer(&priv->sync_timer);
>  }
>  
> @@ -497,31 +499,34 @@ static void sierra_net_kevent(struct work_struct *work)
>  				netdev_err(dev->net, "%s: Bad packet, received"
>  					" %d, expected %d\n",	__func__, len,
>  					hh.hdrlen + hh.payload_len.word);
>  				kfree(buf);
>  				return;
>  			}
>  
> +printk(KERN_INFO "%s: received msg 0x%02x len %d\n", __func__, hh.msgid.byte, len);
>  			/* Switch on received message types */
>  			switch (hh.msgid.byte) {
>  			case SIERRA_NET_HIP_LSI_UMTSID:
>  				dev_dbg(&dev->udev->dev, "LSI for ctx:%d",
>  					hh.msgspecific.byte);
>  				sierra_net_handle_lsi(dev, buf, &hh);
>  				break;
>  			case SIERRA_NET_HIP_RESTART_ID:
> +printk(KERN_INFO "%s: RESTART received code 0x%02x\n", __func__, hh.msgspecific.byte);
>  				dev_dbg(&dev->udev->dev, "Restart reported: %d,"
>  						" stopping sync timer",
>  						hh.msgspecific.byte);
>  				/* Got sync resp - stop timer & clear mask */
>  				del_timer_sync(&priv->sync_timer);
>  				clear_bit(SIERRA_NET_TIMER_EXPIRY,
>  					  &priv->kevent_flags);
>  				break;
>  			case SIERRA_NET_HIP_HSYNC_ID:
> +printk(KERN_INFO "%s: HSYNC received\n", __func__);
>  				dev_dbg(&dev->udev->dev, "SYNC received");
>  				err = sierra_net_send_sync(dev);
>  				if (err < 0)
>  					netdev_err(dev->net,
>  						"Send SYNC failed %d\n", err);
>  				break;
>  			case SIERRA_NET_HIP_EXTENDEDID:
> @@ -537,14 +542,15 @@ static void sierra_net_kevent(struct work_struct *work)
>  				break;
>  			}
>  		}
>  		kfree(buf);
>  	}
>  	/* The sync timer bit might be set */
>  	if (test_bit(SIERRA_NET_TIMER_EXPIRY, &priv->kevent_flags)) {
> +printk(KERN_INFO "%s: re-sending SYNC\n", __func__);
>  		clear_bit(SIERRA_NET_TIMER_EXPIRY, &priv->kevent_flags);
>  		dev_dbg(&dev->udev->dev, "Deferred sync timer expiry");
>  		sierra_net_dosync(priv->usbnet);
>  	}
>  
>  	if (priv->kevent_flags)
>  		dev_dbg(&dev->udev->dev, "sierra_net_kevent done, "
> @@ -562,14 +568,15 @@ static void sierra_net_defer_kevent(struct usbnet *dev, int work)
>  /*
>   * Sync Retransmit Timer Handler. On expiry, kick the work queue
>   */
>  static void sierra_sync_timer(unsigned long syncdata)
>  {
>  	struct usbnet *dev = (struct usbnet *)syncdata;
>  
> +printk(KERN_INFO "%s: sync timer expired\n", __func__);
>  	dev_dbg(&dev->udev->dev, "%s", __func__);
>  	/* Kick the tasklet */
>  	sierra_net_defer_kevent(dev, SIERRA_NET_TIMER_EXPIRY);
>  }
>  
>  static void sierra_net_status(struct usbnet *dev, struct urb *urb)
>  {
> @@ -584,14 +591,15 @@ static void sierra_net_status(struct usbnet *dev, struct urb *urb)
>  	event = urb->transfer_buffer;
>  	switch (event->bNotificationType) {
>  	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
>  	case USB_CDC_NOTIFY_SPEED_CHANGE:
>  		/* USB 305 sends those */
>  		break;
>  	case USB_CDC_NOTIFY_RESPONSE_AVAILABLE:
> +printk(KERN_INFO "%s: firmware indicates response available\n", __func__);
>  		sierra_net_defer_kevent(dev, SIERRA_NET_EVENT_RESP_AVAIL);
>  		break;
>  	default:
>  		netdev_err(dev->net, ": unexpected notification %02x!\n",
>  				event->bNotificationType);
>  		break;
>  	}
> @@ -767,20 +775,32 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
>  	/* tell modem we are going away */
>  	status = sierra_net_send_cmd(dev, priv->shdwn_msg,
>  			sizeof(priv->shdwn_msg), "Shutdown");
>  	if (status < 0)
>  		netdev_err(dev->net,
>  			"usb_control_msg failed, status %d\n", status);
>  
> -	usbnet_status_stop(dev);
> -
>  	sierra_net_set_private(dev, NULL);
>  	kfree(priv);
>  }
>  
> +static int sierra_net_open (struct net_device *net)
> +{
> +	int ret;
> +
> +	ret = usbnet_open(net);
> +	if (ret == 0) {
> +		struct usbnet *dev = netdev_priv(net);
> +
> +		/* Interrupt URB now set up; initiate sync sequence */
> +		sierra_net_dosync(dev);
> +	}
> +	return ret;
> +}
> +
>  static struct sk_buff *sierra_net_skb_clone(struct usbnet *dev,
>  		struct sk_buff *skb, int len)
>  {
>  	struct sk_buff *new_skb;
>  
>  	/* clone skb */
>  	new_skb = skb_clone(skb, GFP_ATOMIC);
> @@ -910,14 +930,15 @@ static const struct driver_info sierra_net_info_direct_ip = {
>  	.bind = sierra_net_bind,
>  	.unbind = sierra_net_unbind,
>  	.status = sierra_net_status,
>  	.rx_fixup = sierra_net_rx_fixup,
>  	.tx_fixup = sierra_net_tx_fixup,
>  };
>  
> +#if 0
>  static int
>  sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
>  {
>  	int ret;
>  
>  	ret = usbnet_probe(udev, prod);
>  	if (ret == 0) {
> @@ -927,14 +948,15 @@ sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
>  		if (ret == 0) {
>  			/* Interrupt URB now set up; initiate sync sequence */
>  			sierra_net_dosync(dev);
>  		}
>  	}
>  	return ret;
>  }
> +#endif
>  
>  #define DIRECT_IP_DEVICE(vend, prod) \
>  	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
>  	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
>  	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 10), \
>  	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
>  	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 11), \
> @@ -950,15 +972,15 @@ static const struct usb_device_id products[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, products);
>  
>  /* We are based on usbnet, so let it handle the USB driver specifics */
>  static struct usb_driver sierra_net_driver = {
>  	.name = "sierra_net",
>  	.id_table = products,
> -	.probe = sierra_net_probe,
> +	.probe = usbnet_probe,
>  	.disconnect = usbnet_disconnect,
>  	.suspend = usbnet_suspend,
>  	.resume = usbnet_resume,
>  	.no_dynamic_id = 1,
>  	.disable_hub_initiated_lpm = 1,
>  };
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Bjørn Mork]

Dan Williams <dcbw@redhat.com> writes:
> On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
>> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
>> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
>> > 
>> > It's not easy to create a driver for all the various firmware
>> > bugs out there.
>> > 
>> > This change caused regressions for a number of devices, which
>> > started to fail link detection and therefore became completely
>> > non-functional. The exact reason is yet unknown, it looks like
>> > the affected firmwares might actually need all or some of the
>> > additional SYNC messages the patch got rid of.
>> > 
>> > Reverting is not optimal, as it will re-introduce the original
>> > problem, but it is currently the only alternative known to fix
>> > this issue.
>> 
>> Instead, how does the following patch work for you?
>
> Bjorn, did you have a chance to try this patch out on your devices?

The only DirectIP device I have is the MC7710, which never had any of
the firmware issues you are trying to fix. I only tried to forward Johns
issue.

When this patch worked for John, then I am pretty confident that you
have solved the problem here.



Thanks,
Bjørn

Re: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Dan Williams]

On Fri, 2013-11-08 at 21:44 +0100, Bjørn Mork wrote:
> Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
> >> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> >> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> >> > 
> >> > It's not easy to create a driver for all the various firmware
> >> > bugs out there.
> >> > 
> >> > This change caused regressions for a number of devices, which
> >> > started to fail link detection and therefore became completely
> >> > non-functional. The exact reason is yet unknown, it looks like
> >> > the affected firmwares might actually need all or some of the
> >> > additional SYNC messages the patch got rid of.
> >> > 
> >> > Reverting is not optimal, as it will re-introduce the original
> >> > problem, but it is currently the only alternative known to fix
> >> > this issue.
> >> 
> >> Instead, how does the following patch work for you?
> >
> > Bjorn, did you have a chance to try this patch out on your devices?
> 
> The only DirectIP device I have is the MC7710, which never had any of
> the firmware issues you are trying to fix. I only tried to forward Johns
> issue.
> 
> When this patch worked for John, then I am pretty confident that you
> have solved the problem here.

Well, "solved", since I still have no idea why the original patch would
cause the device behavior based on what I know and have read about the
expected firmware/host handshaking sequence.  But the patch there
appears to fix the problem *and* not blindly send tons of SYNCs forever.

So I'll go ahead and submit a proper version of it.

Dan

--
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: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Dan Williams]

On Fri, 2013-11-08 at 15:29 -0600, Dan Williams wrote:
> On Fri, 2013-11-08 at 21:44 +0100, Bjørn Mork wrote:
> > Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > > On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
> > >> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> > >> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> > >> > 
> > >> > It's not easy to create a driver for all the various firmware
> > >> > bugs out there.
> > >> > 
> > >> > This change caused regressions for a number of devices, which
> > >> > started to fail link detection and therefore became completely
> > >> > non-functional. The exact reason is yet unknown, it looks like
> > >> > the affected firmwares might actually need all or some of the
> > >> > additional SYNC messages the patch got rid of.
> > >> > 
> > >> > Reverting is not optimal, as it will re-introduce the original
> > >> > problem, but it is currently the only alternative known to fix
> > >> > this issue.
> > >> 
> > >> Instead, how does the following patch work for you?
> > >
> > > Bjorn, did you have a chance to try this patch out on your devices?
> > 
> > The only DirectIP device I have is the MC7710, which never had any of
> > the firmware issues you are trying to fix. I only tried to forward Johns
> > issue.
> > 
> > When this patch worked for John, then I am pretty confident that you
> > have solved the problem here.
> 
> Well, "solved", since I still have no idea why the original patch would
> cause the device behavior based on what I know and have read about the
> expected firmware/host handshaking sequence.  But the patch there
> appears to fix the problem *and* not blindly send tons of SYNCs forever.
> 
> So I'll go ahead and submit a proper version of it.

Actually, is "[PATCH] usbnet: fix status interrupt urb handling" the
real fix for this problem?

John, any chance you could revert my RFC patch and try Felix's patch in
that mail?

Dan

--
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: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Dan Williams]

On Tue, 2013-11-12 at 10:25 -0600, Dan Williams wrote:
> On Fri, 2013-11-08 at 15:29 -0600, Dan Williams wrote:
> > On Fri, 2013-11-08 at 21:44 +0100, Bjørn Mork wrote:
> > > Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > > > On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
> > > >> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> > > >> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> > > >> > 
> > > >> > It's not easy to create a driver for all the various firmware
> > > >> > bugs out there.
> > > >> > 
> > > >> > This change caused regressions for a number of devices, which
> > > >> > started to fail link detection and therefore became completely
> > > >> > non-functional. The exact reason is yet unknown, it looks like
> > > >> > the affected firmwares might actually need all or some of the
> > > >> > additional SYNC messages the patch got rid of.
> > > >> > 
> > > >> > Reverting is not optimal, as it will re-introduce the original
> > > >> > problem, but it is currently the only alternative known to fix
> > > >> > this issue.
> > > >> 
> > > >> Instead, how does the following patch work for you?
> > > >
> > > > Bjorn, did you have a chance to try this patch out on your devices?
> > > 
> > > The only DirectIP device I have is the MC7710, which never had any of
> > > the firmware issues you are trying to fix. I only tried to forward Johns
> > > issue.
> > > 
> > > When this patch worked for John, then I am pretty confident that you
> > > have solved the problem here.
> > 
> > Well, "solved", since I still have no idea why the original patch would
> > cause the device behavior based on what I know and have read about the
> > expected firmware/host handshaking sequence.  But the patch there
> > appears to fix the problem *and* not blindly send tons of SYNCs forever.
> > 
> > So I'll go ahead and submit a proper version of it.
> 
> Actually, is "[PATCH] usbnet: fix status interrupt urb handling" the
> real fix for this problem?
> 
> John, any chance you could revert my RFC patch and try Felix's patch in
> that mail?

It fixes the problem for me without requiring the revert or my RFC
patch....

Dan

--
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: [RFC] Revert "sierra_net: keep status interrupt URB active"

[Bjørn Mork]

Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

>> Actually, is "[PATCH] usbnet: fix status interrupt urb handling" the
>> real fix for this problem?
>> 
>> John, any chance you could revert my RFC patch and try Felix's patch
>in
>> that mail?
>
>It fixes the problem for me without requiring the revert or my RFC
>patch....

Yes, this looks like a reasonable explanation. Good timing. 


Bjørn

--
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