* Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering [not found] <200503302319.j2UNJEBP019719@hera.kernel.org> @ 2005-03-31 0:30 ` Jeff Garzik 2005-03-31 0:50 ` David Brownell 0 siblings, 1 reply; 4+ messages in thread From: Jeff Garzik @ 2005-03-31 0:30 UTC (permalink / raw) To: Linux Kernel Mailing List, dbrownell, Greg KH Linux Kernel Mailing List wrote: > ChangeSet 1.2181.4.72, 2005/03/24 15:31:29-08:00, david-b@pacbell.net > > [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering > > This converts most of the usbnet code to actually use the ethtool > message flags. The ASIX code is left untouched, since there are > a bunch of patches pending there ... that's where the remaining > handful of "sparse -Wbitwise" warnings come from. > > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> It would be nice if people at least CC'd me on net driver patches. > @@ -2886,18 +2885,21 @@ > defer_kevent (dev, EVENT_RX_MEMORY); > break; > case -ENODEV: > - devdbg (dev, "device gone"); > + if (netif_msg_ifdown (dev)) > + devdbg (dev, "device gone"); > netif_device_detach (dev->net); > break; > default: > - devdbg (dev, "rx submit, %d", retval); > + if (netif_msg_rx_err (dev)) > + devdbg (dev, "rx submit, %d", retval); > tasklet_schedule (&dev->bh); > break; > case 0: > __skb_queue_tail (&dev->rxq, skb); > } > } else { > - devdbg (dev, "rx: stopped"); > + if (netif_msg_ifdown (dev)) > + devdbg (dev, "rx: stopped"); > retval = -ENOLINK; > } > spin_unlock_irqrestore (&dev->rxq.lock, lockflags); netfi_msg_ifdown() is only for __interface__ up/down events; as such, there should be only one message of this type in dev->open(), and one message of this type in dev->stop(). > @@ -2963,9 +2967,8 @@ > // software-driven interface shutdown > case -ECONNRESET: // async unlink > case -ESHUTDOWN: // hardware gone > -#ifdef VERBOSE > - devdbg (dev, "rx shutdown, code %d", urb_status); > -#endif > + if (netif_msg_ifdown (dev)) > + devdbg (dev, "rx shutdown, code %d", urb_status); > goto block; > > // we get controller i/o faults during khubd disconnect() delays. ditto > @@ -3026,9 +3030,8 @@ > /* software-driven interface shutdown */ > case -ENOENT: // urb killed > case -ESHUTDOWN: // hardware gone > -#ifdef VERBOSE > - devdbg (dev, "intr shutdown, code %d", status); > -#endif > + if (netif_msg_ifdown (dev)) > + devdbg (dev, "intr shutdown, code %d", status); > return; > > /* NOTE: not throttling like RX/TX, since this endpoint ditto > @@ -3044,7 +3047,7 @@ > > memset(urb->transfer_buffer, 0, urb->transfer_buffer_length); > status = usb_submit_urb (urb, GFP_ATOMIC); > - if (status != 0) > + if (status != 0 && netif_msg_timer (dev)) > deverr(dev, "intr resubmit --> %d", status); > } > this looks more like a debugging message? > @@ -3094,7 +3097,7 @@ > > netif_stop_queue (net); > > - if (dev->msg_level >= 2) > + if (netif_msg_ifdown (dev)) > devinfo (dev, "stop stats: rx/tx %ld/%ld, errs %ld/%ld", > dev->stats.rx_packets, dev->stats.tx_packets, > dev->stats.rx_errors, dev->stats.tx_errors see -- this one is actually OK > @@ -3110,7 +3113,8 @@ > && skb_queue_len (&dev->txq) > && skb_queue_len (&dev->done)) { > msleep(UNLINK_TIMEOUT_MS); > - devdbg (dev, "waited for %d urb completions", temp); > + if (netif_msg_ifdown (dev)) > + devdbg (dev, "waited for %d urb completions", temp); > } > dev->wait = NULL; > remove_wait_queue (&unlink_wakeup, &wait); not OK > @@ -3142,16 +3146,19 @@ > > // put into "known safe" state > if (info->reset && (retval = info->reset (dev)) < 0) { > - devinfo (dev, "open reset fail (%d) usbnet usb-%s-%s, %s", > - retval, > - dev->udev->bus->bus_name, dev->udev->devpath, > + if (netif_msg_ifup (dev)) > + devinfo (dev, > + "open reset fail (%d) usbnet usb-%s-%s, %s", > + retval, > + dev->udev->bus->bus_name, dev->udev->devpath, > info->description); > goto done; > } > > // insist peer be connected > if (info->check_connect && (retval = info->check_connect (dev)) < 0) { > - devdbg (dev, "can't open; %d", retval); > + if (netif_msg_ifup (dev)) > + devdbg (dev, "can't open; %d", retval); > goto done; > } > not OK > @@ -3159,13 +3166,14 @@ > if (dev->interrupt) { > retval = usb_submit_urb (dev->interrupt, GFP_KERNEL); > if (retval < 0) { > - deverr (dev, "intr submit %d", retval); > + if (netif_msg_ifup (dev)) > + deverr (dev, "intr submit %d", retval); > goto done; > } > } > > netif_start_queue (net); > - if (dev->msg_level >= 2) { > + if (netif_msg_ifup (dev)) { > char *framing; > > if (dev->driver_info->flags & FLAG_FRAMING_NC) not OK > @@ -3526,7 +3541,7 @@ > if (urb != NULL) > rx_submit (dev, urb, GFP_ATOMIC); > } > - if (temp != dev->rxq.qlen) > + if (temp != dev->rxq.qlen && netif_msg_link (dev)) > devdbg (dev, "rxqlen %d --> %d", > temp, dev->rxq.qlen); > if (dev->rxq.qlen < qlen) highly questionable. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering 2005-03-31 0:30 ` [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering Jeff Garzik @ 2005-03-31 0:50 ` David Brownell 2005-03-31 1:09 ` Jeff Garzik 0 siblings, 1 reply; 4+ messages in thread From: David Brownell @ 2005-03-31 0:50 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux Kernel Mailing List, dbrownell, Greg KH On Wednesday 30 March 2005 4:30 pm, Jeff Garzik wrote: > Linux Kernel Mailing List wrote: > > ChangeSet 1.2181.4.72, 2005/03/24 15:31:29-08:00, david-b@pacbell.net > > > > [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering > > > > This converts most of the usbnet code to actually use the ethtool > > message flags. The ASIX code is left untouched, since there are > > a bunch of patches pending there ... that's where the remaining > > handful of "sparse -Wbitwise" warnings come from. > > > > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> > > It would be nice if people at least CC'd me on net driver patches. Sorry. When drivers fit multiple classifications (e.g. USB _and_ NET, or USB _and_ PCI _and_ PM, etc) it's unfortunately routine that not all interested parties see them until something hits LKML. Even when the changes have significant cross-subsystem impact (these don't). > netfi_msg_ifdown() is only for __interface__ up/down events; as such, > there should be only one message of this type in dev->open(), and one > message of this type in dev->stop(). I was going by the only writeup I've ever seen, which doesn't mention such a rule at all. The messages you highlighted are compatible with these rules: the interface is actually going down at that point. http://www.tux.org/hypermail/linux-vortex/2001-Nov/0021.html If there are other rules, they belong in Documentation/netif-msg.txt don't they? That way folk won't be forced to guess. Or risk accidentally following the "wrong" set of rules... > > @@ -3044,7 +3047,7 @@ > > > > memset(urb->transfer_buffer, 0, urb->transfer_buffer_length); > > status = usb_submit_urb (urb, GFP_ATOMIC); > > - if (status != 0) > > + if (status != 0 && netif_msg_timer (dev)) > > deverr(dev, "intr resubmit --> %d", status); > > } > > > > this looks more like a debugging message? It's an error of the "what do I do now??" variety, triggered by what's effectively a timer callback. USB interrupt transfers are polled by the host controller according to a schedule that's maintained by the HCD. - Dave ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering 2005-03-31 0:50 ` David Brownell @ 2005-03-31 1:09 ` Jeff Garzik 2005-03-31 2:10 ` David Brownell 0 siblings, 1 reply; 4+ messages in thread From: Jeff Garzik @ 2005-03-31 1:09 UTC (permalink / raw) To: David Brownell; +Cc: Linux Kernel Mailing List, dbrownell, Greg KH David Brownell wrote: > On Wednesday 30 March 2005 4:30 pm, Jeff Garzik wrote: > >>Linux Kernel Mailing List wrote: >> >>>ChangeSet 1.2181.4.72, 2005/03/24 15:31:29-08:00, david-b@pacbell.net >>> >>> [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering >>> >>> This converts most of the usbnet code to actually use the ethtool >>> message flags. The ASIX code is left untouched, since there are >>> a bunch of patches pending there ... that's where the remaining >>> handful of "sparse -Wbitwise" warnings come from. >>> >>> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> >> >>It would be nice if people at least CC'd me on net driver patches. > > > Sorry. When drivers fit multiple classifications (e.g. USB _and_ NET, > or USB _and_ PCI _and_ PM, etc) it's unfortunately routine that not all > interested parties see them until something hits LKML. Even when the > changes have significant cross-subsystem impact (these don't). I don't care who merges the patches -- presumably the current system works just fine -- but netdev@oss.sgi.com and I should be reviewing the patches. >>netfi_msg_ifdown() is only for __interface__ up/down events; as such, >>there should be only one message of this type in dev->open(), and one >>message of this type in dev->stop(). > > > I was going by the only writeup I've ever seen, which doesn't mention > such a rule at all. The messages you highlighted are compatible with > these rules: the interface is actually going down at that point. > > http://www.tux.org/hypermail/linux-vortex/2001-Nov/0021.html > > If there are other rules, they belong in Documentation/netif-msg.txt > don't they? That way folk won't be forced to guess. Or risk > accidentally following the "wrong" set of rules... I don't see from the code that the struct net_device interface is going down (via dev->stop) at that point. Am I mistaken? Moreover, if you look at any other user of netif_msg_if{up,down}, you will see that it does not produce multiple lines of status register information opaque to anyone but the programmer. Its not a debugging message, but something a user should feel comfortable enabling (if not enabled by default). >>>@@ -3044,7 +3047,7 @@ >>> >>> memset(urb->transfer_buffer, 0, urb->transfer_buffer_length); >>> status = usb_submit_urb (urb, GFP_ATOMIC); >>>- if (status != 0) >>>+ if (status != 0 && netif_msg_timer (dev)) >>> deverr(dev, "intr resubmit --> %d", status); >>> } >>> >> >>this looks more like a debugging message? > > > It's an error of the "what do I do now??" variety, triggered by > what's effectively a timer callback. USB interrupt transfers > are polled by the host controller according to a schedule that's > maintained by the HCD. The above example seems more like netif_msg_tx_err() or even just KERN_ERR ? Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering 2005-03-31 1:09 ` Jeff Garzik @ 2005-03-31 2:10 ` David Brownell 0 siblings, 0 replies; 4+ messages in thread From: David Brownell @ 2005-03-31 2:10 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux Kernel Mailing List, dbrownell, Greg KH On Wednesday 30 March 2005 5:09 pm, Jeff Garzik wrote: > David Brownell wrote: > > http://www.tux.org/hypermail/linux-vortex/2001-Nov/0021.html > > > > If there are other rules, they belong in Documentation/netif-msg.txt > > don't they? That way folk won't be forced to guess. Or risk > > accidentally following the "wrong" set of rules... > > I don't see from the code that the struct net_device interface is going > down (via dev->stop) at that point. Am I mistaken? It's going down, either by netdev->stop() or by hot-unplug, in every place I used the netif_msg_ifdown() filter. > Moreover, if you look at any other user of netif_msg_if{up,down}, you > will see that it does not produce multiple lines of status register > information opaque to anyone but the programmer. Its not a debugging > message, but something a user should feel comfortable enabling (if not > enabled by default). Again, if that's going to become an official guideline it ought to be documented in Documentation/netif-msg.txt ... along with information about how to handle categorization of debug messages. I suspect a general "how to use debug messages" document would help too. With guidance like "fault paths should have diagnostics that let you distinguish each fault", and "never provide success messages at default message logging levels". (Log spamming ... we hates it forever!) If it's wrong to use netif_msg_if{up,down} for debug messages (which go away when DEBUG isn't defined, and yes are intended for debugging aids) which are related to interfaces going up and down ... then what's the "correct" category to filter those out? > >>>@@ -3044,7 +3047,7 @@ > >>> > >>> memset(urb->transfer_buffer, 0, urb->transfer_buffer_length); > >>> status = usb_submit_urb (urb, GFP_ATOMIC); > >>>- if (status != 0) > >>>+ if (status != 0 && netif_msg_timer (dev)) > >>> deverr(dev, "intr resubmit --> %d", status); > >>> } > >>> > >> > >>this looks more like a debugging message? > > > > > > It's an error of the "what do I do now??" variety, triggered by > > what's effectively a timer callback. USB interrupt transfers > > are polled by the host controller according to a schedule that's > > maintained by the HCD. > > The above example seems more like netif_msg_tx_err() or even just KERN_ERR ? It's not coupled to a packet TX or RX, so the guidelines I've seen suggest netif_msg_{tx,rx}_err() would be wrong. Maybe KERN_ERR with no filter would be right ... but I'm used to having one goal of all message filtering as being able to _completely shut up_ the component. That's why there's a filter ... deverr() does wrap KERN_ERR. - Dave ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-03-31 2:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200503302319.j2UNJEBP019719@hera.kernel.org>
2005-03-31 0:30 ` [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering Jeff Garzik
2005-03-31 0:50 ` David Brownell
2005-03-31 1:09 ` Jeff Garzik
2005-03-31 2:10 ` David Brownell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.