Re: [PATCH net-next 3/3] net: phylink: remove "using_mac_select_pcs"

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Oct 09, 2024 at 03:29:38PM +0300, Vladimir Oltean wrote:
> On Tue, Oct 08, 2024 at 03:41:44PM +0100, Russell King (Oracle) wrote:
> > With DSA's implementation of the mac_select_pcs() method removed, we
> > can now remove the detection of mac_select_pcs() implementation.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 4309317de3d1..8f86599d3d78 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -79,7 +79,6 @@ struct phylink {
> >  	unsigned int pcs_state;
> >  
> >  	bool mac_link_dropped;
> > -	bool using_mac_select_pcs;
> >  
> >  	struct sfp_bus *sfp_bus;
> >  	bool sfp_may_have_phy;
> > @@ -661,12 +660,12 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
> >  	int ret;
> >  
> >  	/* Get the PCS for this interface mode */
> > -	if (pl->using_mac_select_pcs) {
> > +	if (pl->mac_ops->mac_select_pcs) {
> >  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
> >  		if (IS_ERR(pcs))
> >  			return PTR_ERR(pcs);
> >  	} else {
> > -		pcs = pl->pcs;
> > +		pcs = NULL;
> 
> The assignment from the "else" branch could have been folded into the
> variable initialization.
> 
> Also, maybe a word in the commit message would be good about why the
> "pcs = pl->pcs" line became "pcs = NULL". I get the impression that
> these are 2 logical changes in one patch. This second aspect I'm
> highlighting seems to be cleaning up the last remnants of phylink_set_pcs().
> Since all phylink users have been converted to mac_select_pcs(), there's
> no other possible value for "pl->pcs" than NULL if "using_mac_select_pcs"
> is true.

Hmm. Looking at this again, we're getting into quite a mess because of
one of your previous review comments from a number of years back.

You stated that you didn't see the need to support a transition from
having-a-PCS to having-no-PCS. I don't have a link to that discussion.
However, it is why we've ended up with phylink_major_config() having
the extra complexity here, effectively preventing mac_select_pcs()
from being able to remove a PCS that was previously added:

		pcs_changed = pcs && pl->pcs != pcs;

because if mac_select_pcs() returns NULL, it was decided that any
in-use PCS would not be removed. It seems (at least to me) to be a
silly decision now.

However, if mac_select_pcs() in phylink_major_config() returns NULL,
we don't do any validation of the PCS.

So this, today, before these patches, is already an inconsistent mess.

To fix this, I think:

	struct phylink_pcs *pcs = NULL;
...
        if (pl->mac_ops->mac_select_pcs) {
                pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
                if (IS_ERR(pcs))
                        return PTR_ERR(pcs);
	}

	if (!pcs)
		pcs = pl->pcs;

is needed to give consistent behaviour.

Alternatively, we could allow mac_select_pcs() to return NULL, which
would then allow the PCS to be removed.

Let me know if you've changed your mind on what behaviour we should
have, because this affects what I do to sort this out.

However, it is true that if mac_select_pcs() is NULL, since commit
a5081bad2eac ("net: phylink: remove phylink_set_pcs()") there is no
way pl->pcs can be non-NULL, since there is no other way for it to
be set.

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