Message-ID: <20241011125421.eflqwvpnkrt4pdxh@skbuf>
Date: Fri, 11 Oct 2024 15:54:21 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 3/3] net: phylink: remove "using_mac_select_pcs"

On Fri, Oct 11, 2024 at 11:58:07AM +0100, Russell King (Oracle) wrote:
> On Fri, Oct 11, 2024 at 01:39:12PM +0300, Vladimir Oltean wrote:
> > On Thu, Oct 10, 2024 at 02:00:32PM +0100, Russell King (Oracle) wrote:
> > > On Thu, Oct 10, 2024 at 12:21:43PM +0100, Russell King (Oracle) wrote:
> > > > 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.
> > 
> > Important note: I cannot find any discussion on any mailing list which
> > fills the gap between me asking what is the real world applicability of
> > mac_select_pcs() returning NULL after it has returned non-NULL, and the
> > current phylink behavior, as described above by you. That behavior was
> > first posted here:
> > https://lore.kernel.org/netdev/Ybiue1TPCwsdHmV4@shell.armlinux.org.uk/
> > in patches 1/7 and 2/7. I did not state that phylink should keep the old
> > PCS around, and I do not take responsibility for that.
> 
> I wanted to add support for phylink_set_pcs() to remove the current
> PCS and submitted a patch for it. You didn't see a use case and objected
> to the patch, which wasn't merged.

It was an RFC, it wasn't a candidate for merging anyway.

> I've kept that behaviour ever since on the grounds of your objection -
> as per the link that I posted. This has been carried forward into the
> mac_select_pcs() implementation where it explicitly does not allow a
> PCS to be removed. Pointing to the introduction of mac_select_pcs() is
> later than your objection.

This does not invalidate my previous point in any way. The phylink_set_pcs()
API at that time implied a voluntary call from the driver. "pcs" was not
allowed to be NULL, and your patch was the one introducing phylink_set_pcs(NULL)
as a valid call. That's what I objected to as not seeing the purpose of.

Whereas the new mac_select_pcs() has "return NULL" already baked into it
from day one (the one-to-one equivalent of it being: don't call
phylink_set_pcs()). It becomes inevitable, in this new model, to handle
somehow the "what if" scenario of returning NULL after non-NULL, whereas
that was not necessary before. It's a different API, which automatically
implies a new set of rules.

My point is that it was impossible for me to consciously contribute to
the definition of the mac_select_pcs() API rules. Saying that the
introduction of mac_select_pcs() came later than my comment proves
exactly that. I couldn't have possibly known that my review comment will
be used for a different API than phylink_set_pcs(), because the new API
hadn't been posted.

Whereas what you are saying is that the mac_select_pcs() posting took
place after my comment => of course you took my comment into consideration.
You seem to be omitting that I did not have all information.

> Let me restate it. As a *direct* result of your comments on patch 8/8
> which starts here:
> https://lore.kernel.org/netdev/E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk/
> I took your comments as meaning that you saw no reason why we should
> allow a PCS to ever be removed.

I still stand by that statement, in that context. You took it out of
context.

> phylink_set_pcs() needed to be modified to allow that to happen. Given
> your objection, that behaviour has been carried forward by having
> explicit additional code to ensure that a PCS can't be removed from
> phylink without replacing it with a different PCS. In other words,
> mac_select_pcs() returning NULL when it has previously returned a PCS
> does *nothing* to remove the previous PCS.

The carrying over of old behavior from one API to another API is merely
one of the design choices that can be made, and far from the only
option. In general, you introduce new API exactly _because_ you want to
change the behavior in some conditions.

> Maybe this was not your intention when reviewing patch 8/8, but that's
> how your comments came over, and lead me to the conclusion that we
> need to enforce that - and that is enforced by:
> 
>                 pcs_changed = pcs && pl->pcs != pcs;
> 
> so pcs_change will always be false if pcs == NULL, thus preventing the
> replacement of the pcs:
> 
>         if (pcs_changed) {
>                 phylink_pcs_disable(pl->pcs);
> 
>                 if (pl->pcs)
>                         pl->pcs->phylink = NULL;
> 
>                 pcs->phylink = pl;
> 
>                 pl->pcs = pcs;
>         }
> 
> I wouldn't have coded it this way had you not objected to patch 8/8
> which lead me to believe we should not allow a PCS to be removed.
> 
> Review comments do have implications for future patches... maybe it
> wasn't want you intended, but this is a great example of cause and
> (possibly unintended) effect.

This restatement was not necessary, as I believe I got the point from
your previous message.

I still fundamentally reject any responsibility you wish to attribute to
me here. For one, my review feedback was in a different context. But
let's even assume it was directly related, and I knew mac_select_pcs()
would come. If I were maintaining a piece of code and received some
review feedback, I would not translate it into code that carries
exclusively _my_ sign-off, unless _I_ agree with it and can justify it.
I would not seek to transfer responsibility to somebody else. In fact,
if I were to be held accountable for patches signed off by others, I
wouldn't even be reviewing anything. So I think it's a very fair
collaboration rule, and I'm only asking you to apply it to me as well.

Powered by blists - more mailing lists