On Thu, 2013-11-14 at 05:31 +0900, Greg KH wrote:
> On Wed, Nov 13, 2013 at 06:13:06PM +0100, [email protected] wrote:
> > From: Oliver Neukum <[email protected]>
> >
> > To allow a full switch to dynamic debugging make the
> > debug parameter conditional on defined(DEBUF) || defined(DYNAMIC_DEBUG)
> >
> > Signed-off-by: Oliver Neukum <[email protected]>
> > ---
> > 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?
Well, shouldn't that wait for the change of the Makefile?
> > 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.
OK
> Or just remove this entirely, as I doubt it's really needed anymore.
>
> > -#else
> > -#define DEBUG_CONFIGURED 0
> > -#define debug 0
> > -#endif
> > -
> > static char *errbuf;
> > #define ERRBUF_LEN (32 * 1024)
> > +#endif
> >
> > static struct kmem_cache *uhci_up_cachep; /* urb_priv */
> >
> > @@ -462,12 +457,14 @@ static irqreturn_t uhci_irq(struct usb_hcd *hcd)
> > if (uhci->rh_state >= UHCI_RH_RUNNING) {
> > dev_err(uhci_dev(uhci),
> > "host controller halted, very bad!\n");
> > +#if defined(DEBUG) || defined(DYNAMIC_DEBUG)
> > if (debug > 1 && errbuf) {
> > /* Print the schedule for debugging */
> > uhci_sprint_schedule(uhci, errbuf,
> > ERRBUF_LEN - EXTRA_SPACE);
> > lprintk(errbuf);
> > }
> > +#endif
> > uhci_hc_died(uhci);
> > usb_hc_died(hcd);
> >
> > @@ -516,13 +513,12 @@ static void release_uhci(struct uhci_hcd *uhci)
> > {
> > int i;
> >
> > - 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.
>
> > +
> > + debugfs_remove(uhci->dentry);
> >
> > for (i = 0; i < UHCI_NUM_SKELQH; i++)
> > uhci_free_qh(uhci, uhci->skelqh[i]);
> > @@ -823,8 +819,10 @@ static int uhci_count_ports(struct usb_hcd *hcd)
> > if (!(portstatus & 0x0080) || portstatus == 0xffff)
> > break;
> > }
> > +#if defined(DEBUG) || defined(DYNAMIC_DEBUG)
> > if (debug)
> > dev_info(uhci_dev(uhci), "detected %d ports\n", port);
>
> Just make this a dev_dbg() call which will handle all of the needed
> permission checking of DYNAMIC_DEBUG?
OK
> > +#endif
> >
> > /* Anything greater than 7 is weird so we'll ignore it. */
> > if (port > UHCI_RH_MAXCHILD) {
> > @@ -868,14 +866,14 @@ static int __init uhci_hcd_init(void)
> > ignore_oc ? ", overcurrent ignored" : "");
> > set_bit(USB_UHCI_LOADED, &usb_hcds_loaded);
> >
> > - 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.
OK
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
