[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 01/12] scsi/NCR5380: remove unused BOARD_NORMAL and BOARD_NCR53C400

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-BOARD-macros
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/1fe6b0cf/attachment.ksh>

[PATCH 02/12] scsi/NCR5380: remove redundant HOSTS_C macro tests

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-HOSTS_C-macro
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/fec75727/attachment.ksh>

[PATCH 03/12] scsi/NCR5380: remove old CVS keywords

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-remove-cvs-keywords
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/c7fa994e/attachment.ksh>

[PATCH 04/12] scsi/NCR5380: use NCR5380_dprint() instead of NCR5380_print()

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-sun3-NCR5380_print
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/ba5a5468/attachment.ksh>

[PATCH 05/12] scsi/NCR5380: fix build failures when debugging is enabled

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-build-failures
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/ffbc9e5f/attachment.ksh>

[PATCH 06/12] scsi/NCR5380: fix dprintk macro usage and definition

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-dprintk-params
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/d7ab36b9/attachment.ksh>

[PATCH 07/12] scsi/NCR5380: adopt NCR5380_dprint() and NCR5380_dprint_phase()

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-substitute-NCRPRINT
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/ee78eeed/attachment.ksh>

[PATCH 08/12] scsi/NCR5380: adopt dprintk()

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-substitute-X_PRINTK
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/30865ff5/attachment.ksh>

[PATCH 09/12] scsi/NCR5380: fix and standardize NDEBUG macros

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-fix-NDEBUG
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/0214b636/attachment.ksh>

[PATCH 10/12] scsi/NCR5380: remove unused macro definitions

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-debug-macros-remove-X_PRINTK-NCRPRINT
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/ef907de7/attachment.ksh>

[PATCH 11/12] scsi/NCR5380: reduce depth of sun3_scsi nested includes

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-sun3-includes
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/96fc3260/attachment.ksh>

[PATCH 12/12] scsi/NCR5380: merge sun3_scsi_vme.c into sun3_scsi.c

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-sun3-vme-merge
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/72dfb411/attachment.ksh>

[PATCH 01/12] scsi/NCR5380: remove unused BOARD_NORMAL and BOARD_NCR53C400

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:13AM +1100, Finn Thain wrote:
> BOARD_NORMAL is completely unused and BOARD_NCR53C400 is used only by
> g_NCR5380 internally. Remove the unused definitions.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 02/12] scsi/NCR5380: remove redundant HOSTS_C macro tests

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:14AM +1100, Finn Thain wrote:
> HOSTS_C is always undefined. There is no hosts.c anymore.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 03/12] scsi/NCR5380: remove old CVS keywords

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:15AM +1100, Finn Thain wrote:
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 04/12] scsi/NCR5380: use NCR5380_dprint() instead of NCR5380_print()

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:16AM +1100, Finn Thain wrote:
> Only the NCR5380_dprint() macro should invoke the NCR5380_print() function.
> That's why NCR5380.c only defines the function #if NDEBUG. Use the standard
> macro.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/12] scsi/NCR5380: fix build failures when debugging is enabled

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:17AM +1100, Finn Thain wrote:
> The change from cmd->target to cmd->device->id was apparently the purpose of
> commit a7f251228390e87d86c5e3846f99a455517fdd8e in
> kernel/git/tglx/history.git but some instances have been missed.
> 
> Also fix the "NDEBUG_LAST_WRITE_SENT" and "NDEBUG_ALL" typo's.
> 
> Also fix some format strings (%ul becomes %lu) that caused compiler warnings.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 06/12] scsi/NCR5380: fix dprintk macro usage and definition

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:18AM +1100, Finn Thain wrote:
> There are three implementations of the core NCR5380 driver and three sets
> of debugging macro definitions. And all three implementations use the
> NCR5380.h header as well.
> 
> Two of the definitions of the dprintk macro accept a variable argument list
> whereas the third does not. Standardize on the variable argument list.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 07/12] scsi/NCR5380: adopt NCR5380_dprint() and NCR5380_dprint_phase()

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:19AM +1100, Finn Thain wrote:
> All NCR5380 drivers already include the NCR5380.h header. Better to
> adopt those macros rather than have three variations on them.
> 
> Moreover, the macros in NCR5380.h are preferable anyway: the atari_NCR5380
> and sun3_NCR5380 versions are inflexible. For example, they can't accomodate
> NCR5380_dprint(NDEBUG_MAIN | NDEBUG_QUEUES, ...)
> 
> Replace the NCR_PRINT* macros from atari_NCR5380.h and sun3_NCR5380.h with
> the equivalent macros from NCR5380.h.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 08/12] scsi/NCR5380: adopt dprintk()

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:20AM +1100, Finn Thain wrote:
> All NCR5380 drivers already include the NCR5380.h header. Better to
> adopt those macros rather than have three variations on them.
> 
> Moreover, the macros in NCR5380.h are preferable because the atari_NCR5380
> and sun3_NCR5380 versions are inflexible. For example, they can't accomodate
> dprintk(NDEBUG_MAIN | NDEBUG_QUEUES, ...)
> 
> Replace the *_PRINTK macros from atari_NCR5380.h and sun3_NCR5380.h with
> the equivalent macros from NCR5380.h.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 09/12] scsi/NCR5380: fix and standardize NDEBUG macros

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:21AM +1100, Finn Thain wrote:
> All three NCR5380 core driver implementations share the same NCR5380.h
> header file so they need to agree on certain macro definitions.
> 
> The flag bit used by the NDEBUG_MERGING macro in atari_NCR5380 and
> sun3_NCR5380 collides with the bit used by NDEBUG_LISTS.
> 
> Moreover, NDEBUG_ABORT appears in NCR5380.c so it should be defined in
> NCR5380.h rather than in each of the many drivers using that core.
> 
> An undefined NDEBUG_ABORT macro caused compiler errors and led to dodgy
> workarounds in the core driver that can now be removed.
> (See commits f566a576bca09de85bf477fc0ab2c8c96405b77b and
> 185a7a1cd79b9891e3c17abdb103ba1c98d6ca7a.)
> 
> Move all of the NDEBUG_ABORT, NDEBUG_TAGS and NDEBUG_MERGING macro
> definitions into NCR5380.h where all the other NDEBUG macros live.
> 
> Also, incorrect "#ifdef NDEBUG" becomes "#if NDEBUG" to fix the warning:
> drivers/scsi/mac_scsi.c: At top level:
> drivers/scsi/NCR5380.c:418: warning: 'NCR5380_print' defined but not used
> drivers/scsi/NCR5380.c:459: warning: 'NCR5380_print_phase' defined but not used
> 
> The debugging code is now enabled when NDEBUG != 0.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 10/12] scsi/NCR5380: remove unused macro definitions

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:22AM +1100, Finn Thain wrote:
> Remove the unused (and divergent) debugging macro definitions from
> the sun3_NCR5380 and atari_NCR5380 drivers. These drivers have been
> converted to use the common macros in NCR5380.h.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 11/12] scsi/NCR5380: reduce depth of sun3_scsi nested includes

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:23AM +1100, Finn Thain wrote:
> Move the #include "NCR5380.h" out of the sun3_scsi.h header file and into
> the driver .c files, like all the other NCR5380 drivers in the tree.
> 
> This improves uniformity and reduces the depth of nested includes. The
> sequence of #include's, #define's and #if's no longer does my head in.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 12/12] scsi/NCR5380: merge sun3_scsi_vme.c into sun3_scsi.c

[Christoph Hellwig]

On Tue, Mar 18, 2014 at 11:42:24AM +1100, Finn Thain wrote:
> The sun3 drivers suffer from a whole bunch of duplicated code. Fix this
> by following the g_NCR5380_mmio example. (Notionally, sun3_scsi relates to
> sun3_scsi_vme in the same way that g_NCR5380 relates to g_NCR5380_mmio.)
> 
> Dead code is also removed: we now have working debug macros so
> SUN3_SCSI_DEBUG is undesirable. Dead code within #ifdef OLD_DMA is also
> dropped, consistent with sun3_scsi_vme.c.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/12] scsi/NCR5380: fix build failures when debugging is enabled

[Finn Thain]

On Wed, 28 May 2014, Christoph Hellwig wrote:

> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks for reviewing. There was a later submission (version 2) with 
changes made to three of these patches,

[PATCH v2 05/12] scsi/NCR5380: fix build failures when debugging is enabled
[PATCH v2 06/12] scsi/NCR5380: fix dprintk macro usage and definition
[PATCH v2 09/12] scsi/NCR5380: fix and standardize NDEBUG macros

And several patches in the v2 submission had some acked-by tags added.

The v2 series is the one to merge (but I think James intends to do that).

-- 

[PATCH 05/12] scsi/NCR5380: fix build failures when debugging is enabled

[Christoph Hellwig]

On Wed, May 28, 2014 at 10:09:33PM +1000, Finn Thain wrote:
> [PATCH v2 05/12] scsi/NCR5380: fix build failures when debugging is enabled
> [PATCH v2 06/12] scsi/NCR5380: fix dprintk macro usage and definition
> [PATCH v2 09/12] scsi/NCR5380: fix and standardize NDEBUG macros
> 
> And several patches in the v2 submission had some acked-by tags added.
> 
> The v2 series is the one to merge (but I think James intends to do that).

I've pulled this version into the for-3.16 drivers tree I'm collection
for james:

  http://git.infradead.org/users/hch/scsi-queue.git/shortlog/refs/heads/drivers-for-3.16

  git://git.infradead.org/users/hch/scsi-queue.git drivers-for-3.16

Can you send incremental changes if needed?