On Thu, 14 Nov 2013, Greg KH wrote:
> On Wed, Nov 13, 2013 at 06:13:06PM +0100, oliver@xxxxxxxxxx wrote:
> > From: Oliver Neukum <oneukum@xxxxxxx>
> >
> > To allow a full switch to dynamic debugging make the
> > debug parameter conditional on defined(DEBUF) || defined(DYNAMIC_DEBUG)
> >
> > Signed-off-by: Oliver Neukum <oneukum@xxxxxxx>
> > ---
> > drivers/usb/host/uhci-hcd.c | 44 +++++++++++++++++++++++---------------------
> > 1 file changed, 23 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> > index 4a86b63..c53058d 100644
> > --- a/drivers/usb/host/uhci-hcd.c
> > +++ b/drivers/usb/host/uhci-hcd.c
> > @@ -69,19 +69,14 @@ MODULE_PARM_DESC(ignore_oc, "ignore hardware overcurrent indications");
> > * show all queues in /sys/kernel/debug/uhci/[pci_addr]
> > * debug = 3, show all TDs in URBs when dumping
> > */
> > -#ifdef DEBUG
> > -#define DEBUG_CONFIGURED 1
> > +#if defined(DEBUG) || defined(DYNAMIC_DEBUG)
>
> My goal would be to see DEBUG go away entirely, so why not just remove
> it here as well and only depend on DYNAMIC_DEBUG?
What Oliver did here is along the same lines as what Xenia Ragiadakou
did in commit 1512c91f1c76. Maybe that should be updated as well.
I wanted to have a way of enabling USB debugging permanently for my
personal testing tree. What's the best way to do that? Keep a local
patch that adds "CFLAGS += -DDYNAMIC_DEBUG" to the Makefiles?
> > static int debug = 1;
> > module_param(debug, int, S_IRUGO | S_IWUSR);
> > MODULE_PARM_DESC(debug, "Debug level");
>
> You can leave the module parameter in, but just set the default to 0,
> and we should be fine.
I don't think the default should be 0. It would mean users would
have to set the module parameter in addition to turning on dynamic
debugging, in order to get any output.
> Or just remove this entirely, as I doubt it's really needed anymore.
Actually, I can't remember the last time anybody needed debugging
output for uhci-hcd...
> > - if (DEBUG_CONFIGURED) {
> > - spin_lock_irq(&uhci->lock);
> > - uhci->is_initialized = 0;
> > - spin_unlock_irq(&uhci->lock);
> >
> > - debugfs_remove(uhci->dentry);
> > - }
> > + spin_lock_irq(&uhci->lock);
> > + uhci->is_initialized = 0;
> > + spin_unlock_irq(&uhci->lock);
>
> Funny that we only use this flag today if debugging is enabled, I never
> noticed that.
No, the flag gets used even when debugging isn't enabled. The purpose
of that line is to prevent debugging output from being generated while
the driver is being unbound.
> > - if (DEBUG_CONFIGURED) {
> > - errbuf = kmalloc(ERRBUF_LEN, GFP_KERNEL);
> > - if (!errbuf)
> > - goto errbuf_failed;
> > - uhci_debugfs_root = debugfs_create_dir("uhci", usb_debug_root);
> > - if (!uhci_debugfs_root)
> > - goto debug_failed;
> > - }
> > +#if defined(DEBUG) || defined(DYNAMIC_DEBUG)
> > + errbuf = kmalloc(ERRBUF_LEN, GFP_KERNEL);
> > + if (!errbuf)
> > + goto errbuf_failed;
> > +#endif
> > + uhci_debugfs_root = debugfs_create_dir("uhci", usb_debug_root);
> > + if (!uhci_debugfs_root)
> > + goto debug_failed;
>
> You should never need to check the return values of debugfs calls,
> unless you _really_ want to. If you do, then you need to handle the
> -ENODEV case for when it is just compiled away, you don't want to abort
> the driver load on that case.
I think you have this backward. If you want to check the return values
of debugfs calls, you only need to check for NULL. You can ignore
-ENODEV, because it means debugfs wasn't configured.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
