[PATCH 0/7] usbnet: misc cleanup

[PATCH 0/7] usbnet: misc cleanup

[Ming Lei]

This patchset does some cleanup on usbnet.

 drivers/net/usb/cdc-phonet.c |    4 +--
 drivers/net/usb/pegasus.c    |    4 ---
 drivers/net/usb/usbnet.c     |   81 +++++++++++++++++++-----------------------
 include/linux/usb/usbnet.h   |    4 +--
 4 files changed, 38 insertions(+), 55 deletions(-)


Thanks,
--
Ming Lei

--
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 1/7] usbnet: remove usb_get/put_dev in .probe and .disconnect

[Ming Lei]

usb_device is parent device of usb_interface in the view of driver
model, so its reference count is always held during .probe/.disconnect
of usb_interface instance.

This patch just removes the unnecessay usb_get/put_dev.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/net/usb/usbnet.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9f58330..022c1e7 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1307,7 +1307,6 @@ void usbnet_disconnect (struct usb_interface *intf)
 	usb_free_urb(dev->interrupt);
 
 	free_netdev(net);
-	usb_put_dev (xdev);
 }
 EXPORT_SYMBOL_GPL(usbnet_disconnect);
 
@@ -1363,8 +1362,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	xdev = interface_to_usbdev (udev);
 	interface = udev->cur_altsetting;
 
-	usb_get_dev (xdev);
-
 	status = -ENOMEM;
 
 	// set up our own records
@@ -1493,7 +1490,6 @@ out3:
 out1:
 	free_netdev(net);
 out:
-	usb_put_dev(xdev);
 	return status;
 }
 EXPORT_SYMBOL_GPL(usbnet_probe);
-- 
1.7.9.5

[PATCH 2/7] usbnet:pegasus: remove usb_get/put_dev in .probe and .disconnect

[Ming Lei]

usb_device is parent device of usb_interface in the view of driver
model, so its reference count is always held during .probe/.disconnect
of usb_interface instance.

This patch just removes the unnecessay usb_get/put_dev.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/usb/pegasus.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 7023220..a0b5807 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -1329,8 +1329,6 @@ static int pegasus_probe(struct usb_interface *intf,
 	}
 	pegasus_count++;
 
-	usb_get_dev(dev);

[PATCH 3/7] usbnet:cdc-phonet: remove usb_get/put_dev in .probe and .disconnect

[Ming Lei]

usb_device is parent device of usb_interface in the view of driver
model, so its reference count is always held during .probe/.disconnect
of usb_interface instance.

This patch just removes the unnecessay usb_get/put_dev.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/net/usb/cdc-phonet.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/usb/cdc-phonet.c b/drivers/net/usb/cdc-phonet.c
index d848d4d..187c144 100644
--- a/drivers/net/usb/cdc-phonet.c
+++ b/drivers/net/usb/cdc-phonet.c
@@ -394,7 +394,7 @@ int usbpn_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	SET_NETDEV_DEV(dev, &intf->dev);
 
 	pnd->dev = dev;
-	pnd->usb = usb_get_dev(usbdev);
+	pnd->usb = usbdev;
 	pnd->intf = intf;
 	pnd->data_intf = data_intf;
 	spin_lock_init(&pnd->tx_lock);
@@ -440,7 +440,6 @@ out:
 static void usbpn_disconnect(struct usb_interface *intf)
 {
 	struct usbpn_dev *pnd = usb_get_intfdata(intf);
-	struct usb_device *usb = pnd->usb;
 
 	if (pnd->disconnected)
 		return;
@@ -449,7 +448,6 @@ static void usbpn_disconnect(struct usb_interface *intf)
 	usb_driver_release_interface(&usbpn_driver,
 			(pnd->intf == intf) ? pnd->data_intf : pnd->intf);
 	unregister_netdev(pnd->dev);
-	usb_put_dev(usb);
 }
 
 static struct usb_driver usbpn_driver = {
-- 
1.7.9.5

[PATCH 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Ming Lei]

EVENT_DEV_OPEN is introduced to mark if the interface is opened or
not, but we already have IFF_UP to handle it, so just
remove the flag and use IFF_UP.

Also the patch may fix one bug in failure path of usbnet_open:
it may return failure but EVENT_DEV_OPEN is not cleared. After
this convering, dev->net->flags & IFF_UP always return consistent
status of the interface.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/usb/usbnet.c   |    6 ++----
 include/linux/usb/usbnet.h |    1 -
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 022c1e7..96f8a08 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -686,7 +686,6 @@ int usbnet_stop (struct net_device *net)
 	struct driver_info	*info = dev->driver_info;
 	int			retval;
 
-	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue (net);
 
 	netif_info(dev, ifdown, dev->net,
@@ -778,7 +777,6 @@ int usbnet_open (struct net_device *net)
 		}
 	}
 
-	set_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_start_queue (net);
 	netif_info(dev, ifup, dev->net,
 		   "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
@@ -1542,7 +1540,7 @@ int usbnet_resume (struct usb_interface *intf)
 
 	if (!--dev->suspend_count) {
 		/* resume interrupt URBs */
-		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
+		if (dev->interrupt && (dev->net->flags & IFF_UP))
 			usb_submit_urb(dev->interrupt, GFP_NOIO);
 
 		spin_lock_irq(&dev->txq.lock);
@@ -1564,7 +1562,7 @@ int usbnet_resume (struct usb_interface *intf)
 		clear_bit(EVENT_DEV_ASLEEP, &dev->flags);
 		spin_unlock_irq(&dev->txq.lock);
 
-		if (test_bit(EVENT_DEV_OPEN, &dev->flags)) {
+		if (dev->net->flags & IFF_UP) {
 			if (!(dev->txq.qlen >= TX_QLEN(dev)))
 				netif_tx_wake_all_queues(dev->net);
 			tasklet_schedule (&dev->bh);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 76f4396..f15a729 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -68,7 +68,6 @@ struct usbnet {
 #		define EVENT_RX_PAUSED	5
 #		define EVENT_DEV_WAKING 6
 #		define EVENT_DEV_ASLEEP 7
-#		define EVENT_DEV_OPEN	8
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.9.5

--
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/7] usbnet: remove flag of EVENT_DEV_WAKING

[Ming Lei]

The flag of EVENT_DEV_WAKING is not used any more, so just remove it.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/linux/usb/usbnet.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index f15a729..b994c0f 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -66,8 +66,7 @@ struct usbnet {
 #		define EVENT_STS_SPLIT	3
 #		define EVENT_LINK_RESET	4
 #		define EVENT_RX_PAUSED	5
-#		define EVENT_DEV_WAKING 6
-#		define EVENT_DEV_ASLEEP 7
+#		define EVENT_DEV_ASLEEP 6
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.9.5

[PATCH 6/7] usbnet: remove declaration for intr_complete

[Ming Lei]

Remove declaration for intr_complete so that ctags may be happy to
decrease duplicated symbols, also decrease one line code.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/usb/usbnet.c |   72 ++++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 96f8a08..f4bd5fa 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -180,7 +180,41 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int iMACAddress)
 }
 EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr);
 
-static void intr_complete (struct urb *urb);
+static void intr_complete (struct urb *urb)
+{
+	struct usbnet	*dev = urb->context;
+	int		status = urb->status;
+
+	switch (status) {
+	/* success */
+	case 0:
+		dev->driver_info->status(dev, urb);
+		break;
+
+	/* software-driven interface shutdown */
+	case -ENOENT:		/* urb killed */
+	case -ESHUTDOWN:	/* hardware gone */
+		netif_dbg(dev, ifdown, dev->net,
+			  "intr shutdown, code %d\n", status);
+		return;
+
+	/* NOTE:  not throttling like RX/TX, since this endpoint
+	 * already polls infrequently
+	 */
+	default:
+		netdev_dbg(dev->net, "intr status %d\n", status);
+		break;
+	}
+
+	if (!netif_running (dev->net))
+		return;
+
+	memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
+	status = usb_submit_urb (urb, GFP_ATOMIC);
+	if (status != 0)
+		netif_err(dev, timer, dev->net,
+			  "intr resubmit --> %d\n", status);
+}
 
 static int init_status (struct usbnet *dev, struct usb_interface *intf)
 {
@@ -519,42 +553,6 @@ block:
 	netif_dbg(dev, rx_err, dev->net, "no read resubmitted\n");
 }
 
-static void intr_complete (struct urb *urb)
-{
-	struct usbnet	*dev = urb->context;
-	int		status = urb->status;
-
-	switch (status) {
-	/* success */
-	case 0:
-		dev->driver_info->status(dev, urb);
-		break;
-
-	/* software-driven interface shutdown */
-	case -ENOENT:		/* urb killed */
-	case -ESHUTDOWN:	/* hardware gone */
-		netif_dbg(dev, ifdown, dev->net,
-			  "intr shutdown, code %d\n", status);
-		return;
-
-	/* NOTE:  not throttling like RX/TX, since this endpoint
-	 * already polls infrequently
-	 */
-	default:
-		netdev_dbg(dev->net, "intr status %d\n", status);
-		break;
-	}
-
-	if (!netif_running (dev->net))
-		return;
-
-	memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
-	status = usb_submit_urb (urb, GFP_ATOMIC);
-	if (status != 0)
-		netif_err(dev, timer, dev->net,
-			  "intr resubmit --> %d\n", status);
-}

[PATCH 7/7] usbnet: don't initialize transfer buffer before submit status URB

[Ming Lei]

The line below in intr_complete isn't needed,

	memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);

so just remove it.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/usb/usbnet.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f4bd5fa..5d7956d 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -209,7 +209,6 @@ static void intr_complete (struct urb *urb)
 	if (!netif_running (dev->net))
 		return;
 
-	memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
 	status = usb_submit_urb (urb, GFP_ATOMIC);
 	if (status != 0)
 		netif_err(dev, timer, dev->net,
-- 
1.7.9.5

--
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 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Oliver Neukum]

Am Dienstag, 12. Juni 2012, 03:19:42 schrieb Ming Lei:
> EVENT_DEV_OPEN is introduced to mark if the interface is opened or
> not, but we already have IFF_UP to handle it, so just
> remove the flag and use IFF_UP.

When is IFF_UP cleared? The flag is tested in usbnet_resume(),
so it must be cleared before usbnet_stop() is called.

	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

Re: [PATCH 0/7] usbnet: misc cleanup

[David Miller]

From: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Tue, 12 Jun 2012 09:19:38 +0800

> This patchset does some cleanup on usbnet.

I've applied all of these patches except the EVENT_DEV_OPEN removal
which still needs some discussion.
--
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 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Ming Lei]

On Wed, Jun 13, 2012 at 2:14 AM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Dienstag, 12. Juni 2012, 03:19:42 schrieb Ming Lei:
>> EVENT_DEV_OPEN is introduced to mark if the interface is opened or
>> not, but we already have IFF_UP to handle it, so just
>> remove the flag and use IFF_UP.
>
> When is IFF_UP cleared? The flag is tested in usbnet_resume(),

The flag is cleared just after usbnet_stop completes.

> so it must be cleared before usbnet_stop() is called.

Yes, I see, otherwise system or runtime resume may happen
at the same time with usbnet_stop.

Thanking you for point it out.

Thanks,
-- 
Ming Lei

Re: [PATCH 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Ming Lei]

On Wed, Jun 13, 2012 at 10:12 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Wed, Jun 13, 2012 at 2:14 AM, Oliver Neukum <oliver@neukum.org> wrote:
>> Am Dienstag, 12. Juni 2012, 03:19:42 schrieb Ming Lei:
>>> EVENT_DEV_OPEN is introduced to mark if the interface is opened or
>>> not, but we already have IFF_UP to handle it, so just
>>> remove the flag and use IFF_UP.
>>
>> When is IFF_UP cleared? The flag is tested in usbnet_resume(),
>
> The flag is cleared just after usbnet_stop completes.

Looks we can use the below to replace EVENT_DEV_OPEN:

     (netif_running((dev)->net) && ((dev)->net->flags & IFF_UP))


Thanks,
-- 
Ming Lei

Re: [PATCH 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Oliver Neukum]

Am Mittwoch, 13. Juni 2012, 06:47:18 schrieb Ming Lei:
> On Wed, Jun 13, 2012 at 10:12 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> > On Wed, Jun 13, 2012 at 2:14 AM, Oliver Neukum <oliver@neukum.org> wrote:
> >> Am Dienstag, 12. Juni 2012, 03:19:42 schrieb Ming Lei:
> >>> EVENT_DEV_OPEN is introduced to mark if the interface is opened or
> >>> not, but we already have IFF_UP to handle it, so just
> >>> remove the flag and use IFF_UP.
> >>
> >> When is IFF_UP cleared? The flag is tested in usbnet_resume(),
> >
> > The flag is cleared just after usbnet_stop completes.
> 
> Looks we can use the below to replace EVENT_DEV_OPEN:
> 
>      (netif_running((dev)->net) && ((dev)->net->flags & IFF_UP))

That goes down a bit into the interna of the network code.
If we do this, please encapsulated and with a big fat comment.

	Regards
		Oliver

Re: [PATCH 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Bjørn Mork]

Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> writes:
> Am Mittwoch, 13. Juni 2012, 06:47:18 schrieb Ming Lei:
>> On Wed, Jun 13, 2012 at 10:12 AM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Wed, Jun 13, 2012 at 2:14 AM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
>> >> Am Dienstag, 12. Juni 2012, 03:19:42 schrieb Ming Lei:
>> >>> EVENT_DEV_OPEN is introduced to mark if the interface is opened or
>> >>> not, but we already have IFF_UP to handle it, so just
>> >>> remove the flag and use IFF_UP.
>> >>
>> >> When is IFF_UP cleared? The flag is tested in usbnet_resume(),
>> >
>> > The flag is cleared just after usbnet_stop completes.
>> 
>> Looks we can use the below to replace EVENT_DEV_OPEN:
>> 
>>      (netif_running((dev)->net) && ((dev)->net->flags & IFF_UP))
>
> That goes down a bit into the interna of the network code.
> If we do this, please encapsulated and with a big fat comment.

There are already plenty of places in usbnet.c where
netif_running(dev->net) is tested instead of EVENT_DEV_OPEN.
Why should the test in usbnet_resume be any different?

The only reason I see is that some devices might want to keep interrupts
running without the netdev being up, but the current code doesn't
support that anyway.  So better implement it when there is a device and
driver needing it.

BTW, does the "&& (dev->net->flags & IFF_UP)" really make any
difference, or could the test be simplified to

    (netif_running(dev->net))





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

Re: [PATCH 4/7] usbnet: remove EVENT_DEV_OPEN flag

[Ming Lei]

On Wed, Jun 13, 2012 at 4:28 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>
> BTW, does the "&& (dev->net->flags & IFF_UP)" really make any
> difference, or could the test be simplified to
>
>    (netif_running(dev->net))

Neither netif_running(dev->net) nor  (dev->net->flags & IFF_UP)
is enough.

In the start of usbnet_open(), the usb device may be waken up and
usbnet_resume will see netif_running(dev->net) in the situation, so
may cause problem since the interface hasn't been UP yet.

If just checking on (dev->net->flags & IFF_UP), it still may cause
problem in usbnet_resume as pointed by Oliver.

So looks only checking on both netif_running(dev->net) and
(dev->net->flags & IFF_UP) is OK.

Thanks,
-- 
Ming Lei
--
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