Re: [PATCH 7/9] net: phy: use linkmode operation return values in phy_device.c

[Andrew Lunn]

On Tue, Sep 08, 2026 at 11:19:19AM -0400, Yury Norov wrote:
> On Tue, Sep 08, 2026 at 03:52:09PM +0200, Andrew Lunn wrote:
> > > -	linkmode_and(sfp_support, port->supported, caps->link_modes);
> > > -	if (linkmode_empty(sfp_support)) {
> > > +	if (!linkmode_and(sfp_support, port->supported, caps->link_modes)) {
> > >  		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted, no common linkmode\n");
> > 
> > From a readability perspective, i like linkmode_empty(). It is more
> > obvious than !linkmode_and().
> 
> OK, a function returning 2 values is not something obvious in C. What
> about this?
> 
>         
> 	sfp_supported = linkmode_and(sfp_support, port->supported, caps->link_modes);
>         if (!sfp_supported)
> 		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted, no common linkmode\n");

Why not keep it as it is? If the link mode is empty, we know we have a
problem. 


> 
> > None of this code is in the hot path. So we should put readability
> > above performance. 
> > 
> > >  	/* Some PHYs may advertise, by default, not support EEE modes. So,
> > >  	 * we need to clean them. In addition remove all disabled EEE modes.
> > >  	 */
> > > -	linkmode_and(phydev->advertising_eee, phydev->supported_eee,
> > > -		     phydev->advertising_eee);
> > > -	linkmode_andnot(phydev->advertising_eee, phydev->advertising_eee,
> > > -			phydev->eee_disabled_modes);
> > > -
> > >  	/* There is no "enabled" flag. If PHY is advertising, assume it is
> > >  	 * kind of enabled.
> > >  	 */
> > > -	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
> > > +	phydev->eee_cfg.eee_enabled =
> > > +		linkmode_and_andnot(phydev->advertising_eee,
> > > +				    phydev->advertising_eee,
> > > +				    phydev->supported_eee,
> > > +				    phydev->eee_disabled_modes);
> > 
> > So, which is more readable, the original or this?
> 
> The new version is more readable to me. What about this:
>         
>         eee_advertised = linkmode_and_andnot(phydev->advertising_eee,
> 	                                     phydev->advertising_eee,
>                                              phydev->supported_eee,
> 		                             phydev->eee_disabled_modes);

I still prefer the original version.

Where do you stop with these functions? Why not add
linkmode_and_andnot_or()?  linkmode_and_nand_xor_nor()?

If complex expressions were used on the fast path, every packet
needing some bitmap logic, i can see the benefit of such complex
functions, if they can be optimised. But this is slow path, probe
time, or when the link goes up. Human readability comes first.

Just out of interest, did you look at the disassembly for both
versions? Isn't it the inline linkmode_ wrapper function which drops
the return value, which you are adding back. But the
compiler/optimizer sees it and could make use of it? Is gcc/clang
clever enough to do that?

	Andrew