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

[Paul Menzel <pmenzel@molgen.mpg.de>]

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