diff mbox series

[RFC,net-next,8/8] net: phylink: allow PCS to be removed

Message ID E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series net: phylink: introduce legacy mode flag | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: linux@armlinux.org.uk
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 30 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Russell King (Oracle) Nov. 23, 2021, 10 a.m. UTC
Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Vladimir Oltean Nov. 23, 2021, 12:08 p.m. UTC | #1
On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
> Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
> the PCS from phylink. This is only supported on non-legacy drivers
> where doing so will have no effect on the mac_config() calling
> behaviour.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a935655c39c0..9f0f0e0aad55 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
>   * in mac_prepare() or mac_config() methods if it is desired to dynamically
>   * change the PCS.
>   *
> - * Please note that there are behavioural changes with the mac_config()
> - * callback if a PCS is present (denoting a newer setup) so removing a PCS
> - * is not supported, and if a PCS is going to be used, it must be registered
> - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
> + * Please note that for legacy phylink users, there are behavioural changes
> + * with the mac_config() callback if a PCS is present (denoting a newer setup)
> + * so removing a PCS is not supported. If a PCS is going to be used, it must
> + * be registered by calling phylink_set_pcs() at the latest in the first
> + * mac_config() call.
> + *
> + * For modern drivers, this may be called with a NULL pcs argument to
> + * disconnect the PCS from phylink.
>   */
>  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
>  {
> +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
> +		phylink_warn(pl,
> +			     "Removing PCS is not supported in a legacy driver");
> +		return;
> +	}
> +
>  	pl->pcs = pcs;
> -	pl->pcs_ops = pcs->ops;
> +	pl->pcs_ops = pcs ? pcs->ops : NULL;
>  }
>  EXPORT_SYMBOL_GPL(phylink_set_pcs);
>  
> -- 
> 2.30.2
> 

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?
Russell King (Oracle) Nov. 23, 2021, 4:08 p.m. UTC | #2
On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
> On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
> > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
> > the PCS from phylink. This is only supported on non-legacy drivers
> > where doing so will have no effect on the mac_config() calling
> > behaviour.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index a935655c39c0..9f0f0e0aad55 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
> >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
> >   * change the PCS.
> >   *
> > - * Please note that there are behavioural changes with the mac_config()
> > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
> > - * is not supported, and if a PCS is going to be used, it must be registered
> > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
> > + * Please note that for legacy phylink users, there are behavioural changes
> > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
> > + * so removing a PCS is not supported. If a PCS is going to be used, it must
> > + * be registered by calling phylink_set_pcs() at the latest in the first
> > + * mac_config() call.
> > + *
> > + * For modern drivers, this may be called with a NULL pcs argument to
> > + * disconnect the PCS from phylink.
> >   */
> >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> >  {
> > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
> > +		phylink_warn(pl,
> > +			     "Removing PCS is not supported in a legacy driver");
> > +		return;
> > +	}
> > +
> >  	pl->pcs = pcs;
> > -	pl->pcs_ops = pcs->ops;
> > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
> >  }
> >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
> >  
> > -- 
> > 2.30.2
> > 
> 
> I've read the discussion at
> https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
> and I still am not sure that I understand what is the use case behind
> removing a PCS?

Passing that to Sean to answer in detail...
Sean Anderson Nov. 23, 2021, 5:30 p.m. UTC | #3
On 11/23/21 11:08 AM, Russell King (Oracle) wrote:
> On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
>> On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
>> > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
>> > the PCS from phylink. This is only supported on non-legacy drivers
>> > where doing so will have no effect on the mac_config() calling
>> > behaviour.
>> >
>> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>> > ---
>> >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
>> >  1 file changed, 15 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> > index a935655c39c0..9f0f0e0aad55 100644
>> > --- a/drivers/net/phy/phylink.c
>> > +++ b/drivers/net/phy/phylink.c
>> > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
>> >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
>> >   * change the PCS.
>> >   *
>> > - * Please note that there are behavioural changes with the mac_config()
>> > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
>> > - * is not supported, and if a PCS is going to be used, it must be registered
>> > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
>> > + * Please note that for legacy phylink users, there are behavioural changes
>> > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
>> > + * so removing a PCS is not supported. If a PCS is going to be used, it must
>> > + * be registered by calling phylink_set_pcs() at the latest in the first
>> > + * mac_config() call.
>> > + *
>> > + * For modern drivers, this may be called with a NULL pcs argument to
>> > + * disconnect the PCS from phylink.
>> >   */
>> >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
>> >  {
>> > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
>> > +		phylink_warn(pl,
>> > +			     "Removing PCS is not supported in a legacy driver");
>> > +		return;
>> > +	}
>> > +
>> >  	pl->pcs = pcs;
>> > -	pl->pcs_ops = pcs->ops;
>> > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
>> >  }
>> >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
>> >
>> > --
>> > 2.30.2
>> >
>>
>> I've read the discussion at
>> https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
>> and I still am not sure that I understand what is the use case behind
>> removing a PCS?
>
> Passing that to Sean to answer in detail...

My original feedback was regarding selecting the correct PCS to use. In
response to the question "What PCS do you want to use for this phy
interface mode?" a valid response is "I don't need a PCS," even if for a
different mode a valid response might be "Please use X PCS." Because
this function is used in validate(), it is necessary to evaluate
"what-if" scenarios, even if a scenario requiring a PCS and one
requiring no PCS would never actually be configured.

Typically the PCS is physically attached to the next layer in the link,
even if the hardware could be configured not to use the PCS. So it does
not usually make sense to configure a link to use modes both requiring a
PCS and requiring no PCS. However, it is possible that such a system
could exist. Most systems should use `phy-mode` to restrict the phy
interfaces modes to whatever makes sense for the board. I think Marek's
series (and specifically [1]) is an good step in this regard.

--Sean

[1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/
Vladimir Oltean Nov. 23, 2021, 6:15 p.m. UTC | #4
On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:
> On 11/23/21 11:08 AM, Russell King (Oracle) wrote:
> > On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
> > > On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
> > > > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
> > > > the PCS from phylink. This is only supported on non-legacy drivers
> > > > where doing so will have no effect on the mac_config() calling
> > > > behaviour.
> > > >
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
> > > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index a935655c39c0..9f0f0e0aad55 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
> > > >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
> > > >   * change the PCS.
> > > >   *
> > > > - * Please note that there are behavioural changes with the mac_config()
> > > > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
> > > > - * is not supported, and if a PCS is going to be used, it must be registered
> > > > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
> > > > + * Please note that for legacy phylink users, there are behavioural changes
> > > > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
> > > > + * so removing a PCS is not supported. If a PCS is going to be used, it must
> > > > + * be registered by calling phylink_set_pcs() at the latest in the first
> > > > + * mac_config() call.
> > > > + *
> > > > + * For modern drivers, this may be called with a NULL pcs argument to
> > > > + * disconnect the PCS from phylink.
> > > >   */
> > > >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> > > >  {
> > > > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
> > > > +		phylink_warn(pl,
> > > > +			     "Removing PCS is not supported in a legacy driver");
> > > > +		return;
> > > > +	}
> > > > +
> > > >  	pl->pcs = pcs;
> > > > -	pl->pcs_ops = pcs->ops;
> > > > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
> > > >
> > > > --
> > > > 2.30.2
> > > >
> > > 
> > > I've read the discussion at
> > > https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
> > > and I still am not sure that I understand what is the use case behind
> > > removing a PCS?
> > 
> > Passing that to Sean to answer in detail...
> 
> My original feedback was regarding selecting the correct PCS to use. In
> response to the question "What PCS do you want to use for this phy
> interface mode?" a valid response is "I don't need a PCS," even if for a
> different mode a valid response might be "Please use X PCS."

Yes, but that is not a reason why you'd want to _remove_ one. Just don't
call phylink_set_pcs() in the first place, there you go, no PCS.

> Because this function is used in validate(), it is necessary to
> evaluate "what-if" scenarios, even if a scenario requiring a PCS and
> one requiring no PCS would never actually be configured.

Yes, but on the same port on the same board? The MAC-side PCS is an
integral part of serial Ethernet links, be it modeled as a discrete part
by hardware manufacturers or not. We are effectively talking about a
situation where a serial link would become parallel, or the other way
around. Have you seen such a thing?

> Typically the PCS is physically attached to the next layer in the link,
> even if the hardware could be configured not to use the PCS. So it does
> not usually make sense to configure a link to use modes both requiring a
> PCS and requiring no PCS. However, it is possible that such a system
> could exist. Most systems should use `phy-mode` to restrict the phy
> interfaces modes to whatever makes sense for the board. I think Marek's
> series (and specifically [1]) is an good step in this regard.
> 
> --Sean
> 
> [1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/

Marek's patches are for reconfiguring the SERDES protocol on the same
lanes. But the lanes are still physically there, and you'd need a PCS to
talk to them no matter what you do, they won't magically turn into RGMII.
If you need to switch the MAC PCS you're configuring with another MAC
PCS (within the same hardware block more or less) due to the fact that
the SERDES protocol is changing, that doesn't count as removing the PCS,
does it? Or what are you thinking of when you say PCS? Phylink doesn't
support any other kind of PCS than a MAC-side PCS.
Sean Anderson Nov. 23, 2021, 7:04 p.m. UTC | #5
On 11/23/21 1:15 PM, Vladimir Oltean wrote:
> On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:
>> On 11/23/21 11:08 AM, Russell King (Oracle) wrote:
>> > On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
>> > > On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
>> > > > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
>> > > > the PCS from phylink. This is only supported on non-legacy drivers
>> > > > where doing so will have no effect on the mac_config() calling
>> > > > behaviour.
>> > > >
>> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>> > > > ---
>> > > >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
>> > > >  1 file changed, 15 insertions(+), 5 deletions(-)
>> > > >
>> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> > > > index a935655c39c0..9f0f0e0aad55 100644
>> > > > --- a/drivers/net/phy/phylink.c
>> > > > +++ b/drivers/net/phy/phylink.c
>> > > > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
>> > > >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
>> > > >   * change the PCS.
>> > > >   *
>> > > > - * Please note that there are behavioural changes with the mac_config()
>> > > > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
>> > > > - * is not supported, and if a PCS is going to be used, it must be registered
>> > > > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
>> > > > + * Please note that for legacy phylink users, there are behavioural changes
>> > > > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
>> > > > + * so removing a PCS is not supported. If a PCS is going to be used, it must
>> > > > + * be registered by calling phylink_set_pcs() at the latest in the first
>> > > > + * mac_config() call.
>> > > > + *
>> > > > + * For modern drivers, this may be called with a NULL pcs argument to
>> > > > + * disconnect the PCS from phylink.
>> > > >   */
>> > > >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
>> > > >  {
>> > > > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
>> > > > +		phylink_warn(pl,
>> > > > +			     "Removing PCS is not supported in a legacy driver");
>> > > > +		return;
>> > > > +	}
>> > > > +
>> > > >  	pl->pcs = pcs;
>> > > > -	pl->pcs_ops = pcs->ops;
>> > > > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
>> > > >  }
>> > > >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
>> > > >
>> > > > --
>> > > > 2.30.2
>> > > >
>> > >
>> > > I've read the discussion at
>> > > https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
>> > > and I still am not sure that I understand what is the use case behind
>> > > removing a PCS?
>> >
>> > Passing that to Sean to answer in detail...
>>
>> My original feedback was regarding selecting the correct PCS to use. In
>> response to the question "What PCS do you want to use for this phy
>> interface mode?" a valid response is "I don't need a PCS," even if for a
>> different mode a valid response might be "Please use X PCS."
>
> Yes, but that is not a reason why you'd want to _remove_ one. Just don't
> call phylink_set_pcs() in the first place, there you go, no PCS.

Yeah.

>> Because this function is used in validate(), it is necessary to
>> evaluate "what-if" scenarios, even if a scenario requiring a PCS and
>> one requiring no PCS would never actually be configured.
>
> Yes, but on the same port on the same board? The MAC-side PCS is an
> integral part of serial Ethernet links, be it modeled as a discrete part
> by hardware manufacturers or not. We are effectively talking about a
> situation where a serial link would become parallel, or the other way
> around. Have you seen such a thing?

I have not. It's certainly possible to create (since the serial link
often uses different physical pins from the parallel link). I think
we can cross that bridge if/when we ever come to it.

>> Typically the PCS is physically attached to the next layer in the link,
>> even if the hardware could be configured not to use the PCS. So it does
>> not usually make sense to configure a link to use modes both requiring a
>> PCS and requiring no PCS. However, it is possible that such a system
>> could exist. Most systems should use `phy-mode` to restrict the phy
>> interfaces modes to whatever makes sense for the board. I think Marek's
>> series (and specifically [1]) is an good step in this regard.
>>
>> --Sean
>>
>> [1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/
>
> Marek's patches are for reconfiguring the SERDES protocol on the same
> lanes. But the lanes are still physically there, and you'd need a PCS to
> talk to them no matter what you do, they won't magically turn into RGMII.
> If you need to switch the MAC PCS you're configuring with another MAC
> PCS (within the same hardware block more or less) due to the fact that
> the SERDES protocol is changing, that doesn't count as removing the PCS,
> does it? Or what are you thinking of when you say PCS? Phylink doesn't
> support any other kind of PCS than a MAC-side PCS.

I mean that with that patch applied, phylink will no longer try and
validate modes which aren't supported on a particular board (see
phylink_validate_any). Although, it looks like set_pcs never was called
in the validate path in the first place (looks like I misremembered).

--Sean
Vladimir Oltean Nov. 23, 2021, 7:30 p.m. UTC | #6
On Tue, Nov 23, 2021 at 02:04:03PM -0500, Sean Anderson wrote:
> On 11/23/21 1:15 PM, Vladimir Oltean wrote:
> > On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:
> > > On 11/23/21 11:08 AM, Russell King (Oracle) wrote:
> > > > On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
> > > > > On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
> > > > > > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
> > > > > > the PCS from phylink. This is only supported on non-legacy drivers
> > > > > > where doing so will have no effect on the mac_config() calling
> > > > > > behaviour.
> > > > > >
> > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > > > ---
> > > > > >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
> > > > > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > index a935655c39c0..9f0f0e0aad55 100644
> > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
> > > > > >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
> > > > > >   * change the PCS.
> > > > > >   *
> > > > > > - * Please note that there are behavioural changes with the mac_config()
> > > > > > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
> > > > > > - * is not supported, and if a PCS is going to be used, it must be registered
> > > > > > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
> > > > > > + * Please note that for legacy phylink users, there are behavioural changes
> > > > > > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
> > > > > > + * so removing a PCS is not supported. If a PCS is going to be used, it must
> > > > > > + * be registered by calling phylink_set_pcs() at the latest in the first
> > > > > > + * mac_config() call.
> > > > > > + *
> > > > > > + * For modern drivers, this may be called with a NULL pcs argument to
> > > > > > + * disconnect the PCS from phylink.
> > > > > >   */
> > > > > >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> > > > > >  {
> > > > > > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
> > > > > > +		phylink_warn(pl,
> > > > > > +			     "Removing PCS is not supported in a legacy driver");
> > > > > > +		return;
> > > > > > +	}
> > > > > > +
> > > > > >  	pl->pcs = pcs;
> > > > > > -	pl->pcs_ops = pcs->ops;
> > > > > > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
> > > > > >
> > > > > > --
> > > > > > 2.30.2
> > > > > >
> > > > >
> > > > > I've read the discussion at
> > > > > https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
> > > > > and I still am not sure that I understand what is the use case behind
> > > > > removing a PCS?
> > > >
> > > > Passing that to Sean to answer in detail...
> > > 
> > > My original feedback was regarding selecting the correct PCS to use. In
> > > response to the question "What PCS do you want to use for this phy
> > > interface mode?" a valid response is "I don't need a PCS," even if for a
> > > different mode a valid response might be "Please use X PCS."
> > 
> > Yes, but that is not a reason why you'd want to _remove_ one. Just don't
> > call phylink_set_pcs() in the first place, there you go, no PCS.
> 
> Yeah.
> 
> > > Because this function is used in validate(), it is necessary to
> > > evaluate "what-if" scenarios, even if a scenario requiring a PCS and
> > > one requiring no PCS would never actually be configured.
> > 
> > Yes, but on the same port on the same board? The MAC-side PCS is an
> > integral part of serial Ethernet links, be it modeled as a discrete part
> > by hardware manufacturers or not. We are effectively talking about a
> > situation where a serial link would become parallel, or the other way
> > around. Have you seen such a thing?
> 
> I have not. It's certainly possible to create (since the serial link
> often uses different physical pins from the parallel link). I think
> we can cross that bridge if/when we ever come to it.

So what do you mean, something like a MAC which can be routed dynamically
via pinctrl either to RGMII or to a SERDES lane? I would expect that
would require rather intricate interaction with the net device driver.
Can that be done today from user space, even in principle?

> > > Typically the PCS is physically attached to the next layer in the link,
> > > even if the hardware could be configured not to use the PCS. So it does
> > > not usually make sense to configure a link to use modes both requiring a
> > > PCS and requiring no PCS. However, it is possible that such a system
> > > could exist. Most systems should use `phy-mode` to restrict the phy
> > > interfaces modes to whatever makes sense for the board. I think Marek's
> > > series (and specifically [1]) is an good step in this regard.
> > > 
> > > --Sean
> > > 
> > > [1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/
> > 
> > Marek's patches are for reconfiguring the SERDES protocol on the same
> > lanes. But the lanes are still physically there, and you'd need a PCS to
> > talk to them no matter what you do, they won't magically turn into RGMII.
> > If you need to switch the MAC PCS you're configuring with another MAC
> > PCS (within the same hardware block more or less) due to the fact that
> > the SERDES protocol is changing, that doesn't count as removing the PCS,
> > does it? Or what are you thinking of when you say PCS? Phylink doesn't
> > support any other kind of PCS than a MAC-side PCS.
> 
> I mean that with that patch applied, phylink will no longer try and
> validate modes which aren't supported on a particular board (see
> phylink_validate_any). Although, it looks like set_pcs never was called
> in the validate path in the first place (looks like I misremembered).

Sorry if you feel like I am asking too many questions. I just want to
understand what I'm being asked to review here :)

So going back to the initial question. What use case do these patches
help to make some progress with?

As far as I understand, being able to call phylink_set_pcs(NULL) is
basically the end goal of it. Sorry if I'm misinterpreting, Russell says
in the cover letter that "we now need to allow the PCS to be optional
for modern drivers". I really don't know how to interpret what
"optional" means. Just judging from that phrase, I don't interpret
"optional" as "having the ability to remove it dynamically", but rather
"the same driver can either interact with phylink using a pcs [on some
ports] or without a pcs [on other ports]".  But that deeply confuses me
because that's already supported, and many drivers make use of that
ability already, this is why the pl->pcs_ops checks are there.
Russell King (Oracle) Nov. 23, 2021, 7:50 p.m. UTC | #7
On Tue, Nov 23, 2021 at 09:30:17PM +0200, Vladimir Oltean wrote:
> Sorry if you feel like I am asking too many questions. I just want to
> understand what I'm being asked to review here :)
> 
> So going back to the initial question. What use case do these patches
> help to make some progress with?

If we exclude patch 8, this series:

1) identifies all those drivers that are reliant on the legacy behaviour
   of phylink, which can then be targetted for modernisation - some of
   which may be trivial to do. ag71xx and axienet have turned out to be
   two drivers that can be trivially converted.

2) hopefully stops the legacy use finding its way into new drivers by
   making it easier to spot in review, but hopefully people will realise
   that setting the legacy flag in their driver to use the old hooks is
   something they probably want to avoid.

3) gives consistent phylink behaviour to modern drivers which may or
   may not decide to register a PCS with phylink.

(3) is probably the most important point for any driver that registers
a PCS conditionally. Right now, any driver that does this gets a
slightly different behaviour from phylink as detailed in patch 7.

I would like to remove the legacy code and old .mac_pcs_get_state and
.mac_an_restart callbacks some day...
diff mbox series

Patch

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1196,15 +1196,25 @@  EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);