chelsio: Use a more common const struct pci_device_id foo[] style

chelsio: Use a more common const struct pci_device_id foo[] style

[Joe Perches]

Chelsio code shares a pci_device_table from an #include file.
Make the include guard simpler and make the arrays const.

Reduces data by moving tables to text.

Removed unnecessary macros:
o CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
o CH_PCI_DEVICE_ID_TABLE_DEFINE_END
o CH_PCI_ID_TABLE_ENTRY (moved to the .h file)
Added new macro define:
o CH_PCI_ID_TABLE_ENTRY_DATA

  text	   data	    bss	    dec	    hex	filename
  50550	    923	    172	  51645	   c9bd	drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.new
  46935	   4531	    172	  51638	   c9b6	drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.old
  27864	    355	      8	  28227	   6e43	drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.new
  26072	   2203	      8	  28283	   6e7b	drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.old
   9734	    450	     24	  10208	   27e0	drivers/scsi/csiostor/csio_init.o.new
   7942	   2242	     24	  10208	   27e0	drivers/scsi/csiostor/csio_init.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    | 17 ++++------
 drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h | 38 ++++++++--------------
 .../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c    | 10 ++----
 drivers/scsi/csiostor/csio_init.c                  | 11 +++----
 4 files changed, 27 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index a22cf93..8a01eeb 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -123,23 +123,18 @@ struct filter_entry {
 
 /* Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-	static struct pci_device_id cxgb4_pci_tbl[] = {
-#define CH_PCI_DEVICE_ID_FUNCTION 0x4
+
+#define CH_PCI_DEVICE_ID_FUNCTION	0x4
+#define CH_PCI_ID_TABLE_ENTRY_DATA	4
 
 /* Include PCI Device IDs for both PF4 and PF0-3 so our PCI probe() routine is
  * called for both.
  */
-#define CH_PCI_DEVICE_ID_FUNCTION2 0x0
-
-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-		{PCI_VDEVICE(CHELSIO, (devid)), 4}
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END \
-		{ 0, } \
-	}
+#define CH_PCI_DEVICE_ID_FUNCTION2	0x0
 
+static const struct pci_device_id cxgb4_pci_tbl[] = {
 #include "t4_pci_id_tbl.h"
+};
 
 #define FW4_FNAME "cxgb4/t4fw.bin"
 #define FW5_FNAME "cxgb4/t5fw.bin"
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
index ddfb5b8..f648091 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
@@ -39,9 +39,6 @@
  *
  * The macros are:
  *
- * CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
- *   -- Used to start the definition of the PCI ID Table.
- *
  * CH_PCI_DEVICE_ID_FUNCTION
  *   -- The PCI Function Number to use in the PCI Device ID Table.  "0"
  *   -- for drivers attaching to PF0-3, "4" for drivers attaching to PF4,
@@ -51,25 +48,17 @@
  *   -- If defined, create a PCI Device ID Table with both
  *   -- CH_PCI_DEVICE_ID_FUNCTION and CH_PCI_DEVICE_ID_FUNCTION2 populated.
  *
- * CH_PCI_ID_TABLE_ENTRY(DeviceID)
+ * CH_PCI_ID_TABLE_ENTRY_DATA(DeviceID)
  *   -- Used for the individual PCI Device ID entries.  Note that we will
  *   -- be adding a trailing comma (",") after all of the entries (and
  *   -- between the pairs of entries if CH_PCI_DEVICE_ID_FUNCTION2 is defined).
- *
- * CH_PCI_DEVICE_ID_TABLE_DEFINE_END
- *   -- Used to finish the definition of the PCI ID Table.  Note that we
- *   -- will be adding a trailing semi-colon (";") here.
  */
-#ifdef CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
 
 #ifndef CH_PCI_DEVICE_ID_FUNCTION
 #error CH_PCI_DEVICE_ID_FUNCTION not defined!
 #endif
-#ifndef CH_PCI_ID_TABLE_ENTRY
-#error CH_PCI_ID_TABLE_ENTRY not defined!
-#endif
-#ifndef CH_PCI_DEVICE_ID_TABLE_DEFINE_END
-#error CH_PCI_DEVICE_ID_TABLE_DEFINE_END not defined!
+#ifndef CH_PCI_ID_TABLE_ENTRY_DATA
+#error CH_PCI_ID_TABLE_ENTRY_DATA not defined!
 #endif
 
 /* T4 and later ASICs use a PCI Device ID scheme of 0xVFPP where:
@@ -81,19 +70,22 @@
  * We use this consistency in order to create the proper PCI Device IDs
  * for the specified CH_PCI_DEVICE_ID_FUNCTION.
  */
+
+#define CH_PCI_ID_TABLE_ENTRY(devid)					\
+	{ PCI_VDEVICE(CHELSIO, devid), CH_PCI_ID_TABLE_ENTRY_DATA }
+
 #ifndef CH_PCI_DEVICE_ID_FUNCTION2
-#define CH_PCI_ID_TABLE_FENTRY(devid) \
-	CH_PCI_ID_TABLE_ENTRY((devid) | \
+#define CH_PCI_ID_TABLE_FENTRY(devid)					\
+	CH_PCI_ID_TABLE_ENTRY((devid) |					\
 			      ((CH_PCI_DEVICE_ID_FUNCTION) << 8))
 #else
-#define CH_PCI_ID_TABLE_FENTRY(devid) \
-	CH_PCI_ID_TABLE_ENTRY((devid) | \
-			      ((CH_PCI_DEVICE_ID_FUNCTION) << 8)), \
-	CH_PCI_ID_TABLE_ENTRY((devid) | \
+#define CH_PCI_ID_TABLE_FENTRY(devid)					\
+	CH_PCI_ID_TABLE_ENTRY((devid) |					\
+			      ((CH_PCI_DEVICE_ID_FUNCTION) << 8)),	\
+	CH_PCI_ID_TABLE_ENTRY((devid) |					\
 			      ((CH_PCI_DEVICE_ID_FUNCTION2) << 8))
 #endif
 
-CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
 	/* T4 adapters:
 	 */
 	CH_PCI_ID_TABLE_FENTRY(0x4000),	/* T440-dbg */
@@ -154,8 +146,6 @@ CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
 	CH_PCI_ID_TABLE_FENTRY(0x5087),	/* Custom T580-CR */
 	CH_PCI_ID_TABLE_FENTRY(0x5088),	/* Custom T570-CR */
 	CH_PCI_ID_TABLE_FENTRY(0x5089),	/* Custom T520-CR */
-CH_PCI_DEVICE_ID_TABLE_DEFINE_END;
-
-#endif /* CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN */
+	{},
 
 #endif /* __T4_PCI_ID_TBL_H__ */
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index 122e296..7b8b834 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -3033,16 +3033,12 @@ static void cxgb4vf_pci_shutdown(struct pci_dev *pdev)
 
 /* Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-	static struct pci_device_id cxgb4vf_pci_tbl[] = {
 #define CH_PCI_DEVICE_ID_FUNCTION	0x8
+#define CH_PCI_ID_TABLE_ENTRY_DATA	0
 
-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-		{ PCI_VDEVICE(CHELSIO, (devid)), 0 }
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END { 0, } }
-
+static const struct pci_device_id cxgb4vf_pci_tbl[] = {
 #include "../cxgb4/t4_pci_id_tbl.h"
+};
 
 MODULE_DESCRIPTION(DRV_DESC);
 MODULE_AUTHOR("Chelsio Communications");
diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
index d9631e1..0618fbd 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -1171,17 +1171,14 @@ static struct pci_error_handlers csio_err_handler = {
 /*
  *  Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-	static struct pci_device_id csio_pci_tbl[] = {
+
 /* Define for FCoE uses PF6 */
 #define CH_PCI_DEVICE_ID_FUNCTION	0x6
+#define CH_PCI_ID_TABLE_ENTRY_DATA	0
 
-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-		{ PCI_VDEVICE(CHELSIO, (devid)), 0 }
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END { 0, } }
-
+static const struct pci_device_id csio_pci_tbl[] = {
 #include "t4_pci_id_tbl.h"
+};
 
 static struct pci_driver csio_pci_driver = {
 	.name		= KBUILD_MODNAME,

RE: chelsio: Use a more common const struct pci_device_id foo[] style

[Casey Leedom]

  I can't quite tell if this is a patch request being sent to netdev/David Miller or if it's a suggestion sent to Chelsio that you'd like Chelsio to adopt.  I ~think~ it's the latter because the subject doesn't include the standard formatting for a patch request but I'm not 100% familiar with the netdev/kernel.org conventions for this.  My apologies if I'm misinterpreting your message.

1.  The use of "const" certainly seems like a win.

 2. Thanks for catching the redundant use of CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN to guard
    the actual contents of t4_pci_id_tbl.h.  That's already being handled via the check for
    __T4_PCI_ID_TBL_H__ — no idea why I put that in there ...

 3. The use of the CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN and
    CH_PCI_DEVICE_ID_TABLE_DEFINE_END are used to make the t4_pci_id_tbl.h
    accommodate different driver needs.  The header file is only concerned with providing
    a common enumeration of existing PCI Device Identifiers associated with adapters.
    The files including the header are only concerned with providing the necessary context
    for the header file.  The header file ai an OS-independent header file which is
    shared across six existing OS driver implementations; similar to our OS-independent
    register definitions file.

 4. The CH_PCI_ID_TABLE_ENTRY() macro is similarly used to strictly partition
    the roles of t4_pci_id_tbl.h and the files which include it.  t4_pci_id_tbl.h is
    exactly what it's name implies: solely an enumeration of assigned hardware
    adapter PCI Device Identifiers.

 5. Because of the above change in the original abstraction layering, a new macro
    CH_PCI_ID_TABLE_ENTRY_DATA is introduced in this patch which passes in
    a desired value for the "dev" parameter of the PCI_VDEVICE() macro.  But the
    documentation for this new macro in t4_pci_id_tbl.h is incorrectly given the
    documentation of the original CH_PCI_ID_TABLE_ENTRY() macro which
    was originally supplied by the file including t4_pci_id_tbl.h.  This leaves its
    usage confusing for anyone reading the header file.

In conclusion:

 A. I like the use of "const" in the table.

 B. I like removing the redundant content inclusion check of
    CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN.

 C. I'm uncomfortable with all the other changes.

Casey

________________________________________
From: Joe Perches [joe@perches.com]
Sent: Friday, February 13, 2015 6:05 PM
To: Hariprasad S; Casey Leedom; James E.J. Bottomley
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi
Subject: chelsio: Use a more common const struct pci_device_id foo[] style

Chelsio code shares a pci_device_table from an #include file.
Make the include guard simpler and make the arrays const.

Reduces data by moving tables to text.

Removed unnecessary macros:
o CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
o CH_PCI_DEVICE_ID_TABLE_DEFINE_END
o CH_PCI_ID_TABLE_ENTRY (moved to the .h file)
Added new macro define:
o CH_PCI_ID_TABLE_ENTRY_DATA

  text     data     bss     dec     hex filename
  50550     923     172   51645    c9bd drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.new
  46935    4531     172   51638    c9b6 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.old
  27864     355       8   28227    6e43 drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.new
  26072    2203       8   28283    6e7b drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.old
   9734     450      24   10208    27e0 drivers/scsi/csiostor/csio_init.o.new
   7942    2242      24   10208    27e0 drivers/scsi/csiostor/csio_init.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    | 17 ++++------
 drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h | 38 ++++++++--------------
 .../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c    | 10 ++----
 drivers/scsi/csiostor/csio_init.c                  | 11 +++----
 4 files changed, 27 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index a22cf93..8a01eeb 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -123,23 +123,18 @@ struct filter_entry {

 /* Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-       static struct pci_device_id cxgb4_pci_tbl[] = {
-#define CH_PCI_DEVICE_ID_FUNCTION 0x4
+
+#define CH_PCI_DEVICE_ID_FUNCTION      0x4
+#define CH_PCI_ID_TABLE_ENTRY_DATA     4

 /* Include PCI Device IDs for both PF4 and PF0-3 so our PCI probe() routine is
  * called for both.
  */
-#define CH_PCI_DEVICE_ID_FUNCTION2 0x0
-
-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-               {PCI_VDEVICE(CHELSIO, (devid)), 4}
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END \
-               { 0, } \
-       }
+#define CH_PCI_DEVICE_ID_FUNCTION2     0x0

+static const struct pci_device_id cxgb4_pci_tbl[] = {
 #include "t4_pci_id_tbl.h"
+};

 #define FW4_FNAME "cxgb4/t4fw.bin"
 #define FW5_FNAME "cxgb4/t5fw.bin"
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
index ddfb5b8..f648091 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
@@ -39,9 +39,6 @@
  *
  * The macros are:
  *
- * CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
- *   -- Used to start the definition of the PCI ID Table.
- *
  * CH_PCI_DEVICE_ID_FUNCTION
  *   -- The PCI Function Number to use in the PCI Device ID Table.  "0"
  *   -- for drivers attaching to PF0-3, "4" for drivers attaching to PF4,
@@ -51,25 +48,17 @@
  *   -- If defined, create a PCI Device ID Table with both
  *   -- CH_PCI_DEVICE_ID_FUNCTION and CH_PCI_DEVICE_ID_FUNCTION2 populated.
  *
- * CH_PCI_ID_TABLE_ENTRY(DeviceID)
+ * CH_PCI_ID_TABLE_ENTRY_DATA(DeviceID)
  *   -- Used for the individual PCI Device ID entries.  Note that we will
  *   -- be adding a trailing comma (",") after all of the entries (and
  *   -- between the pairs of entries if CH_PCI_DEVICE_ID_FUNCTION2 is defined).
- *
- * CH_PCI_DEVICE_ID_TABLE_DEFINE_END
- *   -- Used to finish the definition of the PCI ID Table.  Note that we
- *   -- will be adding a trailing semi-colon (";") here.
  */
-#ifdef CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN

 #ifndef CH_PCI_DEVICE_ID_FUNCTION
 #error CH_PCI_DEVICE_ID_FUNCTION not defined!
 #endif
-#ifndef CH_PCI_ID_TABLE_ENTRY
-#error CH_PCI_ID_TABLE_ENTRY not defined!
-#endif
-#ifndef CH_PCI_DEVICE_ID_TABLE_DEFINE_END
-#error CH_PCI_DEVICE_ID_TABLE_DEFINE_END not defined!
+#ifndef CH_PCI_ID_TABLE_ENTRY_DATA
+#error CH_PCI_ID_TABLE_ENTRY_DATA not defined!
 #endif

 /* T4 and later ASICs use a PCI Device ID scheme of 0xVFPP where:
@@ -81,19 +70,22 @@
  * We use this consistency in order to create the proper PCI Device IDs
  * for the specified CH_PCI_DEVICE_ID_FUNCTION.
  */
+
+#define CH_PCI_ID_TABLE_ENTRY(devid)                                   \
+       { PCI_VDEVICE(CHELSIO, devid), CH_PCI_ID_TABLE_ENTRY_DATA }
+
 #ifndef CH_PCI_DEVICE_ID_FUNCTION2
-#define CH_PCI_ID_TABLE_FENTRY(devid) \
-       CH_PCI_ID_TABLE_ENTRY((devid) | \
+#define CH_PCI_ID_TABLE_FENTRY(devid)                                  \
+       CH_PCI_ID_TABLE_ENTRY((devid) |                                 \
                              ((CH_PCI_DEVICE_ID_FUNCTION) << 8))
 #else
-#define CH_PCI_ID_TABLE_FENTRY(devid) \
-       CH_PCI_ID_TABLE_ENTRY((devid) | \
-                             ((CH_PCI_DEVICE_ID_FUNCTION) << 8)), \
-       CH_PCI_ID_TABLE_ENTRY((devid) | \
+#define CH_PCI_ID_TABLE_FENTRY(devid)                                  \
+       CH_PCI_ID_TABLE_ENTRY((devid) |                                 \
+                             ((CH_PCI_DEVICE_ID_FUNCTION) << 8)),      \
+       CH_PCI_ID_TABLE_ENTRY((devid) |                                 \
                              ((CH_PCI_DEVICE_ID_FUNCTION2) << 8))
 #endif

-CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
        /* T4 adapters:
         */
        CH_PCI_ID_TABLE_FENTRY(0x4000), /* T440-dbg */
@@ -154,8 +146,6 @@ CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
        CH_PCI_ID_TABLE_FENTRY(0x5087), /* Custom T580-CR */
        CH_PCI_ID_TABLE_FENTRY(0x5088), /* Custom T570-CR */
        CH_PCI_ID_TABLE_FENTRY(0x5089), /* Custom T520-CR */
-CH_PCI_DEVICE_ID_TABLE_DEFINE_END;
-
-#endif /* CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN */
+       {},

 #endif /* __T4_PCI_ID_TBL_H__ */
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index 122e296..7b8b834 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -3033,16 +3033,12 @@ static void cxgb4vf_pci_shutdown(struct pci_dev *pdev)

 /* Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-       static struct pci_device_id cxgb4vf_pci_tbl[] = {
 #define CH_PCI_DEVICE_ID_FUNCTION      0x8
+#define CH_PCI_ID_TABLE_ENTRY_DATA     0

-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-               { PCI_VDEVICE(CHELSIO, (devid)), 0 }
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END { 0, } }
-
+static const struct pci_device_id cxgb4vf_pci_tbl[] = {
 #include "../cxgb4/t4_pci_id_tbl.h"
+};

 MODULE_DESCRIPTION(DRV_DESC);
 MODULE_AUTHOR("Chelsio Communications");
diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
index d9631e1..0618fbd 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -1171,17 +1171,14 @@ static struct pci_error_handlers csio_err_handler = {
 /*
  *  Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-       static struct pci_device_id csio_pci_tbl[] = {
+
 /* Define for FCoE uses PF6 */
 #define CH_PCI_DEVICE_ID_FUNCTION      0x6
+#define CH_PCI_ID_TABLE_ENTRY_DATA     0

-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-               { PCI_VDEVICE(CHELSIO, (devid)), 0 }
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END { 0, } }
-
+static const struct pci_device_id csio_pci_tbl[] = {
 #include "t4_pci_id_tbl.h"
+};

 static struct pci_driver csio_pci_driver = {
        .name           = KBUILD_MODNAME,

Re: chelsio: Use a more common const struct pci_device_id foo[] style

[Joe Perches]

On Mon, 2015-02-16 at 18:05 +0000, Casey Leedom wrote:
>   I can't quite tell if this is a patch request being sent to
> netdev/David Miller or if it's a suggestion sent to Chelsio that you'd
> like Chelsio to adopt.  I ~think~ it's the latter because the subject
> doesn't include the standard formatting for a patch request but I'm
> not 100% familiar with the netdev/kernel.org conventions for this.  My
> apologies if I'm misinterpreting your message.

Hi Casey.

Nah, that's not it.

I just forgot/neglected to prefix [PATCH] on the email when I
sent it.

The patch touches drivers/net/ethernet and drivers/scsi which
generally means that it's better for the company maintainers
to apply rather than coordinating between David and James,
the linux networking and scsi maintainers.  Those guys for
most part don't like touching each others code areas.

> 1.  The use of "const" certainly seems like a win.

I think so.

My goal here was to make obvious the use of
"const struct pci_device_id" which in the original is very
obfuscated/unclear.

>  2. Thanks for catching the redundant use of CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN to guard
>     the actual contents of t4_pci_id_tbl.h.  That's already being handled via the check for
>     __T4_PCI_ID_TBL_H__ — no idea why I put that in there ...

That didn't matter much to me.

>  3. The use of the CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN and
>     CH_PCI_DEVICE_ID_TABLE_DEFINE_END are used to make the t4_pci_id_tbl.h
>     accommodate different driver needs.  The header file is only concerned with providing
>     a common enumeration of existing PCI Device Identifiers associated with adapters.
>     The files including the header are only concerned with providing the necessary context
>     for the header file.  The header file ai an OS-independent header file which is
>     shared across six existing OS driver implementations; similar to our OS-independent
>     register definitions file.

I think that OS independent bit is not useful here.

>  4. The CH_PCI_ID_TABLE_ENTRY() macro is similarly used to strictly partition
>     the roles of t4_pci_id_tbl.h and the files which include it.  t4_pci_id_tbl.h is
>     exactly what it's name implies: solely an enumeration of assigned hardware
>     adapter PCI Device Identifiers.

Which is how it's used both before and after this change.

>  5. Because of the above change in the original abstraction layering, a new macro
>     CH_PCI_ID_TABLE_ENTRY_DATA is introduced in this patch which passes in
>     a desired value for the "dev" parameter of the PCI_VDEVICE() macro.  But the
>     documentation for this new macro in t4_pci_id_tbl.h is incorrectly given the
>     documentation of the original CH_PCI_ID_TABLE_ENTRY() macro which
>     was originally supplied by the file including t4_pci_id_tbl.h.  This leaves its
>     usage confusing for anyone reading the header file.

Do you have any clarifying text to suggest?

cheers, Joe

> In conclusion:
> 
>  A. I like the use of "const" in the table.
> 
>  B. I like removing the redundant content inclusion check of
>     CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN.
> 
>  C. I'm uncomfortable with all the other changes.

> Casey
> 
> ________________________________________
> From: Joe Perches [joe@perches.com]
> Sent: Friday, February 13, 2015 6:05 PM
> To: Hariprasad S; Casey Leedom; James E.J. Bottomley
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi
> Subject: chelsio: Use a more common const struct pci_device_id foo[] style
> 
> Chelsio code shares a pci_device_table from an #include file.
> Make the include guard simpler and make the arrays const.
> 
> Reduces data by moving tables to text.
> 
> Removed unnecessary macros:
> o CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
> o CH_PCI_DEVICE_ID_TABLE_DEFINE_END
> o CH_PCI_ID_TABLE_ENTRY (moved to the .h file)
> Added new macro define:
> o CH_PCI_ID_TABLE_ENTRY_DATA
> 
>   text     data     bss     dec     hex filename
>   50550     923     172   51645    c9bd drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.new
>   46935    4531     172   51638    c9b6 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.old
>   27864     355       8   28227    6e43 drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.new
>   26072    2203       8   28283    6e7b drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.old
>    9734     450      24   10208    27e0 drivers/scsi/csiostor/csio_init.o.new
>    7942    2242      24   10208    27e0 drivers/scsi/csiostor/csio_init.o.old

RE: chelsio: Use a more common const struct pci_device_id foo[] style

[Casey Leedom]

  I understand that OS-independence issues aren't something which are normally accommodated, but as long as definitions don't introduce unnecessary "foreign intrusion" I would hope that it would be okay.  As I noted, our t4_regs.h file is also OS-independent and used by six other OS device drivers.  Putting Linux definitions into t4_pci_id_tbl.h would be somewhat akin to injecting Linux dependencies into t4_regs.h.

  But, if the change must be made, then we'll just maintain a translation between our Common Code and the kernel.org code.  If that's the case, probably the best documentation for the proposed CH_PCI_ID_TABLE_ENTRY_DATA might be something like:

 * CH_PCI_ID_TABLE_ENTRY_DATA
 *   -- Used for the individual PCI Device ID entries for the PCI_VDEVICE() "dev"
 *   -- parameter.

  So it sounds like Chelsio would be required to make this change then?  I'm still unclear on the likes of responsibility/authority here.  We're being told that we must do this but we have to be the ones requesting it?  Sorry for my confusion.  (Which is doubly apparent since I came into work this morning only to realize that it's a company holiday.  Color me a moron today.)

Casey

________________________________________
From: Joe Perches [joe@perches.com]
Sent: Monday, February 16, 2015 10:21 AM
To: Casey Leedom
Cc: Hariprasad S; James E.J. Bottomley; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi; David Miller
Subject: Re: chelsio: Use a more common const struct pci_device_id foo[] style

On Mon, 2015-02-16 at 18:05 +0000, Casey Leedom wrote:
>   I can't quite tell if this is a patch request being sent to
> netdev/David Miller or if it's a suggestion sent to Chelsio that you'd
> like Chelsio to adopt.  I ~think~ it's the latter because the subject
> doesn't include the standard formatting for a patch request but I'm
> not 100% familiar with the netdev/kernel.org conventions for this.  My
> apologies if I'm misinterpreting your message.

Hi Casey.

Nah, that's not it.

I just forgot/neglected to prefix [PATCH] on the email when I
sent it.

The patch touches drivers/net/ethernet and drivers/scsi which
generally means that it's better for the company maintainers
to apply rather than coordinating between David and James,
the linux networking and scsi maintainers.  Those guys for
most part don't like touching each others code areas.

> 1.  The use of "const" certainly seems like a win.

I think so.

My goal here was to make obvious the use of
"const struct pci_device_id" which in the original is very
obfuscated/unclear.

>  2. Thanks for catching the redundant use of CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN to guard
>     the actual contents of t4_pci_id_tbl.h.  That's already being handled via the check for
>     __T4_PCI_ID_TBL_H__ — no idea why I put that in there ...

That didn't matter much to me.

>  3. The use of the CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN and
>     CH_PCI_DEVICE_ID_TABLE_DEFINE_END are used to make the t4_pci_id_tbl.h
>     accommodate different driver needs.  The header file is only concerned with providing
>     a common enumeration of existing PCI Device Identifiers associated with adapters.
>     The files including the header are only concerned with providing the necessary context
>     for the header file.  The header file ai an OS-independent header file which is
>     shared across six existing OS driver implementations; similar to our OS-independent
>     register definitions file.

I think that OS independent bit is not useful here.

>  4. The CH_PCI_ID_TABLE_ENTRY() macro is similarly used to strictly partition
>     the roles of t4_pci_id_tbl.h and the files which include it.  t4_pci_id_tbl.h is
>     exactly what it's name implies: solely an enumeration of assigned hardware
>     adapter PCI Device Identifiers.

Which is how it's used both before and after this change.

>  5. Because of the above change in the original abstraction layering, a new macro
>     CH_PCI_ID_TABLE_ENTRY_DATA is introduced in this patch which passes in
>     a desired value for the "dev" parameter of the PCI_VDEVICE() macro.  But the
>     documentation for this new macro in t4_pci_id_tbl.h is incorrectly given the
>     documentation of the original CH_PCI_ID_TABLE_ENTRY() macro which
>     was originally supplied by the file including t4_pci_id_tbl.h.  This leaves its
>     usage confusing for anyone reading the header file.

Do you have any clarifying text to suggest?

cheers, Joe

> In conclusion:
>
>  A. I like the use of "const" in the table.
>
>  B. I like removing the redundant content inclusion check of
>     CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN.
>
>  C. I'm uncomfortable with all the other changes.

> Casey
>
> ________________________________________
> From: Joe Perches [joe@perches.com]
> Sent: Friday, February 13, 2015 6:05 PM
> To: Hariprasad S; Casey Leedom; James E.J. Bottomley
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi
> Subject: chelsio: Use a more common const struct pci_device_id foo[] style
>
> Chelsio code shares a pci_device_table from an #include file.
> Make the include guard simpler and make the arrays const.
>
> Reduces data by moving tables to text.
>
> Removed unnecessary macros:
> o CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
> o CH_PCI_DEVICE_ID_TABLE_DEFINE_END
> o CH_PCI_ID_TABLE_ENTRY (moved to the .h file)
> Added new macro define:
> o CH_PCI_ID_TABLE_ENTRY_DATA
>
>   text     data     bss     dec     hex filename
>   50550     923     172   51645    c9bd drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.new
>   46935    4531     172   51638    c9b6 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.old
>   27864     355       8   28227    6e43 drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.new
>   26072    2203       8   28283    6e7b drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.old
>    9734     450      24   10208    27e0 drivers/scsi/csiostor/csio_init.o.new
>    7942    2242      24   10208    27e0 drivers/scsi/csiostor/csio_init.o.old

Re: chelsio: Use a more common const struct pci_device_id foo[] style

[Joe Perches]

On Mon, 2015-02-16 at 19:07 +0000, Casey Leedom wrote:
>   I understand that OS-independence issues aren't something which are
> normally accommodated, but as long as definitions don't introduce
> unnecessary "foreign intrusion" I would hope that it would be okay.
> As I noted, our t4_regs.h file is also OS-independent and used by six
> other OS device drivers.  Putting Linux definitions into
> t4_pci_id_tbl.h would be somewhat akin to injecting Linux dependencies
> into t4_regs.h.
> 
>   But, if the change must be made, then we'll just maintain a
> translation between our Common Code and the kernel.org code.  If
> that's the case, probably the best documentation for the proposed
> CH_PCI_ID_TABLE_ENTRY_DATA might be something like:
> 
>  * CH_PCI_ID_TABLE_ENTRY_DATA
>  *   -- Used for the individual PCI Device ID entries for the
> PCI_VDEVICE() "dev"
>  *   -- parameter.
> 
>   So it sounds like Chelsio would be required to make this change
> then?  I'm still unclear on the likes of responsibility/authority
> here.  We're being told that we must do this but we have to be the
> ones requesting it?  Sorry for my confusion.

It's just a suggested patch.
It's your code, you don't _have_ to do anything.

> (Which is doubly apparent since I came into work this morning only to
> realize that it's a company holiday.  Color me a moron today.)

Yay for holidays.

It can be personally beneficial to take the day off.
It could also be very productive to work with no distractions.

As always, your choice...

cheers, Joe

RE: chelsio: Use a more common const struct pci_device_id foo[] style

[Casey Leedom]

  Okay, thanks for your patience with my lack of understanding.  I'll work with Hariprasad tomorrow to get a revised patch out with the "const" change and eliminate the redundant check of CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN to guard the file contents.  Thanks for caching these improvements!

  And given that it's pretty empty at work here on this holiday and how beautiful it is outside, I think it's time to head home to enjoy lunch outside!

Casey

________________________________________
From: Joe Perches [joe@perches.com]
Sent: Monday, February 16, 2015 11:18 AM
To: Casey Leedom
Cc: Hariprasad S; James E.J. Bottomley; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi; David Miller
Subject: Re: chelsio: Use a more common const struct pci_device_id foo[] style

On Mon, 2015-02-16 at 19:07 +0000, Casey Leedom wrote:
>   I understand that OS-independence issues aren't something which are
> normally accommodated, but as long as definitions don't introduce
> unnecessary "foreign intrusion" I would hope that it would be okay.
> As I noted, our t4_regs.h file is also OS-independent and used by six
> other OS device drivers.  Putting Linux definitions into
> t4_pci_id_tbl.h would be somewhat akin to injecting Linux dependencies
> into t4_regs.h.
>
>   But, if the change must be made, then we'll just maintain a
> translation between our Common Code and the kernel.org code.  If
> that's the case, probably the best documentation for the proposed
> CH_PCI_ID_TABLE_ENTRY_DATA might be something like:
>
>  * CH_PCI_ID_TABLE_ENTRY_DATA
>  *   -- Used for the individual PCI Device ID entries for the
> PCI_VDEVICE() "dev"
>  *   -- parameter.
>
>   So it sounds like Chelsio would be required to make this change
> then?  I'm still unclear on the likes of responsibility/authority
> here.  We're being told that we must do this but we have to be the
> ones requesting it?  Sorry for my confusion.

It's just a suggested patch.
It's your code, you don't _have_ to do anything.

> (Which is doubly apparent since I came into work this morning only to
> realize that it's a company holiday.  Color me a moron today.)

Yay for holidays.

It can be personally beneficial to take the day off.
It could also be very productive to work with no distractions.

As always, your choice...

cheers, Joe