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

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

[Aleksandr Loktionov]

In order to avoid no link after plugging a different type PHY module.

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.

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
v1->v2 fixed Reviewed-by tags
---
---
 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};
+	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",
+			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.");
 		}
 	}
 }
-- 
2.25.1

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

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

[Alexander Lobakin]

From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Date: Wed, 11 Oct 2023 11:13:42 +0200

Please add netdev and linux-kernel MLs to CCs when sending the next version.

> In order to avoid no link after plugging a different type PHY module.

The sentence is incomplete, it tells "why", but no "what".

> 
> 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.
> 
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>

How did Radoslaw participate?
If he's the author, he must be in the "From" field as well. If not, his
SoB tells me nothing. Author, co-developer, reviewer, ...?

> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> v1->v2 fixed Reviewed-by tags
> ---
> ---
>  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};

Prefer `= { }` over `= {0}`.

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

If it's an error, why dev_dbg(0), not dev_err()?

You have @hw pointer, but dereference it manually throughout the whole
function :D

> +		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;

Why not in one line?

> +	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;

FIELD_GET()?

> +	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",
> +			err, i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));

(same re dev_err())

> +		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;

	return 0;

> +}
> +
>  /**
>   * 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");

IIRC we don't usually end kernel messages with a dot.

> +			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.");

This should be dev_warn() I guess. At least dev_notice().

>  		}
>  	}
>  }

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

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

[Alexander Lobakin]

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Wed, 11 Oct 2023 11:22:21 +0200

> From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Date: Wed, 11 Oct 2023 11:13:42 +0200
> 
> Please add netdev and linux-kernel MLs to CCs when sending the next version.
> 
>> In order to avoid no link after plugging a different type PHY module.
> 
> The sentence is incomplete, it tells "why", but no "what".
> 
>>
>> 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.
>>
>> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
> 
> How did Radoslaw participate?
> If he's the author, he must be in the "From" field as well. If not, his
> SoB tells me nothing. Author, co-developer, reviewer, ...?

Also, his email address bounces, IOW there's no point in adding this
SoB. If you want to credit his work, use his working email, either
private or dunno, otherwise makes no sense.

> 
>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

[...]

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

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Sent: Wednesday, October 11, 2023 11:25 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Jagielski, Jedrzej <jedrzej.jagielski@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2] i40e: add restore default speed
> when changed PHY doesn't support it
> 
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> Date: Wed, 11 Oct 2023 11:22:21 +0200
> 
> > From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Date: Wed, 11 Oct 2023 11:13:42 +0200
> >
> > Please add netdev and linux-kernel MLs to CCs when sending the next version.
Sure will do it in next version, thank you for the note

> >
> >> In order to avoid no link after plugging a different type PHY module.
> >
> > The sentence is incomplete, it tells "why", but no "what".
Please clarify your suggestion, what is your "what" expectations?

> >
> >>
> >> 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.
> >>
> >> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> >> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
> >
> > How did Radoslaw participate?
> > If he's the author, he must be in the "From" field as well. If not,
> > his SoB tells me nothing. Author, co-developer, reviewer, ...?
> 
> Also, his email address bounces, IOW there's no point in adding this SoB. If you
> want to credit his work, use his working email, either private or dunno,
> otherwise makes no sense.
Can you explain what do you mean by 'IOW'?

Radek is original author of the patch for OOT driver which a had re-work to be accepted for upstream. Now he is no longer works in Intel. I wanted to give him a credit. What do you suggest?

> >
> >> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> [...]
> 
> Thanks,
> Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Lobakin]

From: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
Date: Wed, 11 Oct 2023 12:32:25 +0200

> 
> 
>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
>> Sent: Wednesday, October 11, 2023 11:25 AM
>> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
>> <anthony.l.nguyen@intel.com>; Jagielski, Jedrzej <jedrzej.jagielski@intel.com>
>> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2] i40e: add restore default speed
>> when changed PHY doesn't support it
>>
>> From: Alexander Lobakin <aleksander.lobakin@intel.com>
>> Date: Wed, 11 Oct 2023 11:22:21 +0200
>>
>>> From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>> Date: Wed, 11 Oct 2023 11:13:42 +0200
>>>
>>> Please add netdev and linux-kernel MLs to CCs when sending the next version.
> Sure will do it in next version, thank you for the note
> 
>>>
>>>> In order to avoid no link after plugging a different type PHY module.
>>>
>>> The sentence is incomplete, it tells "why", but no "what".
> Please clarify your suggestion, what is your "what" expectations?

Usually, "in order" is only the first part of a sentence.
Like,

"In order to reply to your message, I need to click "Reply All".".

But you say something like

"In order to reply to your message."

and that's it. And I don't get what you wanted to say here, as the
second part is missing.

> 
>>>
>>>>
>>>> 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.
>>>>
>>>> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>>>> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
>>>
>>> How did Radoslaw participate?
>>> If he's the author, he must be in the "From" field as well. If not,
>>> his SoB tells me nothing. Author, co-developer, reviewer, ...?
>>
>> Also, his email address bounces, IOW there's no point in adding this SoB. If you
>> want to credit his work, use his working email, either private or dunno,
>> otherwise makes no sense.
> Can you explain what do you mean by 'IOW'?

"In Other Words" -- IOW.

> 
> Radek is original author of the patch for OOT driver which a had re-work to be accepted for upstream. Now he is no longer works in Intel. I wanted to give him a credit. What do you suggest?

There's no point in specifying non-working email addresses.
If you want to credit him, pick his personal email or his new work email
or whatever works and allows to contact with him.

The fact that he's the original author implies he should be the author
of the commit as well, but you send it as if you was the author.
`git commit --author='Radoslaw ...'`

> 
>>>
>>>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>
>> [...]
>>
>> Thanks,
>> Olek

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