Date:	Fri, 08 Nov 2013 13:29:04 -0600
From:	Dan Williams <dcbw@...hat.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	John Henderson <jhen@...k21.com>
Subject: Re: [RFC] Revert "sierra_net: keep status interrupt URB active"

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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Powered by blists - more mailing lists