[Intel-wired-lan] [PATCH net v1] ice: ethtool: advertise 1000M speeds properly

[Intel-wired-lan] [PATCH net v1] ice: ethtool: advertise 1000M speeds properly

[Anatolii Gerasymenko]

In current implementation ice_update_phy_type enables all link modes
for selected speed. This approach doesn't work for 1000M speeds,
because both copper (1000baseT) and optical (1000baseX) standards
cannot be enabled at once.

Add a special treatment for 1000M speeds.

Fixes: 48cb27f2fd18 ("ice: Implement handlers for ethtool PHY/link operations")
Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 39 +++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 1e71b70f0e52..e3080c564432 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -2189,6 +2189,42 @@ ice_setup_autoneg(struct ice_port_info *p, struct ethtool_link_ksettings *ks,
 	return err;
 }
 
+/**
+ * ice_set_phy_type_from_speed - set phy_types based on speeds
+ * and advertised modes
+ * @ks: ethtool link ksettings struct
+ * @phy_type_low: pointer to the lower part of phy_type
+ * @phy_type_high: pointer to the higher part of phy_type
+ * @adv_link_speed: targeted link speeds bitmap
+ */
+static void
+ice_set_phy_type_from_speed(const struct ethtool_link_ksettings *ks,
+			    u64 *phy_type_low, u64 *phy_type_high,
+			    u16 adv_link_speed)
+{
+	/* Handle 1000M speed in a special way because ice_update_phy_type
+	 * enables all link modes, but having mixed copper and optic standards
+	 * is not supported
+	 */
+	adv_link_speed &= ~ICE_AQ_LINK_SPEED_1000MB;
+
+	if (ethtool_link_ksettings_test_link_mode(ks, advertising,
+						  1000baseT_Full))
+		*phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_T |
+				 ICE_PHY_TYPE_LOW_1G_SGMII;
+
+	if (ethtool_link_ksettings_test_link_mode(ks, advertising,
+						  1000baseKX_Full))
+		*phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_KX;
+
+	if (ethtool_link_ksettings_test_link_mode(ks, advertising,
+						  1000baseX_Full))
+		*phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_SX |
+				 ICE_PHY_TYPE_LOW_1000BASE_LX;
+
+	ice_update_phy_type(phy_type_low, phy_type_high, adv_link_speed);
+}
+
 /**
  * ice_set_link_ksettings - Set Speed and Duplex
  * @netdev: network interface device structure
@@ -2320,7 +2356,8 @@ ice_set_link_ksettings(struct net_device *netdev,
 		adv_link_speed = curr_link_speed;
 
 	/* Convert the advertise link speeds to their corresponded PHY_TYPE */
-	ice_update_phy_type(&phy_type_low, &phy_type_high, adv_link_speed);
+	ice_set_phy_type_from_speed(ks, &phy_type_low, &phy_type_high,
+				    adv_link_speed);
 
 	if (!autoneg_changed && adv_link_speed == curr_link_speed) {
 		netdev_info(netdev, "Nothing changed, exiting without setting anything.\n");
-- 
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 net v1] ice: ethtool: advertise 1000M speeds properly

[G, GurucharanX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Anatolii Gerasymenko
> Sent: Tuesday, June 7, 2022 12:26 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Gerasymenko, Anatolii <anatolii.gerasymenko@intel.com>
> Subject: [Intel-wired-lan] [PATCH net v1] ice: ethtool: advertise 1000M
> speeds properly
> 
> In current implementation ice_update_phy_type enables all link modes for
> selected speed. This approach doesn't work for 1000M speeds, because both
> copper (1000baseT) and optical (1000baseX) standards cannot be enabled at
> once.
> 
> Add a special treatment for 1000M speeds.
> 
> Fixes: 48cb27f2fd18 ("ice: Implement handlers for ethtool PHY/link
> operations")
> Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 39 +++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 

Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] ice: ethtool: advertise 1000M speeds properly

[Paul Menzel]

Dear Anatolii,


Thank you for the patch.

Am 07.06.22 um 08:55 schrieb Anatolii Gerasymenko:
> In current implementation ice_update_phy_type enables all link modes
> for selected speed. This approach doesn't work for 1000M speeds,
> because both copper (1000baseT) and optical (1000baseX) standards
> cannot be enabled at once.

Is some error shown? What is the behavior of the system? I wonder why 
this has gone unnoticed for such a long time.

> Add a special treatment for 1000M speeds.

Fix this, by adding the function `ice_set_phy_type_from_speed()` for 
1000M speeds.

> 
> Fixes: 48cb27f2fd18 ("ice: Implement handlers for ethtool PHY/link operations")
> Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 39 +++++++++++++++++++-
>   1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 1e71b70f0e52..e3080c564432 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -2189,6 +2189,42 @@ ice_setup_autoneg(struct ice_port_info *p, struct ethtool_link_ksettings *ks,
>   	return err;
>   }
>   
> +/**
> + * ice_set_phy_type_from_speed - set phy_types based on speeds
> + * and advertised modes
> + * @ks: ethtool link ksettings struct
> + * @phy_type_low: pointer to the lower part of phy_type
> + * @phy_type_high: pointer to the higher part of phy_type
> + * @adv_link_speed: targeted link speeds bitmap
> + */
> +static void
> +ice_set_phy_type_from_speed(const struct ethtool_link_ksettings *ks,
> +			    u64 *phy_type_low, u64 *phy_type_high,
> +			    u16 adv_link_speed)
> +{
> +	/* Handle 1000M speed in a special way because ice_update_phy_type
> +	 * enables all link modes, but having mixed copper and optic standards

opitic*al*?

> +	 * is not supported

Add a dot/period at the end?

> +	 */
> +	adv_link_speed &= ~ICE_AQ_LINK_SPEED_1000MB;
> +
> +	if (ethtool_link_ksettings_test_link_mode(ks, advertising,
> +						  1000baseT_Full))
> +		*phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_T |
> +				 ICE_PHY_TYPE_LOW_1G_SGMII;
> +
> +	if (ethtool_link_ksettings_test_link_mode(ks, advertising,
> +						  1000baseKX_Full))
> +		*phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_KX;
> +
> +	if (ethtool_link_ksettings_test_link_mode(ks, advertising,
> +						  1000baseX_Full))
> +		*phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_SX |
> +				 ICE_PHY_TYPE_LOW_1000BASE_LX;

Make the second and third branch all else-if, and add an error, when all 
there checks fail?

> +
> +	ice_update_phy_type(phy_type_low, phy_type_high, adv_link_speed);
> +}
> +
>   /**
>    * ice_set_link_ksettings - Set Speed and Duplex
>    * @netdev: network interface device structure
> @@ -2320,7 +2356,8 @@ ice_set_link_ksettings(struct net_device *netdev,
>   		adv_link_speed = curr_link_speed;
>   
>   	/* Convert the advertise link speeds to their corresponded PHY_TYPE */
> -	ice_update_phy_type(&phy_type_low, &phy_type_high, adv_link_speed);
> +	ice_set_phy_type_from_speed(ks, &phy_type_low, &phy_type_high,
> +				    adv_link_speed);
>   
>   	if (!autoneg_changed && adv_link_speed == curr_link_speed) {
>   		netdev_info(netdev, "Nothing changed, exiting without setting anything.\n");


Kind regards,

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

Re: [Intel-wired-lan] [PATCH net v1] ice: ethtool: advertise 1000M speeds properly

[Anatolii Gerasymenko]

On 17.06.2022 10:03, Paul Menzel wrote:
> Dear Anatolii,
> 
> 
> Thank you for the patch.
> 
> Am 07.06.22 um 08:55 schrieb Anatolii Gerasymenko:
>> In current implementation ice_update_phy_type enables all link modes
>> for selected speed. This approach doesn't work for 1000M speeds,
>> because both copper (1000baseT) and optical (1000baseX) standards
>> cannot be enabled at once.
> 
> Is some error shown? What is the behavior of the system? I wonder why this has gone unnoticed for such a long time.

No error is shown. The link just doesn't come up. Having both mixed copper and optical standards enabled is not
a proper configuration for the firmware and it cannot establish the link.
I think it went unnoticed for a long time, because it is a rare case to use 100G hardware for 1G mode.

> 
>> Add a special treatment for 1000M speeds.
> 
> Fix this, by adding the function `ice_set_phy_type_from_speed()` for 1000M speeds.

Fixed.

> 
>>
>> Fixes: 48cb27f2fd18 ("ice: Implement handlers for ethtool PHY/link operations")
>> Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 39 +++++++++++++++++++-
>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> index 1e71b70f0e52..e3080c564432 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> @@ -2189,6 +2189,42 @@ ice_setup_autoneg(struct ice_port_info *p, struct ethtool_link_ksettings *ks,
>>       return err;
>>   }
>>   +/**
>> + * ice_set_phy_type_from_speed - set phy_types based on speeds
>> + * and advertised modes
>> + * @ks: ethtool link ksettings struct
>> + * @phy_type_low: pointer to the lower part of phy_type
>> + * @phy_type_high: pointer to the higher part of phy_type
>> + * @adv_link_speed: targeted link speeds bitmap
>> + */
>> +static void
>> +ice_set_phy_type_from_speed(const struct ethtool_link_ksettings *ks,
>> +                u64 *phy_type_low, u64 *phy_type_high,
>> +                u16 adv_link_speed)
>> +{
>> +    /* Handle 1000M speed in a special way because ice_update_phy_type
>> +     * enables all link modes, but having mixed copper and optic standards
> 
> opitic*al*?

Good catch. Thanks.

> 
>> +     * is not supported
> 
> Add a dot/period at the end?

Added.

> 
>> +     */
>> +    adv_link_speed &= ~ICE_AQ_LINK_SPEED_1000MB;
>> +
>> +    if (ethtool_link_ksettings_test_link_mode(ks, advertising,
>> +                          1000baseT_Full))
>> +        *phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_T |
>> +                 ICE_PHY_TYPE_LOW_1G_SGMII;
>> +
>> +    if (ethtool_link_ksettings_test_link_mode(ks, advertising,
>> +                          1000baseKX_Full))
>> +        *phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_KX;
>> +
>> +    if (ethtool_link_ksettings_test_link_mode(ks, advertising,
>> +                          1000baseX_Full))
>> +        *phy_type_low |= ICE_PHY_TYPE_LOW_1000BASE_SX |
>> +                 ICE_PHY_TYPE_LOW_1000BASE_LX;
> 
> Make the second and third branch all else-if, and add an error, when all there checks fail?

`advertising` is a bitmask, so different combinations of 1000base* modes can be set by the user.
That's why if-else doesn't fit in here, we should check each mode.

There is no need for an error. This function just maps ethtool modes to the driver's internal modes.
If none of the ethtool 1000base* modes were set, then just none of the driver's internal modes would be
set and firmware will not be configured for 1000M.

> 
>> +
>> +    ice_update_phy_type(phy_type_low, phy_type_high, adv_link_speed);
>> +}
>> +
>>   /**
>>    * ice_set_link_ksettings - Set Speed and Duplex
>>    * @netdev: network interface device structure
>> @@ -2320,7 +2356,8 @@ ice_set_link_ksettings(struct net_device *netdev,
>>           adv_link_speed = curr_link_speed;
>>         /* Convert the advertise link speeds to their corresponded PHY_TYPE */
>> -    ice_update_phy_type(&phy_type_low, &phy_type_high, adv_link_speed);
>> +    ice_set_phy_type_from_speed(ks, &phy_type_low, &phy_type_high,
>> +                    adv_link_speed);
>>         if (!autoneg_changed && adv_link_speed == curr_link_speed) {
>>           netdev_info(netdev, "Nothing changed, exiting without setting anything.\n");
> 
> 
> Kind regards,
> 
> Paul

Thank you,
Anatolii Gerasymenko
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan