Re: [Intel-wired-lan] [PATCH iwl-next v5] i40e: add restore default speed when changed PHY doesn't support it

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 10/16/23 08:56, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
>> Sent: Friday, October 13, 2023 2:35 PM
>> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; intel-wired-
>> lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
>> Jagielski, Jedrzej <jedrzej.jagielski@intel.com>
>> Cc: netdev@vger.kernel.org
>> Subject: Re: [PATCH iwl-next v5] i40e: add restore default speed when changed
>> PHY doesn't support it
>>
>> On 10/13/23 13:52, Aleksandr Loktionov wrote:
>>> Currently, there was no link after plugging a different type of PHY
>>> module if user forced previous PHY specific link type/speed before.
>>>
>>> Add reset link speed settings to the default values for PHY module, if
>>> different PHY module is inserted and currently defined user-specified
>>> speed is not compatible with this module.
>>>
>>> Co-developed-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
>>> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
>>> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>> ---
>>> v1->v2 fixed Reviewed-by tags
>>> v2->v3 fixed commit messages and tags
>>> v3->v4 fixed commit message typo
>>> v4->v5 cc to netdev@vger.kernel.org
>>
>> good move!,
>> now you have to focus on the rules more, like those:
>> do not post next version before 24h of prev one;
>>
> Ok
> 
>> more metadata:
>> I would remove the word 'add' from the Subject line; You still need to change
> I don't agree, because it's not a fix it's a new feature implementation => 'add' feature.
> If you have a better idea please suggest a full commit title which fits 72 chars. What I have now it my best.


i40e: support restoring default speed when changed PHY does not

> 
>> author to Radoslaw.
>>
> The patch was seriously modified, so we are co-authors.

sure, my rule of thumb then is to put bigger contributor as main author,
and the other/s as co-authors,
perhaps some people believe that chronologically first should be the main?

I always try to split patch into more
(each of them still being of value when standing alone ofc).

anyway, it's just a comment, not change-request :)

> And I'd better leave my e-mail, which is still alive, on top for better community support.

doesn't matter, your address is there as a Submitter anyway :)

> 
>>> ---
>>> ---
>>>    drivers/net/ethernet/intel/i40e/i40e_main.c | 65 +++++++++++++++++++--
>>>    1 file changed, 61 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> index d0d0218..6829720 100644
>>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> @@ -10076,6 +10076,55 @@ static void i40e_reset_subtask(struct i40e_pf
>> *pf)
>>>    	rtnl_unlock();
>>>    }
>>>
>>> +/**
>>> + * i40e_restore_supported_phy_link_speed - Restore default PHY speed
>>> + * @pf: board private structure
>>> + *
>>> + * Set PHY module speeds according to values got from
>>> + * initial link speed abilites.
>>> + **/
>>> +static int i40e_restore_supported_phy_link_speed(struct i40e_pf *pf)
>>> +{
>>> +	struct i40e_aq_get_phy_abilities_resp abilities;
>>> +	struct i40e_aq_set_phy_config config = {0};
>>
>> just `= {};`
>>
>>> +	struct i40e_hw *hw = &pf->hw;
>>> +	int err;
>>> +
>>> +	err = i40e_aq_get_phy_capabilities(hw, false, false, &abilities, NULL);
>>> +	if (err) {
>>> +		dev_dbg(&pf->pdev->dev, "failed to get phy cap., ret =  %i
>> last_status =  %s\n",
>>> +			err, i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
>>> +		return err;
>>> +	}
>>> +	config.eee_capability = abilities.eee_capability;
>>> +	config.phy_type_ext = abilities.phy_type_ext;
>>> +	config.low_power_ctrl = abilities.d3_lpan;
>>> +	config.abilities = abilities.abilities;
>>> +	config.abilities |= I40E_AQ_PHY_ENABLE_AN;
>>> +	config.phy_type = abilities.phy_type;
>>> +	config.eeer = abilities.eeer_val;
>>> +	config.fec_config = abilities.fec_cfg_curr_mod_ext_info &
>>> +			    I40E_AQ_PHY_FEC_CONFIG_MASK;
>>> +	err = i40e_aq_get_phy_capabilities(hw, false, true, &abilities, NULL);
>>> +	if (err) {
>>> +		dev_dbg(&pf->pdev->dev, "get supported phy types ret =  %i
>>> +last_status =  %s\n",
>>
>> s/  / /g
>>
>> (in English: replace double spaces by single ones)
>>
>>> +			err, i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
>>> +		return err;
>>> +	}
>>> +	config.link_speed = abilities.link_speed;
>>> +
>>> +	err = i40e_aq_set_phy_config(hw, &config, NULL);
>>> +	if (err)
>>> +		return err;
>>> +	err = i40e_aq_set_link_restart_an(hw, true, NULL);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	pf->hw.phy.link_info.requested_speeds = config.link_speed;
>>> +
>>> +	return err;
>>> +}
>>> +
>>>    /**
>>>     * i40e_handle_link_event - Handle link event
>>>     * @pf: board private structure
>>> @@ -10086,6 +10135,7 @@ static void i40e_handle_link_event(struct
>> i40e_pf *pf,
>>>    {
>>>    	struct i40e_aqc_get_link_status *status =
>>>    		(struct i40e_aqc_get_link_status *)&e->desc.params.raw;
>>> +	int err;
>>>
>>>    	/* Do a new status request to re-enable LSE reporting
>>>    	 * and load new status information into the hw struct @@ -10109,10
>>> +10159,17 @@ static void i40e_handle_link_event(struct i40e_pf *pf,
>>>    		    (!(status->an_info & I40E_AQ_QUALIFIED_MODULE)) &&
>>>    		    (!(status->link_info & I40E_AQ_LINK_UP)) &&
>>>    		    (!(pf->flags &
>> I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED))) {
>>> -			dev_err(&pf->pdev->dev,
>>> -				"Rx/Tx is disabled on this device because an
>> unsupported SFP module type was detected.\n");
>>> -			dev_err(&pf->pdev->dev,
>>> -				"Refer to the Intel(R) Ethernet Adapters and
>> Devices User Guide for a list of supported modules.\n");
>>> +			err = i40e_restore_supported_phy_link_speed(pf);
>>> +			if (err) {
>>> +				dev_err(&pf->pdev->dev,
>>> +					"Rx/Tx is disabled on this device
>> because an unsupported SFP module type was detected.\n");
>>> +				dev_err(&pf->pdev->dev,
>>> +					"Refer to the Intel(R) Ethernet
>> Adapters and Devices User Guide
>>> +for a list of supported modules.\n");
>>> +
>>> +				return;
>>> +			}
>>> +
>>> +			dev_info(&pf->pdev->dev, "The selected speed is
>> incompatible with
>>> +the connected media type. Resetting to the default speed setting for
>>> +the media type.");
>>>    		}
>>>    	}
>>>    }
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan