Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking

[Kevin Traynor <ktraynor@redhat.com>]

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) |
>>>
>>
>