Re: [PATCH 1/7] dynamic debug v2 - infrastructure

[Greg KH]

On Thu, Jul 17, 2008 at 05:20:40PM -0400, Jason Baron wrote:
> On Thu, Jul 17, 2008 at 12:01:03AM -0700, Greg KH wrote:
> > Return-Path: <greg@xxxxxxxxx>
> > On Tue, Jul 15, 2008 at 05:31:08PM -0400, Jason Baron wrote:
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 1a06026..fb03dbc 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -592,7 +592,11 @@ extern const char *dev_driver_string(struct device *dev);
> > >  #define dev_info(dev, format, arg...)		\
> > >  	dev_printk(KERN_INFO , dev , format , ## arg)
> > >  
> > > -#ifdef DEBUG
> > > +#if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
> > > +#define dev_dbg(dev, format, ...) do { \
> > > +	dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
> > > +	} while (0)
> > > +#elif defined(DEBUG)
> > >  #define dev_dbg(dev, format, arg...)		\
> > >  	dev_printk(KERN_DEBUG , dev , format , ## arg)
> > >  #else
> > 
> > In looking at your follow-on patches, where you add this to subsystems,
> > it seems that you have to add a lot of #include <linux/dynamic_printk.h>
> > lines.
> > 
> > And in looking at this modification to device.h, I think lots of the
> > kernel should break, but you are getting it "for free" by including the
> > .h file within kernel.h.
> > 
> > Why not also include it here in device.h?
> > 
> 
> I don't believe the follow-on patches include "<linux/dynamic_printk.h>". 
> However, some of them do include a subsystem specific #include. For example,
> the cpufreq subsystem include, <linux/dynamic_debug_cpufreq.h>:

Ah, you are correct, sorry about that, I misread.

> +++ b/include/linux/dynamic_debug_cpufreq.h
> @@ -0,0 +1,8 @@
> +#define DYNAMIC_DEBUG_NUM_FLAGS "3"
> +#define DYNAMIC_DEBUG_FLAG_NAMES "CPUFREQ_DEBUG_CORE,CPUFREQ_DEBUG_DRIVER,CPUFREQ_DEBUG_GOVERNOR"
> +#define DYNAMIC_DEBUG_TYPE "2"
> +#define DYNAMIC_DEBUG_MODNAME "cpufreq_shared"
> +
> +#ifdef CONFIG_CPU_FREQ_DEBUG
> +#define DEBUG 1
> +#endif
> 
> This header file conveys that cpufreq subsystem, wants non-default options-it
> has 3 debugging flags that can be set, and all modules in the subsystem are
> tied together using the module name: "cpufreq_shared".  
> 
> I don't have an explicit #include of kernel.h in device.h b/c it already picked
> up.
> 
> > Also, with this change, code that is already using dev_dbg() today is
> > instantly converted over to this logic now, right?
> > 
> 
> that is correct. any callers of dev_dbg() don't have to do anything. its really
> only the more complex debugging, where there are flags or levels that need to
> make adjustments to work with the new infrastructure.

For this reason alone, I see no reason why your patch should not be
merged today.  You don't need the other subsystems at this point in time
in my opinion, it's benifit is huge already.

Not to say that you shouldn't also go after these subsystems in your
overall scheme here, but please, don't feel it should hold your code up
at this point in time :)

Because of that, do you want me to try to merge the basic infrastructure
and dev_dbg() support through the driver-core tree?  This code has been
in the past -mm and linux-next releases, right?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/