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

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

[Finn Thain]

(Second attempt... sorry for the earlier spam.)

This patch series addresses several issues with NCR5380 drivers:

1. The complex network of #include directives.

2. Three inconsistent implementations of the core driver all attempting
   to share the same macro definitions in NCR5380.h.

3. Broken debugging code.

In the past these issues have led to compiler warnings and ugly hacks to
fix build failures.

This patch series fixes the debugging code by reducing the divergence
between the various core driver implementations.

The final two patches in the series further reduce divergence by refactoring
sun3_scsi.c and sun3_scsi_vme.c so that they follow the same structure as
the other NCR5380 drivers.

By the end of this patch series over 800 net lines of code have been
removed. This is mostly duplicated code that's easily eliminated once the
debugging code is made consistent (and some dead code is removed).

Better uniformity and less duplication should assist future work such as
modernization and trivial clean-up.

To make code review easier I've tried to keep these patches succinct and
free of extraneous changes. Though I did run checkpatch.pl, I've ignored
whitespace issues in existing code. I will send separate patches for
whitespace clean-up of NCR5380 drivers.

All NCR5380 drivers have been compile-tested with this patch series:
  arm/cumana_1.c
  arm/oak.c
  atari_scsi.c
  dmx3191d.c
  dtc.c
  g_NCR5380.c
  g_NCR5380_mmio.c
  mac_scsi.c
  pas16.c
  sun3_scsi.c
  sun3_scsi_vme.c
  t128.c

I've successfully regression tested this patch series using mac_scsi on a 
PowerBook 180. The debugging macros are now usable again.

-- 

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

[Joe Perches]

On Tue, 2014-03-18 at 11:28 +1100, Finn Thain wrote:
> This patch series addresses several issues with NCR5380 drivers:
[]
> 3. Broken debugging code.

My preference would be to change dprintk to scsi_dbg

Seems sensible otherwise.

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

[Finn Thain]

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?

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.

-- 

[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)

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

[Geert Uytterhoeven]

On Tue, Mar 18, 2014 at 1:45 PM, Joe Perches <joe@perches.com> wrote:
> 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)

Na, no_printk():

#define dprintk(flg, fmt, ...) no_printk(fmt, ##__VA_ARGS__)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Joe Perches]

On Tue, 2014-03-18 at 13:55 +0100, Geert Uytterhoeven wrote:
> On Tue, Mar 18, 2014 at 1:45 PM, Joe Perches <joe@perches.com> wrote:

Hi Geert.

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

Fine, but with a correction.

no_printk keeps all side effects like
performing any function calls made by the
statement or accessing any volatiles.

Using

do { if (0) no_printk(fmt, ##__VA_ARGS__); } while (0)

does not have any side-effects.

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

[Geert Uytterhoeven]

Hi Joe,

On Tue, Mar 18, 2014 at 2:07 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-03-18 at 13:55 +0100, Geert Uytterhoeven wrote:
>> On Tue, Mar 18, 2014 at 1:45 PM, Joe Perches <joe@perches.com> wrote:
>> > #define dprintk(flg, fmt, ...) \
>> > do { if (0) pr_debug(fmt, ##__VA_ARGS__); } while (0)
>>
>> Na, no_printk():
>>
>> #define dprintk(flg, fmt, ...) no_printk(fmt, ##__VA_ARGS__)
>
> Fine, but with a correction.
>
> no_printk keeps all side effects like
> performing any function calls made by the
> statement or accessing any volatiles.

That's true...

> Using
>
> do { if (0) no_printk(fmt, ##__VA_ARGS__); } while (0)
>
> does not have any side-effects.

... but all current users in include/ have the side-effects.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Joe Perches]

On Tue, 2014-03-18 at 14:13 +0100, Geert Uytterhoeven wrote:
> > no_printk keeps all side effects like
> > performing any function calls made by the
> > statement or accessing any volatiles.
> That's true...
> > Using
> > do { if (0) no_printk(fmt, ##__VA_ARGS__); } while (0)
> > does not have any side-effects.
> ... but all current users in include/ have the side-effects.

I believe that's intentional, but <shrug>.

I prefer debug statements to have _no_
side effects when not compiled-in/enabled.

Opinions vary.

cheers, Joe

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

[Finn Thain]

On Tue, 18 Mar 2014, Joe Perches wrote:

> On Tue, 2014-03-18 at 13:55 +0100, Geert Uytterhoeven wrote:

> > 
> > #define dprintk(flg, fmt, ...) no_printk(fmt, ##__VA_ARGS__)
> 
> Fine, but with a correction.
> 
> no_printk keeps all side effects like performing any function calls made 
> by the statement or accessing any volatiles.
> 
> Using
> 
> do { if (0) no_printk(fmt, ##__VA_ARGS__); } while (0)
> 
> does not have any side-effects.

I take your point about having the compiler check arguments when
NDEBUG & flg == 0.

As for side-effects, chip register accesses would be affected if dprintk() 
expanded to no_printk() when NDEBUG & flg == 0.

E.g. NCR5380.c line 1213:
dprintk(NDEBUG_INTR, "scsi : unknown interrupt, BASR 0x%X, MR 0x%X, SR 0x%x\n",
                     basr, NCR5380_read(MODE_REG), NCR5380_read(STATUS_REG));

I don't want to re-introduce side-effects into a dozen different NCR5380 
drivers on three different architectures when I can test only one of those 
drivers. It's difficult to get good code coverage even for one driver.

-- 

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

[Joe Perches]

On Wed, 2014-03-19 at 10:14 +1100, Finn Thain wrote:
> As for side-effects, chip register accesses would be affected if dprintk() 
> expanded to no_printk() when NDEBUG & flg == 0.
> 
> E.g. NCR5380.c line 1213:
> dprintk(NDEBUG_INTR, "scsi : unknown interrupt, BASR 0x%X, MR 0x%X, SR 0x%x\n",
>                      basr, NCR5380_read(MODE_REG), NCR5380_read(STATUS_REG));
> 
> I don't want to re-introduce side-effects into a dozen different NCR5380 
> drivers on three different architectures when I can test only one of those 
> drivers. It's difficult to get good code coverage even for one driver.

Hi again Finn.

If dprintk expanded directly to no_printk, then true.

But using "if (0)" prevents the no_printk from
occurring at all so there would be no side-effects
and the format & args would still be verified by the
compiler.

So I believe you shouldn't worry about side-effects.

cheers, Joe

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

[Finn Thain]

On Tue, 18 Mar 2014, Joe Perches wrote:

> But using "if (0)" prevents the no_printk from occurring at all so there 
> would be no side-effects and the format & args would still be verified 
> by the compiler.

I'd prefer this (for symmetry and clarity):

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

But you seem to be asking for this instead:

#if NDEBUG
#define dprintk(flg, fmt, ...) \
        do { if ((NDEBUG) & (flg)) pr_debug(fmt, ## __VA_ARGS__); } while (0)
#else
#define dprintk(flg, fmt, ...) \
        do { if (0) no_printk(fmt, ## __VA_ARGS__); } while (0)
#endif

Why is that better?

-- 

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

[Joe Perches]

On Wed, 2014-03-19 at 12:46 +1100, Finn Thain wrote:
> On Tue, 18 Mar 2014, Joe Perches wrote:
> 
> > But using "if (0)" prevents the no_printk from occurring at all so there 
> > would be no side-effects and the format & args would still be verified 
> > by the compiler.
> 
> I'd prefer this (for symmetry and clarity):
> 
> #if NDEBUG
> #define dprintk(flg, fmt, ...) \
>         do { if ((NDEBUG) & (flg)) pr_debug(fmt, ## __VA_ARGS__); } while (0)
> #else
> #define dprintk(flg, fmt, ...) \
>         do { if (0) pr_debug(fmt, ## __VA_ARGS__); } while (0)
> #endif
> 
> But you seem to be asking for this instead:
> 
> #if NDEBUG
> #define dprintk(flg, fmt, ...) \
>         do { if ((NDEBUG) & (flg)) pr_debug(fmt, ## __VA_ARGS__); } while (0)
> #else
> #define dprintk(flg, fmt, ...) \
>         do { if (0) no_printk(fmt, ## __VA_ARGS__); } while (0)
> #endif
> 
> Why is that better?

It's not to me.

I suggested exactly your first block with if (0) pr_debug...
in the first thing I wrote.

https://lkml.org/lkml/2014/3/18/216

Geert suggested no_printk.

cheers, Joe