On Wed, Nov 13, 2013 at 05:11:30PM -0500, Alan Stern wrote:
> 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.
Ah, I didn't notice that, yes, it 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?
Just add CONFIG_DYNAMIC_DEBUG to your .config file and then add a boot
option to always enable dynamic debug for all of drivers/usb/*? If you
have modules, then edit the module.conf file and add the echo of the
enabling of debug there?
> > > 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.
But the default shouldn't be 1 if DYNAMIC_DEBUG is enabled, as that will
drive normal users of distro kernels crazy, right?
> > > - 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.
{sigh} Yes, you are right, which is why I wrote the API that way years
ago, which I totally forgot. Nevermind...
greg k-h
--
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
