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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:	Fri,  1 Nov 2013 13:53:48 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	netdev@...r.kernel.org
Cc:	linux-usb@...r.kernel.org, John Henderson <jhen@...k21.com>,
	Bjørn Mork <bjorn@...k.no>,
	Dan Williams <dcbw@...hat.com>
Subject: [RFC] Revert "sierra_net: keep status interrupt URB active"

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@...hat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=998342
Reported-by: John Henderson <jhen@...k21.com>
Signed-off-by: Bjørn Mork <bjorn@...k.no>
---
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;
-}
-
 #define DIRECT_IP_DEVICE(vend, prod) \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
@@ -954,7 +930,7 @@ MODULE_DEVICE_TABLE(usb, products);
 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,
-- 
1.7.10.4

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