[PATCH 0/2] Replace driver's usage of hard-coded device IDs to #defines

[PATCH 0/2] Replace driver's usage of hard-coded device IDs to #defines

[Myron Stowe]

Noa Osherovich introduced a series of new Mellanox device ID definitions to
help differentiate specific controllers that needed INTx masking quirks [1].

Bjorn Helgaas followed on, using the device ID definitions Noa provided to
replace hard-coded values within the mxl4 ID table [2].
    
This patch continues along similar lines, adding a few additional Mellanox
device ID definitions and converting the net/mlx5e driver's mlx5 ID table to
use the defines so tools like 'grep' and 'cscope' can be used to help
identify relationships with other aspects (such as INTx masking).


[1]  7254383341b PCI: Add Mellanox device IDs
     d76d2fe05fd PCI: Convert Mellanox broken INTx quirks to be for listed
                 devices only

[2]  c19e4b9037f net/mlx4_core: Use device ID defines


Myron Stowe (2):
      PCI: Add Mellanox device IDs
      net/mlx5e: Use device ID defines


 drivers/net/ethernet/mellanox/mlx5/core/main.c |   12 ++++++------
 include/linux/pci_ids.h                        |    3 +++
 2 files changed, 9 insertions(+), 6 deletions(-)

--

[PATCH 1/2] PCI: Add Mellanox device IDs

[Myron Stowe]

Add Mellanox device IDs for controllers covered by the mlx5 driver.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---
 include/linux/pci_ids.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 5f6b71d..5626d5a 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2266,6 +2266,9 @@
 #define PCI_DEVICE_ID_MELLANOX_CONNECTIB	0x1011
 #define PCI_DEVICE_ID_MELLANOX_CONNECTX4	0x1013
 #define PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX	0x1015
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX5	0x1017
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX5_EX	0x1019
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX6	0x101b
 #define PCI_DEVICE_ID_MELLANOX_TAVOR		0x5a44
 #define PCI_DEVICE_ID_MELLANOX_TAVOR_BRIDGE	0x5a46
 #define PCI_DEVICE_ID_MELLANOX_SINAI_OLD	0x5e8c

[PATCH 2/2] net/mlx5e: Use device ID defines

[Myron Stowe]

Use Mellanox device ID definitions in the mlx5 ID table so tools such as
'grep' and 'cscope' can be used to help find related aspects.

No functional change intended.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 0c123d5..98642eb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1508,17 +1508,17 @@ static void shutdown(struct pci_dev *pdev)
 }
 
 static const struct pci_device_id mlx5_core_pci_table[] = {
-	{ PCI_VDEVICE(MELLANOX, 0x1011) },			/* Connect-IB */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTIB) },
 	{ PCI_VDEVICE(MELLANOX, 0x1012), MLX5_PCI_DEV_IS_VF},	/* Connect-IB VF */
-	{ PCI_VDEVICE(MELLANOX, 0x1013) },			/* ConnectX-4 */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX4) },
 	{ PCI_VDEVICE(MELLANOX, 0x1014), MLX5_PCI_DEV_IS_VF},	/* ConnectX-4 VF */
-	{ PCI_VDEVICE(MELLANOX, 0x1015) },			/* ConnectX-4LX */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX) },
 	{ PCI_VDEVICE(MELLANOX, 0x1016), MLX5_PCI_DEV_IS_VF},	/* ConnectX-4LX VF */
-	{ PCI_VDEVICE(MELLANOX, 0x1017) },			/* ConnectX-5, PCIe 3.0 */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX5) },
 	{ PCI_VDEVICE(MELLANOX, 0x1018), MLX5_PCI_DEV_IS_VF},	/* ConnectX-5 VF */
-	{ PCI_VDEVICE(MELLANOX, 0x1019) },			/* ConnectX-5 Ex */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX5_EX) },
 	{ PCI_VDEVICE(MELLANOX, 0x101a), MLX5_PCI_DEV_IS_VF},	/* ConnectX-5 Ex VF */
-	{ PCI_VDEVICE(MELLANOX, 0x101b) },			/* ConnectX-6 */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX6) },
 	{ PCI_VDEVICE(MELLANOX, 0x101c), MLX5_PCI_DEV_IS_VF},	/* ConnectX-6 VF */
 	{ 0, }
 };

Re: [PATCH 0/2] Replace driver's usage of hard-coded device IDs to #defines

[David Miller]

From: Myron Stowe <myron.stowe@redhat.com>
Date: Wed, 24 May 2017 16:47:34 -0600

> Noa Osherovich introduced a series of new Mellanox device ID definitions to
> help differentiate specific controllers that needed INTx masking quirks [1].
> 
> Bjorn Helgaas followed on, using the device ID definitions Noa provided to
> replace hard-coded values within the mxl4 ID table [2].
>     
> This patch continues along similar lines, adding a few additional Mellanox
> device ID definitions and converting the net/mlx5e driver's mlx5 ID table to
> use the defines so tools like 'grep' and 'cscope' can be used to help
> identify relationships with other aspects (such as INTx masking).

If you're adding pci_ids.h defines, it's only valid to do so if you
actually use the defines in more than one location.

This patch series is not doing that.

Re: [PATCH 0/2] Replace driver's usage of hard-coded device IDs to #defines

[Myron Stowe]

On Wed, 24 May 2017 20:02:49 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Myron Stowe <myron.stowe@redhat.com>
> Date: Wed, 24 May 2017 16:47:34 -0600
> 
> > Noa Osherovich introduced a series of new Mellanox device ID
> > definitions to help differentiate specific controllers that needed
> > INTx masking quirks [1].
> > 
> > Bjorn Helgaas followed on, using the device ID definitions Noa
> > provided to replace hard-coded values within the mxl4 ID table [2].
> >     
> > This patch continues along similar lines, adding a few additional
> > Mellanox device ID definitions and converting the net/mlx5e
> > driver's mlx5 ID table to use the defines so tools like 'grep' and
> > 'cscope' can be used to help identify relationships with other
> > aspects (such as INTx masking).  
> 
> If you're adding pci_ids.h defines, it's only valid to do so if you
> actually use the defines in more than one location.
> 
> This patch series is not doing that.

Hi David,

Yes, now that you mention that again I do vaguely remember past
conversations stating similar constraints which is a little odd as
Noa's series did exactly that.  It was Bjorn, in a separate patch, that
made the connection to the driver with commit c19e4b9037f
("net/mlx4_core: Use device ID defines") [1] and even after such, some
of the introduced #defines are still currently singular in usage.

Anyway, the part I'm interested in is creating a more transparent
association between the Mellanox controllers that need the INTx masking
quirk and their drivers, something that remains very opaque currently
for a few of the remaining instances (PCI_DEVICE_ID_MELLANOX_CONNECTIB,
PCI_DEVICE_ID_MELLANOX_CONNECTX4, and
PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX).

I'd like to hear back from others as to whether or not there is truly
concern about adding the #defines such as I submitted with singular
usages and if so I can re-submit a more focused patch which would
effectively be the first three substitutions in "[PATCH 2/2] net/mlx5e:
Use device ID defines".


[1] Perhaps Noa's submission had a similar discussion as while it was
a separate series from which Bjorn then made the connection to the
driver, all the patches came in via the same merge commit 25831571419
("Merge branch 'pci/virtualization' into next").


 Thanks for your
feedback,
 Myron

Re: [PATCH 0/2] Replace driver's usage of hard-coded device IDs to #defines

[Bjorn Helgaas]

On Thu, May 25, 2017 at 09:56:55AM -0600, Myron Stowe wrote:
> On Wed, 24 May 2017 20:02:49 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Myron Stowe <myron.stowe@redhat.com>
> > Date: Wed, 24 May 2017 16:47:34 -0600
> > 
> > > Noa Osherovich introduced a series of new Mellanox device ID
> > > definitions to help differentiate specific controllers that needed
> > > INTx masking quirks [1].
> > > 
> > > Bjorn Helgaas followed on, using the device ID definitions Noa
> > > provided to replace hard-coded values within the mxl4 ID table [2].
> > >     
> > > This patch continues along similar lines, adding a few additional
> > > Mellanox device ID definitions and converting the net/mlx5e
> > > driver's mlx5 ID table to use the defines so tools like 'grep' and
> > > 'cscope' can be used to help identify relationships with other
> > > aspects (such as INTx masking).  
> > 
> > If you're adding pci_ids.h defines, it's only valid to do so if you
> > actually use the defines in more than one location.
> > 
> > This patch series is not doing that.
> 
> Hi David,
> 
> Yes, now that you mention that again I do vaguely remember past
> conversations stating similar constraints which is a little odd as
> Noa's series did exactly that.  It was Bjorn, in a separate patch, that
> made the connection to the driver with commit c19e4b9037f
> ("net/mlx4_core: Use device ID defines") [1] and even after such, some
> of the introduced #defines are still currently singular in usage.
> 
> Anyway, the part I'm interested in is creating a more transparent
> association between the Mellanox controllers that need the INTx masking
> quirk and their drivers, something that remains very opaque currently
> for a few of the remaining instances (PCI_DEVICE_ID_MELLANOX_CONNECTIB,
> PCI_DEVICE_ID_MELLANOX_CONNECTX4, and
> PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX).

I think what you want is the patch below (your patch 2, after removing
CONNECTX5, CONNECTX5_EX, and CONNECTX6 since they're only used in one
place).

We added definitions for CONNECTIB, CONNECTX4, and CONNECTX4_LX and uses of
them in a quirk via:

  7254383341bc ("PCI: Add Mellanox device IDs")
  d76d2fe05fd9 ("PCI: Convert Mellanox broken INTx quirks to be for listed
  devices only")

But somehow we missed using those in mlx5/core/main.c.

The patch below doesn't touch PCI, so it would be just for netdev.

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 0c123d571b4c..8a4e292f26b8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1508,11 +1508,11 @@ static void shutdown(struct pci_dev *pdev)
 }
 
 static const struct pci_device_id mlx5_core_pci_table[] = {
-	{ PCI_VDEVICE(MELLANOX, 0x1011) },			/* Connect-IB */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTIB) },
 	{ PCI_VDEVICE(MELLANOX, 0x1012), MLX5_PCI_DEV_IS_VF},	/* Connect-IB VF */
-	{ PCI_VDEVICE(MELLANOX, 0x1013) },			/* ConnectX-4 */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX4) },
 	{ PCI_VDEVICE(MELLANOX, 0x1014), MLX5_PCI_DEV_IS_VF},	/* ConnectX-4 VF */
-	{ PCI_VDEVICE(MELLANOX, 0x1015) },			/* ConnectX-4LX */
+	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX) },
 	{ PCI_VDEVICE(MELLANOX, 0x1016), MLX5_PCI_DEV_IS_VF},	/* ConnectX-4LX VF */
 	{ PCI_VDEVICE(MELLANOX, 0x1017) },			/* ConnectX-5, PCIe 3.0 */
 	{ PCI_VDEVICE(MELLANOX, 0x1018), MLX5_PCI_DEV_IS_VF},	/* ConnectX-5 VF */

Re: [PATCH 0/2] Replace driver's usage of hard-coded device IDs to #defines

[Myron Stowe]

On Fri, Jun 16, 2017 at 2:08 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, May 25, 2017 at 09:56:55AM -0600, Myron Stowe wrote:
>> On Wed, 24 May 2017 20:02:49 -0400 (EDT)
>> David Miller <davem@davemloft.net> wrote:
>>
>> > From: Myron Stowe <myron.stowe@redhat.com>
>> > Date: Wed, 24 May 2017 16:47:34 -0600
>> >
>> > > Noa Osherovich introduced a series of new Mellanox device ID
>> > > definitions to help differentiate specific controllers that needed
>> > > INTx masking quirks [1].
>> > >
>> > > Bjorn Helgaas followed on, using the device ID definitions Noa
>> > > provided to replace hard-coded values within the mxl4 ID table [2].
>> > >
>> > > This patch continues along similar lines, adding a few additional
>> > > Mellanox device ID definitions and converting the net/mlx5e
>> > > driver's mlx5 ID table to use the defines so tools like 'grep' and
>> > > 'cscope' can be used to help identify relationships with other
>> > > aspects (such as INTx masking).
>> >
>> > If you're adding pci_ids.h defines, it's only valid to do so if you
>> > actually use the defines in more than one location.
>> >
>> > This patch series is not doing that.
>>
>> Hi David,
>>
>> Yes, now that you mention that again I do vaguely remember past
>> conversations stating similar constraints which is a little odd as
>> Noa's series did exactly that.  It was Bjorn, in a separate patch, that
>> made the connection to the driver with commit c19e4b9037f
>> ("net/mlx4_core: Use device ID defines") [1] and even after such, some
>> of the introduced #defines are still currently singular in usage.
>>
>> Anyway, the part I'm interested in is creating a more transparent
>> association between the Mellanox controllers that need the INTx masking
>> quirk and their drivers, something that remains very opaque currently
>> for a few of the remaining instances (PCI_DEVICE_ID_MELLANOX_CONNECTIB,
>> PCI_DEVICE_ID_MELLANOX_CONNECTX4, and
>> PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX).
>
> I think what you want is the patch below (your patch 2, after removing
> CONNECTX5, CONNECTX5_EX, and CONNECTX6 since they're only used in one
> place).
>
> We added definitions for CONNECTIB, CONNECTX4, and CONNECTX4_LX and uses of
> them in a quirk via:
>
>   7254383341bc ("PCI: Add Mellanox device IDs")
>   d76d2fe05fd9 ("PCI: Convert Mellanox broken INTx quirks to be for listed
>   devices only")
>
> But somehow we missed using those in mlx5/core/main.c.

Yes, that's the downside to not adding pci_ids.h defines originally
within the driver when its written (even if they are only used once),
it ends up putting the burden on subsequent patch generators to notice
the association and clean things up after the fact.  :)

>
> The patch below doesn't touch PCI, so it would be just for netdev.

Yes, without the pci_ids.h #defines, the resulting patch just ends up
being for netdev.  That's fine, it was the association between the
quirk and the driver, wanting to make such more transparent, that was
the inpetus for this.

>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 0c123d571b4c..8a4e292f26b8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -1508,11 +1508,11 @@ static void shutdown(struct pci_dev *pdev)
>  }
>
>  static const struct pci_device_id mlx5_core_pci_table[] = {
> -       { PCI_VDEVICE(MELLANOX, 0x1011) },                      /* Connect-IB */
> +       { PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTIB) },
>         { PCI_VDEVICE(MELLANOX, 0x1012), MLX5_PCI_DEV_IS_VF},   /* Connect-IB VF */
> -       { PCI_VDEVICE(MELLANOX, 0x1013) },                      /* ConnectX-4 */
> +       { PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX4) },
>         { PCI_VDEVICE(MELLANOX, 0x1014), MLX5_PCI_DEV_IS_VF},   /* ConnectX-4 VF */
> -       { PCI_VDEVICE(MELLANOX, 0x1015) },                      /* ConnectX-4LX */
> +       { PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX) },
>         { PCI_VDEVICE(MELLANOX, 0x1016), MLX5_PCI_DEV_IS_VF},   /* ConnectX-4LX VF */
>         { PCI_VDEVICE(MELLANOX, 0x1017) },                      /* ConnectX-5, PCIe 3.0 */
>         { PCI_VDEVICE(MELLANOX, 0x1018), MLX5_PCI_DEV_IS_VF},   /* ConnectX-5 VF */