diff mbox series

[v2] IGC: Remove I225_I_PHY_ID checking

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

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

iotg.dpdk.ref.app@intel.com Aug. 31, 2022, 10:51 p.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_i225.c | 15 ++-------------
 drivers/net/igc/base/igc_phy.c  |  6 ++----
 2 files changed, 4 insertions(+), 17 deletions(-)

Comments

Kevin Traynor Oct. 11, 2022, 11:04 a.m. UTC | #1
On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
> 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_i225.c | 15 ++-------------
>   drivers/net/igc/base/igc_phy.c  |  6 ++----
>   2 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
> index 5f3d535490..af26602afb 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;

The commit log says it is removing a check on the ID, but it does not 
say why these function pointers are being removed.

Why are they removed, were they not needed?

> -		/* 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) |
Mah, Yock Gen Oct. 12, 2022, 7:45 a.m. UTC | #2
-----Original Message-----
From: Kevin Traynor <ktraynor@redhat.com> 
Sent: Tuesday, 11 October, 2022 7:05 PM
To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking

On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
> 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_i225.c | 15 ++-------------
>   drivers/net/igc/base/igc_phy.c  |  6 ++----
>   2 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/igc/base/igc_i225.c 
> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 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;

> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.

> - Why are they removed, were they not needed?


i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.

> -		/* 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) |
Kevin Traynor Oct. 12, 2022, 8:39 a.m. UTC | #3
On 12/10/2022 08:45, Mah, Yock Gen wrote:
> 
> 
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, 11 October, 2022 7:05 PM
> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
> 
> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>> 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_i225.c | 15 ++-------------
>>    drivers/net/igc/base/igc_phy.c  |  6 ++----
>>    2 files changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/igc/base/igc_i225.c
>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 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;
> 
>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
> 
>> - Why are they removed, were they not needed?
> 
> 
> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
> 

Right, that's clear about the vendor ID check. But it's not clear to me 
why the the resulting code like this:

phy->type = igc_phy_i225;

  and not like this:

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;

So it is using dummy null functions instead:
https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61

Do the device registers not need to be set anymore?

>> -		/* 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) |
>
Kevin Traynor Oct. 18, 2022, 12:54 p.m. UTC | #4
On 12/10/2022 09:39, Kevin Traynor wrote:
> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>
>>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, 11 October, 2022 7:05 PM
>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>
>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>> 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_i225.c | 15 ++-------------
>>>     drivers/net/igc/base/igc_phy.c  |  6 ++----
>>>     2 files changed, 4 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 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;
>>
>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>
>>> - Why are they removed, were they not needed?
>>
>>
>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>
> 
> Right, that's clear about the vendor ID check. But it's not clear to me
> why the the resulting code like this:
> 
> phy->type = igc_phy_i225;
> 
>    and not like this:
> 
> 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;
> 
> So it is using dummy null functions instead:
> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
> 
> Do the device registers not need to be set anymore?
> 

For main branch, it would be nice to have an answer to above.

It only adds a small readability benefit by removing some code branches, 
but does change functionality which adds risk, so I don't think it's a 
good candidate for stable branches.

>>> -		/* 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) |
>>
>
Mah, Yock Gen Oct. 18, 2022, 10:45 p.m. UTC | #5
-----Original Message-----
From: Kevin Traynor <ktraynor@redhat.com> 
Sent: Tuesday, 18 October, 2022 8:54 PM
To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking

On 12/10/2022 09:39, Kevin Traynor wrote:
> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>
>>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, 11 October, 2022 7:05 PM
>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z 
>> <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>
>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>> 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_i225.c | 15 ++-------------
>>>     drivers/net/igc/base/igc_phy.c  |  6 ++----
>>>     2 files changed, 4 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/igc/base/igc_i225.c 
>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 
>>> 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;
>>
>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>
>>> - Why are they removed, were they not needed?
>>
>>
>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>
> 
> Right, that's clear about the vendor ID check. But it's not clear to 
> me why the the resulting code like this:
> 
> phy->type = igc_phy_i225;
> 
>    and not like this:
> 
> 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;
> 
> So it is using dummy null functions instead:
> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
> 
> Do the device registers not need to be set anymore?
> 

> For main branch, it would be nice to have an answer to above.

> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches.

This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below:

+++ b/drivers/net/igc/base/igc_phy.c
@@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw)
                                case I210_I_PHY_ID:
                                /* fall-through */
                                case I225_I_PHY_ID:
+                              case I225_IT_PHY_ID:

However, cleaner solution is to remove those unnecessary checking completely as it does in kernel also https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4

>>> -		/* 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) |
>>
>
Kevin Traynor Oct. 19, 2022, 8:34 a.m. UTC | #6
On 18/10/2022 23:45, Mah, Yock Gen wrote:
> 
> 
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, 18 October, 2022 8:54 PM
> To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
> 
> On 12/10/2022 09:39, Kevin Traynor wrote:
>> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>>
>>>
>>> -----Original Message-----
>>> From: Kevin Traynor <ktraynor@redhat.com>
>>> Sent: Tuesday, 11 October, 2022 7:05 PM
>>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z
>>> <qi.z.zhang@intel.com>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>>
>>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>>> 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_i225.c | 15 ++-------------
>>>>      drivers/net/igc/base/igc_phy.c  |  6 ++----
>>>>      2 files changed, 4 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb
>>>> 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;
>>>
>>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>>
>>>> - Why are they removed, were they not needed?
>>>
>>>
>>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>>
>>
>> Right, that's clear about the vendor ID check. But it's not clear to
>> me why the the resulting code like this:
>>
>> phy->type = igc_phy_i225;
>>
>>     and not like this:
>>
>> 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;
>>
>> So it is using dummy null functions instead:
>> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
>>
>> Do the device registers not need to be set anymore?
>>
> 
>> For main branch, it would be nice to have an answer to above.
> 
>> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches.
> 
> This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below:
> 
> +++ b/drivers/net/igc/base/igc_phy.c
> @@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw)
>                                  case I210_I_PHY_ID:
>                                  /* fall-through */
>                                  case I225_I_PHY_ID:
> +                              case I225_IT_PHY_ID:
> 
> However, cleaner solution is to remove those unnecessary checking completely as it does in kernel also https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4
> 

ok, you are correct that it is more than readability. I can apply but 
first it would be good if you can answer the original question and 
explain why the functionality is changed for I225_I_PHY_ID by removing 
the function pointers that set the registers.

>>>> -		/* 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 mbox series

Patch

diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
index 5f3d535490..af26602afb 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) |