| Message ID | E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: phylink: introduce legacy mode flag | expand |
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?
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...
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/
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.
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
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.
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 --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);
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(-)