| Message ID | 1339463985-9006-5-git-send-email-tom.leiming@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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,
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 -- 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
Oliver Neukum <oliver@neukum.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@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. 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2012 at 4:28 PM, Bjørn Mork <bjorn@mork.no> 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,
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)
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@gmail.com> --- drivers/net/usb/usbnet.c | 6 ++---- include/linux/usb/usbnet.h | 1 - 2 files changed, 2 insertions(+), 5 deletions(-)