[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241012102731.yylcm54ajdy35dud@skbuf>
Date: Sat, 12 Oct 2024 13:27:31 +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 06:51:51PM +0100, Russell King (Oracle) wrote:
> On Fri, Oct 11, 2024 at 03:54:21PM +0300, Vladimir Oltean wrote:
> > On Fri, Oct 11, 2024 at 11:58:07AM +0100, Russell King (Oracle) wrote:
> > > 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.
>
> What does that have to do with it????????????
>
> An idea is put forward (the idea of allowing PCS to be removed.) It's
> put forward as a RFC. It gets shot down. Author then goes away believing
> that there is no desire to allow PCS to be removed. That idea gets
> carried forward into future patches.
>
> _That_ is what exactly happened. I'm not attributing blame for it,
> merely explaining how we got to where we are with this, and how we've
> ended up in the mess we have with PCS able to be used outside of its
> validated set.
>
> You want me to provide more explanation on the patch, but I've
> identified a fundamental error here caused as an effect of a previous
> review comment.
>
> I'm now wondering what to do about it and how to solve this in a way
> that won't cause us to go around another long confrontational discussion
> but it seems that's not possible.
>
> So, do I ignore your review comments and just do what I think is the
> right thing, or do I attempt to discuss it with you? I think, given
> _this_ debacle, I ignore you. I would much rather involve you but it
> seems that's a mistake.
My technical answer was already provided 2 replies ago:
| Keeping in mind that I don't know whether anything has changed since
| 2021 which would make this condition any less theoretical than it was
| back then, I guess if I were maintaining the code involved, I'd choose
| between 2 options (whichever is easiest):
|
| - Imagine a purely theoretical scenario where phylink transitions
| between a state->interface requiring a phylink_pcs, and one not
| requiring a phylink_pcs. I'm not even saying a serial PCS hardware
| block isn't present, just that it isn't modeled as a phylink_pcs
| (for reasons which may be valid or not). Probably the most logical
| thing to do in this scenario is allow the old phylink_pcs to be
| removed, and its ops never to be used for the new state->interface.
|
| - Validate, possibly at phylink_validate_phy() time, that for all
| phy->possible_interfaces, mac_select_pcs() either returns NULL for
| all of them, or non-NULL for all of them. The idea would be to leave
| room for the use case to define itself (and the restriction to be
| lifted whenever necessary), instead of giving a predefined behavior
| for the transition when in reality we have no idea of the use case
| behind it. I don't know whether checking phy->possible_interfaces
| would be sufficient in ensuring that such a transition cannot occur.
I have nothing more to add to this discussion.
Powered by blists - more mailing lists