diff mbox series

IGC: Remove I225_I_PHY_ID checking

Message ID 20220829081436.29945-1-iotg.dpdk.ref.app@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers show
Series IGC: Remove I225_I_PHY_ID checking | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

iotg.dpdk.ref.app@intel.com Aug. 29, 2022, 8:14 a.m. UTC
From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>

i225 devices have only one PHY vendor. There is unnecessary to check _I_PHY_ID during the link establishment
and auto-negotiation process, the checking also caused devices like i225-IT failed. This patch is to remove the
mentioned unnecessary checking.

Cc: stable@dpdk.org
Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
---
 drivers/net/igc/base/igc_api.c  |  1 +
 drivers/net/igc/base/igc_hw.h   |  1 +
 drivers/net/igc/base/igc_i225.c | 15 ++-------------
 drivers/net/igc/base/igc_phy.c  |  6 ++----
 drivers/net/igc/igc_ethdev.c    |  1 +
 5 files changed, 7 insertions(+), 17 deletions(-)

Comments

Zhang, Qi Z Aug. 30, 2022, 11:17 a.m. UTC | #1
> -----Original Message-----
> From: iotg.dpdk.ref.app@intel.com <iotg.dpdk.ref.app@intel.com>
> Sent: Monday, August 29, 2022 4:15 PM
> To: dev@dpdk.org
> Subject: [PATCH] IGC: Remove I225_I_PHY_ID checking
> 
> From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
> 
> i225 devices have only one PHY vendor. There is unnecessary to check
> _I_PHY_ID during the link establishment and auto-negotiation process, the
> checking also caused devices like i225-IT failed. This patch is to remove the
> mentioned unnecessary checking.
> 
> Cc: stable@dpdk.org
> Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>

Is this the expected author name?

> ---
>  drivers/net/igc/base/igc_api.c  |  1 +
>  drivers/net/igc/base/igc_hw.h   |  1 +
>  drivers/net/igc/base/igc_i225.c | 15 ++-------------
> drivers/net/igc/base/igc_phy.c  |  6 ++----
>  drivers/net/igc/igc_ethdev.c    |  1 +
>  5 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/igc/base/igc_api.c b/drivers/net/igc/base/igc_api.c
> index 9b791dc082..c9fc9ed4b0 100644
> --- a/drivers/net/igc/base/igc_api.c
> +++ b/drivers/net/igc/base/igc_api.c
> @@ -886,6 +886,7 @@ s32 igc_set_mac_type(struct igc_hw *hw)
>  	case IGC_DEV_ID_I225_V:
>  	case IGC_DEV_ID_I225_K:
>  	case IGC_DEV_ID_I225_I:
> +	case IGC_DEV_ID_I225_IT:
>  	case IGC_DEV_ID_I220_V:
>  	case IGC_DEV_ID_I225_BLANK_NVM:
>  	case IGC_DEV_ID_I226_K:
> diff --git a/drivers/net/igc/base/igc_hw.h b/drivers/net/igc/base/igc_hw.h
> index 707a1883b4..e919a11c02 100644
> --- a/drivers/net/igc/base/igc_hw.h
> +++ b/drivers/net/igc/base/igc_hw.h
> @@ -164,6 +164,7 @@ struct igc_hw;
>  #define IGC_DEV_ID_I225_V			0x15F3
>  #define IGC_DEV_ID_I225_K			0x3100
>  #define IGC_DEV_ID_I225_I			0x15F8
> +#define IGC_DEV_ID_I225_IT			0x0D9F

The patch's commit log claims to remove something, 
but in code it also add some new device ID, could you clarify why we need this or it should be in a separate patch?
Mah, Yock Gen Aug. 31, 2022, 10:42 p.m. UTC | #2
>-----Original Message-----
>From: Zhang, Qi Z <qi.z.zhang@intel.com> 
>Sent: Tuesday, 30 August, 2022 7:17 PM
>To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; dev@dpdk.org
>Subject: RE: [PATCH] IGC: Remove I225_I_PHY_ID checking

> -----Original Message-----
> From: iotg.dpdk.ref.app@intel.com <iotg.dpdk.ref.app@intel.com>
> Sent: Monday, August 29, 2022 4:15 PM
> To: dev@dpdk.org
> Subject: [PATCH] IGC: Remove I225_I_PHY_ID checking
> 
> From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
> 
> i225 devices have only one PHY vendor. There is unnecessary to check 
> _I_PHY_ID during the link establishment and auto-negotiation process, 
> the checking also caused devices like i225-IT failed. This patch is to 
> remove the mentioned unnecessary checking.
> 
> Cc: stable@dpdk.org
> Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>

>Is this the expected author name?
Yes, this is expected author name, the PDL email contain all developers working on the patch for better support and response. 
> ---
>  drivers/net/igc/base/igc_api.c  |  1 +
>  drivers/net/igc/base/igc_hw.h   |  1 +
>  drivers/net/igc/base/igc_i225.c | 15 ++------------- 
> drivers/net/igc/base/igc_phy.c  |  6 ++----
>  drivers/net/igc/igc_ethdev.c    |  1 +
>  5 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/igc/base/igc_api.c 
> b/drivers/net/igc/base/igc_api.c index 9b791dc082..c9fc9ed4b0 100644
> --- a/drivers/net/igc/base/igc_api.c
> +++ b/drivers/net/igc/base/igc_api.c
> @@ -886,6 +886,7 @@ s32 igc_set_mac_type(struct igc_hw *hw)
>  	case IGC_DEV_ID_I225_V:
>  	case IGC_DEV_ID_I225_K:
>  	case IGC_DEV_ID_I225_I:
> +	case IGC_DEV_ID_I225_IT:
>  	case IGC_DEV_ID_I220_V:
>  	case IGC_DEV_ID_I225_BLANK_NVM:
>  	case IGC_DEV_ID_I226_K:
> diff --git a/drivers/net/igc/base/igc_hw.h 
> b/drivers/net/igc/base/igc_hw.h index 707a1883b4..e919a11c02 100644
> --- a/drivers/net/igc/base/igc_hw.h
> +++ b/drivers/net/igc/base/igc_hw.h
> @@ -164,6 +164,7 @@ struct igc_hw;
>  #define IGC_DEV_ID_I225_V			0x15F3
>  #define IGC_DEV_ID_I225_K			0x3100
>  #define IGC_DEV_ID_I225_I			0x15F8
> +#define IGC_DEV_ID_I225_IT			0x0D9F

>The patch's commit log claims to remove something, but in code it also add some new device ID, could you clarify why we need this or it should be in a separate patch?
The checking issue was firstly discovered because of i225-IT not working on our lab and escalated to DPDK team for support, as informed by DPDK team after the patch sent, that the i225-IT device ID has been patched in DPDK next release, we'll send out v2 patch soon with only removing redundance checking without i225-IT device ID.
David Marchand Sept. 1, 2022, 8:22 a.m. UTC | #3
On Thu, Sep 1, 2022 at 10:08 AM Mah, Yock Gen <yock.gen.mah@intel.com> wrote:
>
> >-----Original Message-----
> >From: Zhang, Qi Z <qi.z.zhang@intel.com>
> >Sent: Tuesday, 30 August, 2022 7:17 PM
> >To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; dev@dpdk.org
> >Subject: RE: [PATCH] IGC: Remove I225_I_PHY_ID checking
>
> > -----Original Message-----
> > From: iotg.dpdk.ref.app@intel.com <iotg.dpdk.ref.app@intel.com>
> > Sent: Monday, August 29, 2022 4:15 PM
> > To: dev@dpdk.org
> > Subject: [PATCH] IGC: Remove I225_I_PHY_ID checking
> >
> > From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
> >
> > i225 devices have only one PHY vendor. There is unnecessary to check
> > _I_PHY_ID during the link establishment and auto-negotiation process,
> > the checking also caused devices like i225-IT failed. This patch is to
> > remove the mentioned unnecessary checking.
> >
> > Cc: stable@dpdk.org
> > Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
>
> >Is this the expected author name?
> Yes, this is expected author name, the PDL email contain all developers working on the patch for better support and response.

This is not acceptable.

https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body

It is clearly stated that:
"""
The signoff must be a real name and not an alias or nickname. More
than one signoff is allowed.
"""

And all developers involved in the making of a patch must follow the
Developer’s Certificate of Origin.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

In its current form, this patch cannot be merged in the DPDK project.


Besides, please register to the @dev mailing list.
David Marchand Sept. 2, 2022, 7:01 a.m. UTC | #4
On Thu, Sep 1, 2022 at 10:22 AM David Marchand
<david.marchand@redhat.com> wrote:
> Besides, please register to the @dev mailing list.

Bis.

Please register to the @dev mailing list.
diff mbox series

Patch

diff --git a/drivers/net/igc/base/igc_api.c b/drivers/net/igc/base/igc_api.c
index 9b791dc082..c9fc9ed4b0 100644
--- a/drivers/net/igc/base/igc_api.c
+++ b/drivers/net/igc/base/igc_api.c
@@ -886,6 +886,7 @@  s32 igc_set_mac_type(struct igc_hw *hw)
 	case IGC_DEV_ID_I225_V:
 	case IGC_DEV_ID_I225_K:
 	case IGC_DEV_ID_I225_I:
+	case IGC_DEV_ID_I225_IT:
 	case IGC_DEV_ID_I220_V:
 	case IGC_DEV_ID_I225_BLANK_NVM:
 	case IGC_DEV_ID_I226_K:
diff --git a/drivers/net/igc/base/igc_hw.h b/drivers/net/igc/base/igc_hw.h
index 707a1883b4..e919a11c02 100644
--- a/drivers/net/igc/base/igc_hw.h
+++ b/drivers/net/igc/base/igc_hw.h
@@ -164,6 +164,7 @@  struct igc_hw;
 #define IGC_DEV_ID_I225_V			0x15F3
 #define IGC_DEV_ID_I225_K			0x3100
 #define IGC_DEV_ID_I225_I			0x15F8
+#define IGC_DEV_ID_I225_IT			0x0D9F
 #define IGC_DEV_ID_I220_V			0x15F7
 #define IGC_DEV_ID_I225_BLANK_NVM		0x15FD
 #define IGC_DEV_ID_I226_K           0x3102
diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
index 5f3d535490..bdc6f74976 100644
--- a/drivers/net/igc/base/igc_i225.c
+++ b/drivers/net/igc/base/igc_i225.c
@@ -173,19 +173,8 @@  static s32 igc_init_phy_params_i225(struct igc_hw *hw)
 	phy->ops.write_reg = igc_write_phy_reg_gpy;
 
 	ret_val = igc_get_phy_id(hw);
-	/* Verify phy id and set remaining function pointers */
-	switch (phy->id) {
-	case I225_I_PHY_ID:
-	case I226_LM_PHY_ID:
-		phy->type		= igc_phy_i225;
-		phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225;
-		phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225;
-		/* TODO - complete with GPY PHY information */
-		break;
-	default:
-		ret_val = -IGC_ERR_PHY;
-		goto out;
-	}
+
+	phy->type		= igc_phy_i225;
 
 out:
 	return ret_val;
diff --git a/drivers/net/igc/base/igc_phy.c b/drivers/net/igc/base/igc_phy.c
index 43bbe69bca..2906bae21a 100644
--- a/drivers/net/igc/base/igc_phy.c
+++ b/drivers/net/igc/base/igc_phy.c
@@ -1474,8 +1474,7 @@  s32 igc_phy_setup_autoneg(struct igc_hw *hw)
 			return ret_val;
 	}
 
-	if ((phy->autoneg_mask & ADVERTISE_2500_FULL) &&
-	    hw->phy.id == I225_I_PHY_ID) {
+	if (phy->autoneg_mask & ADVERTISE_2500_FULL) {
 	/* Read the MULTI GBT AN Control Register - reg 7.32 */
 		ret_val = phy->ops.read_reg(hw, (STANDARD_AN_REG_MASK <<
 					    MMD_DEVADDR_SHIFT) |
@@ -1615,8 +1614,7 @@  s32 igc_phy_setup_autoneg(struct igc_hw *hw)
 		ret_val = phy->ops.write_reg(hw, PHY_1000T_CTRL,
 					     mii_1000t_ctrl_reg);
 
-	if ((phy->autoneg_mask & ADVERTISE_2500_FULL) &&
-	    hw->phy.id == I225_I_PHY_ID)
+	if (phy->autoneg_mask & ADVERTISE_2500_FULL)
 		ret_val = phy->ops.write_reg(hw,
 					     (STANDARD_AN_REG_MASK <<
 					     MMD_DEVADDR_SHIFT) |
diff --git a/drivers/net/igc/igc_ethdev.c b/drivers/net/igc/igc_ethdev.c
index 7f221a5d34..2989b8d488 100644
--- a/drivers/net/igc/igc_ethdev.c
+++ b/drivers/net/igc/igc_ethdev.c
@@ -97,6 +97,7 @@  static const struct rte_pci_id pci_id_igc_map[] = {
 	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_V)  },
 	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_I)  },
 	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_K)  },
+	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_IT)  },
 	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I226_K)  },
 	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I226_LMVP)  },
 	{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I226_LM)  },