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

[Vladimir Oltean <olteanv@gmail.com>]

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.