Re: [PATCH 00/12] scsi/NCR5380: fix debugging macros and #include structure

[Joe Perches]

On Tue, 2014-03-18 at 23:00 +1100, Finn Thain wrote:
> On Mon, 17 Mar 2014, Joe Perches wrote:
> > My preference would be to change dprintk to scsi_dbg
> Can you be more specific? I gather you're not referring to the debugging 
> routines in include/scsi/scsi_dbg.h as they aren't equivalent.
> 
> Is it the name "dprintk" you object to?

It's not an objection, just a preference.

I'm happy you're trying to wade through it and
promote some consistency in scsi.  Thank you.

> I went looking in drivers/scsi/ for some kind of naming convention for a 
> conditional printk. There are some other variations on the theme (DEBUG, 
> PDEBUG, PERROR, etc) but dprintk() seems to be the most popular.

True.  scsi is not what I would call an exemplar
for style consistency in linux-kernel.

Where is DEBUG #defined for this code?
git grep -w DEBUG drivers/scsi isn't pretty.

I suggest that dprintk be converted to something
that integrates well with the dynamic_debug
facility.  Most of the helper functions that
are integrated with dynamic_debug are named
something like <some_prefix>_dbg.

Most of the scsi dprintk defines (not NCR)
are similar to:

drivers/scsi/hptiop.h-#if 0
drivers/scsi/hptiop.h:#define dprintk(fmt, args...) do { printk(fmt, ##args); } while(0)
drivers/scsi/hptiop.h-#else
drivers/scsi/hptiop.h:#define dprintk(fmt, args...)
drivers/scsi/hptiop.h-#endif

No dynamic_debug, no format/arg mismatch checking
if not compiled in, no KERN_DEBUG level, etc.

NCR does use pr_debug for the dprintk call, but
it also doesn't verify fmt/arg matching when not
compiled in

drivers/scsi/NCR5380.h-#if NDEBUG
drivers/scsi/NCR5380.h:#define dprintk(flg, fmt, args...) \
drivers/scsi/NCR5380.h- do { if ((NDEBUG) & (flg)) pr_debug(fmt, ## args); } while (0)
[]
drivers/scsi/NCR5380.h-#else
drivers/scsi/NCR5380.h:#define dprintk(flg, fmt, args...)     do {} while (0)

It'd be nice to change the last do {} while (0)
to something like:

#define dprintk(flg, fmt, args...) \
do { if (0) pr_debug(fmt, ## args); } while (0)

so the compiler can always verify but not emit any
actual code or format strings.

Also, using macros with ... and __VA_ARGS__ is
a bit more modern.

#define dprintk(flg, fmt, ...) \
do { if (0) pr_debug(fmt, ##__VA_ARGS__); } while (0)




--
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/