Re: [PATCH net-next v12 09/18] net: phylink: Move sfp interface selection and filtering to phy_caps

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 09/09/2025 à 17:26, Maxime Chevallier a écrit :
> Phylink's helpers to get the interfaces usable on an SFP module based on
> speed and linkmodes can be modes to phy_caps, so that it can benefit to
> PHY-driver SFP support.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/phy/phy-caps.h |  6 ++++
>   drivers/net/phy/phy_caps.c | 72 ++++++++++++++++++++++++++++++++++++++
>   drivers/net/phy/phylink.c  | 72 +++++---------------------------------
>   3 files changed, 86 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
> index ba81cd75e122..ebed340a2e77 100644
> --- a/drivers/net/phy/phy-caps.h
> +++ b/drivers/net/phy/phy-caps.h
> @@ -66,4 +66,10 @@ void phy_caps_medium_get_supported(unsigned long *supported,
>   				   int lanes);
>   u32 phy_caps_mediums_from_linkmodes(unsigned long *linkmodes);
>   
> +void phy_caps_filter_sfp_interfaces(unsigned long *dst,
> +				    const unsigned long *interfaces);
> +phy_interface_t phy_caps_select_sfp_interface_speed(const unsigned long *interfaces,
> +						    u32 speed);
> +phy_interface_t phy_caps_choose_sfp_interface(const unsigned long *interfaces);
> +
>   #endif /* __PHY_CAPS_H */
> diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
> index b38c567ec6ef..e4adc36e0fe3 100644
> --- a/drivers/net/phy/phy_caps.c
> +++ b/drivers/net/phy/phy_caps.c
> @@ -63,6 +63,22 @@ static int speed_duplex_to_capa(int speed, unsigned int duplex)
>   #define for_each_link_caps_desc_speed(cap) \
>   	for (cap = &link_caps[__LINK_CAPA_MAX - 1]; cap >= link_caps; cap--)
>   
> +static const phy_interface_t phy_caps_sfp_interface_preference[] = {
> +	PHY_INTERFACE_MODE_100GBASEP,
> +	PHY_INTERFACE_MODE_50GBASER,
> +	PHY_INTERFACE_MODE_LAUI,
> +	PHY_INTERFACE_MODE_25GBASER,
> +	PHY_INTERFACE_MODE_USXGMII,
> +	PHY_INTERFACE_MODE_10GBASER,
> +	PHY_INTERFACE_MODE_5GBASER,
> +	PHY_INTERFACE_MODE_2500BASEX,
> +	PHY_INTERFACE_MODE_SGMII,
> +	PHY_INTERFACE_MODE_1000BASEX,
> +	PHY_INTERFACE_MODE_100BASEX,
> +};
> +
> +static DECLARE_PHY_INTERFACE_MASK(phy_caps_sfp_interfaces);
> +
>   /**
>    * phy_caps_init() - Initializes the link_caps array from the link_mode_params.
>    *
> @@ -100,6 +116,10 @@ int phy_caps_init(void)
>   		__set_bit(i, link_caps[capa].linkmodes);
>   	}
>   
> +	for (int i = 0; i < ARRAY_SIZE(phy_caps_sfp_interface_preference); ++i)
> +		__set_bit(phy_caps_sfp_interface_preference[i],
> +			  phy_caps_sfp_interfaces);
> +
>   	return 0;
>   }
>   
> @@ -520,3 +540,55 @@ int phy_caps_interface_max_speed(phy_interface_t interface)
>   	return SPEED_UNKNOWN;
>   }
>   EXPORT_SYMBOL_GPL(phy_caps_interface_max_speed);
> +
> +void phy_caps_filter_sfp_interfaces(unsigned long *dst,
> +				    const unsigned long *interfaces)
> +{
> +	phy_interface_and(dst, interfaces, phy_caps_sfp_interfaces);
> +}
> +
> +phy_interface_t
> +phy_caps_select_sfp_interface_speed(const unsigned long *interfaces, u32 speed)
> +{
> +	phy_interface_t best_interface = PHY_INTERFACE_MODE_NA;
> +	phy_interface_t interface;
> +	u32 max_speed;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(phy_caps_sfp_interface_preference); i++) {
> +		interface = phy_caps_sfp_interface_preference[i];
> +		if (!test_bit(interface, interfaces))
> +			continue;
> +
> +		max_speed = phy_caps_interface_max_speed(interface);
> +
> +		/* The logic here is: if speed == max_speed, then we've found
> +		 * the best interface. Otherwise we find the interface that
> +		 * can just support the requested speed.
> +		 */
> +		if (max_speed >= speed)
> +			best_interface = interface;
> +
> +		if (max_speed <= speed)
> +			break;
> +	}
> +
> +	return best_interface;
> +}
> +EXPORT_SYMBOL_GPL(phy_caps_select_sfp_interface_speed);
> +
> +phy_interface_t phy_caps_choose_sfp_interface(const unsigned long *interfaces)
> +{
> +	phy_interface_t interface;
> +	size_t i;
> +
> +	interface = PHY_INTERFACE_MODE_NA;
> +	for (i = 0; i < ARRAY_SIZE(phy_caps_sfp_interface_preference); i++)
> +		if (test_bit(phy_caps_sfp_interface_preference[i], interfaces)) {
> +			interface = phy_caps_sfp_interface_preference[i];
> +			break;
> +		}
> +
> +	return interface;
> +}
> +EXPORT_SYMBOL_GPL(phy_caps_choose_sfp_interface);
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 091b1ee5c49a..91111ea1b149 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -126,22 +126,6 @@ do {									\
>   })
>   #endif
>   
> -static const phy_interface_t phylink_sfp_interface_preference[] = {
> -	PHY_INTERFACE_MODE_100GBASEP,
> -	PHY_INTERFACE_MODE_50GBASER,
> -	PHY_INTERFACE_MODE_LAUI,
> -	PHY_INTERFACE_MODE_25GBASER,
> -	PHY_INTERFACE_MODE_USXGMII,
> -	PHY_INTERFACE_MODE_10GBASER,
> -	PHY_INTERFACE_MODE_5GBASER,
> -	PHY_INTERFACE_MODE_2500BASEX,
> -	PHY_INTERFACE_MODE_SGMII,
> -	PHY_INTERFACE_MODE_1000BASEX,
> -	PHY_INTERFACE_MODE_100BASEX,
> -};
> -
> -static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
> -
>   /**
>    * phylink_set_port_modes() - set the port type modes in the ethtool mask
>    * @mask: ethtool link mode mask
> @@ -1922,8 +1906,7 @@ static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
>   			/* If the PHY is on a SFP, limit the interfaces to
>   			 * those that can be used with a SFP module.
>   			 */
> -			phy_interface_and(interfaces, interfaces,
> -					  phylink_sfp_interfaces);
> +			phy_caps_filter_sfp_interfaces(interfaces, interfaces);
>   
>   			if (phy_interface_empty(interfaces)) {
>   				phylink_err(pl, "SFP PHY's possible interfaces becomes empty\n");
> @@ -2643,34 +2626,16 @@ static phy_interface_t phylink_sfp_select_interface(struct phylink *pl,
>   static phy_interface_t phylink_sfp_select_interface_speed(struct phylink *pl,
>   							  u32 speed)
>   {
> -	phy_interface_t best_interface = PHY_INTERFACE_MODE_NA;
>   	phy_interface_t interface;
> -	u32 max_speed;
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
> -		interface = phylink_sfp_interface_preference[i];
> -		if (!test_bit(interface, pl->sfp_interfaces))
> -			continue;
> -
> -		max_speed = phy_caps_interface_max_speed(interface);
>   
> -		/* The logic here is: if speed == max_speed, then we've found
> -		 * the best interface. Otherwise we find the interface that
> -		 * can just support the requested speed.
> -		 */
> -		if (max_speed >= speed)
> -			best_interface = interface;
> -
> -		if (max_speed <= speed)
> -			break;
> -	}
> +	interface = phy_caps_select_sfp_interface_speed(pl->sfp_interfaces,
> +							speed);
>   
> -	if (best_interface == PHY_INTERFACE_MODE_NA)
> +	if (interface == PHY_INTERFACE_MODE_NA)
>   		phylink_err(pl, "selection of interface failed, speed %u\n",
>   			    speed);
>   
> -	return best_interface;
> +	return interface;
>   }
>   
>   static void phylink_merge_link_mode(unsigned long *dst, const unsigned long *b)
> @@ -3450,17 +3415,7 @@ static void phylink_sfp_detach(void *upstream, struct sfp_bus *bus)
>   static phy_interface_t phylink_choose_sfp_interface(struct phylink *pl,
>   						    const unsigned long *intf)
>   {
> -	phy_interface_t interface;
> -	size_t i;
> -
> -	interface = PHY_INTERFACE_MODE_NA;
> -	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++)
> -		if (test_bit(phylink_sfp_interface_preference[i], intf)) {
> -			interface = phylink_sfp_interface_preference[i];
> -			break;
> -		}
> -
> -	return interface;
> +	return phy_caps_choose_sfp_interface(intf);
>   }
>   
>   static void phylink_sfp_set_config(struct phylink *pl, unsigned long *supported,
> @@ -3737,8 +3692,8 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>   	phy_support_asym_pause(phy);
>   
>   	/* Set the PHY's host supported interfaces */
> -	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
> -			  pl->config->supported_interfaces);
> +	phy_caps_filter_sfp_interfaces(phy->host_interfaces,
> +				       pl->config->supported_interfaces);
>   
>   	/* Do the initial configuration */
>   	return phylink_sfp_config_phy(pl, phy);
> @@ -4166,16 +4121,5 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
>   }
>   EXPORT_SYMBOL_GPL(phylink_mii_c45_pcs_get_state);
>   
> -static int __init phylink_init(void)
> -{
> -	for (int i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); ++i)
> -		__set_bit(phylink_sfp_interface_preference[i],
> -			  phylink_sfp_interfaces);
> -
> -	return 0;
> -}
> -
> -module_init(phylink_init);
> -
>   MODULE_LICENSE("GPL v2");
>   MODULE_DESCRIPTION("phylink models the MAC to optional PHY connection");