Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering

Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering

[Jeff Garzik]

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.

Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering

[David Brownell]

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

Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering

[Jeff Garzik]

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

Re: [PATCH] USB: usbnet uses netif_msg_*() ethtool filtering

[David Brownell]

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