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

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

On Thu, Oct 10, 2024 at 12:21:43PM +0100, Russell King (Oracle) wrote:
> 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.

Here's a link to the original discussion from November 2021:

https://lore.kernel.org/all/E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk/

Google uselessly refused to find it, so I searched my own mailboxes
to find the message ID.

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