On Sun, 30 Dec 2007, Greg KH wrote:
> > It looks like Greg misused the debugfs API -- which is ironic, because
> > he wrote debugfs in the first place! :-)
>
> Oh crap, sorry, I did mess that up :(
>
> > Let me know if this patch fixes the problem. If it does, I'll submit
> > it to Greg with all the proper accoutrements.
>
> This isn't going to work if CONFIG_DEBUGFS is not enabled either :(
Why not? The debugging files won't be created, true, but the driver
should work just fine regardless. This is exactly the way uhci-hcd
behaves, and it hasn't caused any problems.
> > Index: 2.6.24-rc6-mm1/drivers/usb/host/ohci-hcd.c
> > ===================================================================
> > --- 2.6.24-rc6-mm1.orig/drivers/usb/host/ohci-hcd.c
> > +++ 2.6.24-rc6-mm1/drivers/usb/host/ohci-hcd.c
> > @@ -1067,14 +1067,8 @@ static int __init ohci_hcd_mod_init(void
> >
> > #ifdef DEBUG
> > ohci_debug_root = debugfs_create_dir("ohci", NULL);
> > - if (!ohci_debug_root || IS_ERR(ohci_debug_root)) {
> > - if (!ohci_debug_root)
> > - retval = -ENOENT;
> > - else
> > - retval = PTR_ERR(ohci_debug_root);
> > -
> > - goto error_debug;
> > - }
> > + if (!ohci_debug_root)
> > + return -ENOENT;
>
> It needs to check for ERR_PTR(-ENODEV) which is the return value if
> debugfs is not enabled, and if so, just ignore things.
No. That is the mistake you made before. If debugfs isn't enabled
then the driver should just ignore things, yes -- and in particular it
should ignore the fact that the return code is ERR_PTR(-ENODEV). No
extra checking is required.
> If NULL is returned, or anything else, it's a real error.
If NULL is returned, it's a real error, agreed. But if anything else
is returned then the call was successful as far as the driver is
concerned.
> So, try something like:
> if (!ohci_debug_root) {
> retval = -ENOENT;
> goto error_debug;
> }
> if (IS_ERR(ohci_debug_root) && PTR_ERR(ohci_debug_root) != -ENODEV) {
> retval = PTR_ERR(ohci_debug_root);
> goto error_debug;
> }
>
> and let me know of that works for you.
Greg, it sounds like you have forgotten the rationale you originally
used for specifying the return codes in debugfs! The idea was to
return non-NULL if CONFIG_DEBUGFS was disabled, so that drivers could
pretend the calls had succeeded and not need to worry about matters
beyond their control.
Only a NULL return indicates a genuine error. The kerneldoc for
debugfs_create_dir says this very plainly.
> Although the combination of CONFIG_USB_DEBUG and not CONFIG_DEBUGFS is
> strange, we could just enable CONFIG_DEBUGFS is USB_DEBUG is enabled and
> then simplify this logic a bunch at the same time.
Most distributions enable CONFIG_DEBUGFS by default, don't they? So
this problem only comes up when people make up their own configs.
Having USB_DEBUG enabled and DEBUGFS disabled seems like a perfectly
reasonable combination to me. I wouldn't change any aspect of the
configuration logic.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
