[PATCH net-next] net: phy: fixed_phy: use genphy_read_abilities to simplify the code

[PATCH net-next] net: phy: fixed_phy: use genphy_read_abilities to simplify the code

[Heiner Kallweit]

Populating phy->supported can be achieved easier by using
genphy_read_abilities().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/fixed_phy.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 2e46b7aa6..32a9b99af 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -18,7 +18,6 @@
 #include <linux/of.h>
 #include <linux/idr.h>
 #include <linux/netdevice.h>
-#include <linux/linkmode.h>
 
 #include "swphy.h"
 
@@ -157,27 +156,7 @@ struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
 	phy->mdio.dev.of_node = np;
 	phy->is_pseudo_fixed_link = true;
 
-	switch (status->speed) {
-	case SPEED_1000:
-		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-				 phy->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-				 phy->supported);
-		fallthrough;
-	case SPEED_100:
-		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-				 phy->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-				 phy->supported);
-		fallthrough;
-	case SPEED_10:
-	default:
-		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
-				 phy->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
-				 phy->supported);
-	}
-
+	genphy_read_abilities(phy);
 	phy_advertise_supported(phy);
 
 	ret = phy_device_register(phy);
-- 
2.51.2

Re: [PATCH net-next] net: phy: fixed_phy: use genphy_read_abilities to simplify the code

[Russell King (Oracle)]

On Mon, Nov 10, 2025 at 10:11:24PM +0100, Heiner Kallweit wrote:
> Populating phy->supported can be achieved easier by using
> genphy_read_abilities().

Are you sure about that?

> -	switch (status->speed) {
> -	case SPEED_1000:
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> -				 phy->supported);
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> -				 phy->supported);
> -		fallthrough;
> -	case SPEED_100:
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> -				 phy->supported);
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> -				 phy->supported);
> -		fallthrough;
> -	case SPEED_10:
> -	default:
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> -				 phy->supported);
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> -				 phy->supported);

This code sets both HD and FD for each speed, and if at 1G it sets
100M and 10M as well, if at 100M, it sets 10M as well.

However, swphy emulation (including what was reported through BMSR
and ESTAT) has only ever indicated one speed and duplex supported
via the normal ability bits in these registers. So, "simplifying"
the code introduces user visible changes. This needs to be mentioned
in the commit message.

The next questions are:
1. does this difference matter?
2. is it a bug fix?
3. is swphy wrong?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: fixed_phy: use genphy_read_abilities to simplify the code

[Heiner Kallweit]

On 11/10/2025 10:57 PM, Russell King (Oracle) wrote:
> On Mon, Nov 10, 2025 at 10:11:24PM +0100, Heiner Kallweit wrote:
>> Populating phy->supported can be achieved easier by using
>> genphy_read_abilities().
> 
> Are you sure about that?
> 
>> -	switch (status->speed) {
>> -	case SPEED_1000:
>> -		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
>> -				 phy->supported);
>> -		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
>> -				 phy->supported);
>> -		fallthrough;
>> -	case SPEED_100:
>> -		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
>> -				 phy->supported);
>> -		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
>> -				 phy->supported);
>> -		fallthrough;
>> -	case SPEED_10:
>> -	default:
>> -		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
>> -				 phy->supported);
>> -		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
>> -				 phy->supported);
> 
> This code sets both HD and FD for each speed, and if at 1G it sets
> 100M and 10M as well, if at 100M, it sets 10M as well.
> 
> However, swphy emulation (including what was reported through BMSR
> and ESTAT) has only ever indicated one speed and duplex supported
> via the normal ability bits in these registers. So, "simplifying"
> the code introduces user visible changes. This needs to be mentioned
> in the commit message.
> 
> The next questions are:
> 1. does this difference matter?
> 2. is it a bug fix?
> 3. is swphy wrong?
> 

Once the fixed PHY is attached to the netdev (and bound to the
genphy driver), genphy_read_abilities() will be called anyway.
With the change we have a consistent state before and after
attaching. The link partner advertisement is always determined
by swphy, therefore the additional modes in the old code
should have no effect.

In general I wonder whether setting supported and advertised
modes is still needed here. It was once needed when DSA used to
call genphy_read_status() during port setup. But since the
switch to phylink this isn't the case any longer.

Setting the supported modes was added with 34b31da486a5
("phy: fixed_phy: Set supported speed in phydev").
adjust_link is only set when attaching the PHY, so I'm not
sure the use case exists (any longer?).

So it may be an alternative to remove setting
supported/advertised modes here.

Re: [PATCH net-next] net: phy: fixed_phy: use genphy_read_abilities to simplify the code

[Heiner Kallweit]

On 11/10/2025 10:11 PM, Heiner Kallweit wrote:
> Populating phy->supported can be achieved easier by using
> genphy_read_abilities().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/fixed_phy.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
> 
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index 2e46b7aa6..32a9b99af 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -18,7 +18,6 @@
>  #include <linux/of.h>
>  #include <linux/idr.h>
>  #include <linux/netdevice.h>
> -#include <linux/linkmode.h>
>  
>  #include "swphy.h"
>  
> @@ -157,27 +156,7 @@ struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
>  	phy->mdio.dev.of_node = np;
>  	phy->is_pseudo_fixed_link = true;
>  
> -	switch (status->speed) {
> -	case SPEED_1000:
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> -				 phy->supported);
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> -				 phy->supported);
> -		fallthrough;
> -	case SPEED_100:
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> -				 phy->supported);
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> -				 phy->supported);
> -		fallthrough;
> -	case SPEED_10:
> -	default:
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> -				 phy->supported);
> -		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> -				 phy->supported);
> -	}
> -
> +	genphy_read_abilities(phy);
>  	phy_advertise_supported(phy);
>  
>  	ret = phy_device_register(phy);

After further checking, this code can be removed altogether.

--
pw-bot: cr