| Message ID | 20221118000124.2754581-3-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Let phylink manage in-band AN for the PHY | expand |
| 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: 411 this patch: 411 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 294 this patch: 294 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 394 this patch: 394 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 93 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 11/17/22 19:01, Vladimir Oltean wrote: > Currently, phylink requires that fwnodes with links to SFP cages have > the 'managed = "in-band-status"' property, and based on this, the > initial pl->cfg_link_an_mode gets set to MLO_AN_INBAND. > > However, some PHYs on SFP modules may have broken in-band autoneg, and > in that case, phylink selects a pl->cur_link_an_mode which is MLO_AN_PHY, > to tell the MAC/PCS side to disable in-band autoneg (link speed/status > will come over the MDIO side channel). > > The check for PHY in-band autoneg capability is currently open-coded > based on a PHY ID comparison against the BCM84881. But the same problem > will also be need to solved in another case, where syncing in-band > autoneg will be desired between the MAC/PCS and an on-board PHY. > So the approach needs to be generalized, and eventually what is done for > the BCM84881 needs to be replaced with a more generic solution. > > Add new API to the PHY device structure which allows it to report what > it supports in terms of in-band autoneg (whether it can operate with it > on, and whether it can operate with it off). The assumption is that > there is a Clause 37 compatible state machine in the PHY's PCS, and it > requires that the autoneg process completes before the lane transitions > to data mode. If we have a mismatch between in-band autoneg modes, the > system side link will be broken. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > v3->v4: > - split the SFP cur_link_an_mode fixup to separate patch (this one) > - s/inband_aneg/an_inband/ to be more in line with phylink terminology > - clearer documentation, added kerneldocs > - don't return -EIO in phy_validate_an_inband(), this breaks with the > Generic PHY driver because the expected return code is a bit mask, not > a negative integer > > drivers/net/phy/phy.c | 25 +++++++++++++++++++++++++ > drivers/net/phy/phylink.c | 20 +++++++++++++++++--- > include/linux/phy.h | 17 +++++++++++++++++ > 3 files changed, 59 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index e5b6cb1a77f9..2abbacf2c7cb 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -733,6 +733,31 @@ static int phy_check_link_status(struct phy_device *phydev) > return 0; > } > > +/** > + * phy_validate_an_inband - validate which in-band autoneg modes are supported > + * @phydev: the phy_device struct > + * @interface: the MAC-side interface type > + * > + * Returns @PHY_AN_INBAND_UNKNOWN if it is unknown what in-band autoneg setting > + * is required for the given PHY mode, or a bit mask of @PHY_AN_INBAND_OFF (if > + * the PHY is able to work with in-band AN turned off) and @PHY_AN_INBAND_ON > + * (if it works with the feature turned on). With the Generic PHY driver, the > + * result will always be @PHY_AN_INBAND_UNKNOWN. > + */ > +int phy_validate_an_inband(struct phy_device *phydev, > + phy_interface_t interface) > +{ > + /* We may be called before phy_attach_direct() force-binds the > + * generic PHY driver to this device. In that case, report an unknown > + * setting rather than -EIO as most other functions do. > + */ > + if (!phydev->drv || !phydev->drv->validate_an_inband) > + return PHY_AN_INBAND_UNKNOWN; > + > + return phydev->drv->validate_an_inband(phydev, interface); > +} > +EXPORT_SYMBOL_GPL(phy_validate_an_inband); > + > /** > * _phy_start_aneg - start auto-negotiation for this PHY device > * @phydev: the phy_device struct > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 9e4b2dfc98d8..40b7e730fb33 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -2936,10 +2936,24 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy) > return -EINVAL; > } > > - if (phylink_phy_no_inband(phy)) > - mode = MLO_AN_PHY; > - else > + /* Select whether to operate in in-band mode or not, based on the > + * capability of the PHY in the current link mode. > + */ > + ret = phy_validate_an_inband(phy, iface); > + if (ret == PHY_AN_INBAND_UNKNOWN) { > + if (phylink_phy_no_inband(phy)) > + mode = MLO_AN_PHY; > + else > + mode = MLO_AN_INBAND; > + > + phylink_dbg(pl, > + "PHY driver does not report in-band autoneg capability, assuming %s\n", > + phylink_autoneg_inband(mode) ? "true" : "false"); > + } else if (ret & PHY_AN_INBAND_ON) { > mode = MLO_AN_INBAND; > + } else { > + mode = MLO_AN_PHY; > + } > > config.interface = iface; > linkmode_copy(support1, support); > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 9a3752c0c444..56a431d88dd9 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -761,6 +761,12 @@ struct phy_tdr_config { > }; > #define PHY_PAIR_ALL -1 > > +enum phy_an_inband { > + PHY_AN_INBAND_UNKNOWN = BIT(0), Shouldn't this be something like PHY_AN_INBAND_UNKNOWN = 0, ? What does it mean if a phy returns e.g. 0b101? --Sean > + PHY_AN_INBAND_OFF = BIT(1), > + PHY_AN_INBAND_ON = BIT(2), > +}; > + > /** > * struct phy_driver - Driver structure for a particular PHY type > * > @@ -845,6 +851,15 @@ struct phy_driver { > */ > int (*config_aneg)(struct phy_device *phydev); > > + /** > + * @validate_an_inband: Report what types of in-band auto-negotiation > + * are available for the given PHY interface type. Returns a bit mask > + * of type enum phy_an_inband. Returning negative error codes is not > + * permitted. > + */ > + int (*validate_an_inband)(struct phy_device *phydev, > + phy_interface_t interface); > + > /** @aneg_done: Determines the auto negotiation result */ > int (*aneg_done)(struct phy_device *phydev); > > @@ -1540,6 +1555,8 @@ void phy_stop(struct phy_device *phydev); > int phy_config_aneg(struct phy_device *phydev); > int phy_start_aneg(struct phy_device *phydev); > int phy_aneg_done(struct phy_device *phydev); > +int phy_validate_an_inband(struct phy_device *phydev, > + phy_interface_t interface); > int phy_speed_down(struct phy_device *phydev, bool sync); > int phy_speed_up(struct phy_device *phydev); >
On Fri, Nov 18, 2022 at 10:11:06AM -0500, Sean Anderson wrote: > > +enum phy_an_inband { > > + PHY_AN_INBAND_UNKNOWN = BIT(0), > > Shouldn't this be something like > > PHY_AN_INBAND_UNKNOWN = 0, > > ? Could be 0 as well. The code explicitly tests against PHY_AN_INBAND_UNKNOWN everywhere, so the precise value doesn't matter too much. > What does it mean if a phy returns e.g. 0b101? You mean PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN. Well, it doesn't mean anything, it's not a valid return code. I didn't make the code too defensive in this regard, because I didn't see a reason for making some pieces of code defend themselves against other pieces of code. It's a bit mask of 3 bits where not all combinations are valid. Even if PHY_AN_INBAND_UNKNOWN was defined as 0 instead of BIT(0), it would still be just as logically invalid to return PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN, but this would be indistinguishable in machine code from just PHY_AN_INBAND_ON. I don't know, I don't see a practical reason to make a change here.
On 11/18/22 10:42, Vladimir Oltean wrote: > On Fri, Nov 18, 2022 at 10:11:06AM -0500, Sean Anderson wrote: >> > +enum phy_an_inband { >> > + PHY_AN_INBAND_UNKNOWN = BIT(0), >> >> Shouldn't this be something like >> >> PHY_AN_INBAND_UNKNOWN = 0, >> >> ? > > Could be 0 as well. The code explicitly tests against PHY_AN_INBAND_UNKNOWN > everywhere, so the precise value doesn't matter too much. > >> What does it mean if a phy returns e.g. 0b101? > > You mean PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN. Well, it doesn't mean > anything, it's not a valid return code. I didn't make the code too defensive > in this regard, because I didn't see a reason for making some pieces of > code defend themselves against other pieces of code. It's a bit mask of > 3 bits where not all combinations are valid. Even if PHY_AN_INBAND_UNKNOWN > was defined as 0 instead of BIT(0), it would still be just as logically > invalid to return PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN, but this > would be indistinguishable in machine code from just PHY_AN_INBAND_ON. > > I don't know, I don't see a practical reason to make a change here. If we have the opportunity, we should try to make invalid return codes inexpressible. If we remove the extra bit, then all the combinations we would like to have: - I don't know what I support - In-band must be enabled - In-band must be disabled - I can support either are exactly the combinations supported by the underlying data. --Sean
On Fri, Nov 18, 2022 at 10:49:30AM -0500, Sean Anderson wrote: > If we have the opportunity, we should try to make invalid return codes > inexpressible. If we remove the extra bit, then all the combinations we > would like to have: > > - I don't know what I support > - In-band must be enabled > - In-band must be disabled > - I can support either > > are exactly the combinations supported by the underlying data. So the change request is to make the enum look like this? enum phy_an_inband { PHY_AN_INBAND_UNKNOWN = 0, PHY_AN_INBAND_OFF = BIT(0), PHY_AN_INBAND_ON = BIT(1), };
On 11/18/22 10:56, Vladimir Oltean wrote: > On Fri, Nov 18, 2022 at 10:49:30AM -0500, Sean Anderson wrote: >> If we have the opportunity, we should try to make invalid return codes >> inexpressible. If we remove the extra bit, then all the combinations we >> would like to have: >> >> - I don't know what I support >> - In-band must be enabled >> - In-band must be disabled >> - I can support either >> >> are exactly the combinations supported by the underlying data. > > So the change request is to make the enum look like this? > > enum phy_an_inband { > PHY_AN_INBAND_UNKNOWN = 0, > PHY_AN_INBAND_OFF = BIT(0), > PHY_AN_INBAND_ON = BIT(1), > }; Yes. --Sean
On Fri, Nov 18, 2022 at 10:57:13AM -0500, Sean Anderson wrote: > On 11/18/22 10:56, Vladimir Oltean wrote: > > So the change request is to make the enum look like this? > > > > enum phy_an_inband { > > PHY_AN_INBAND_UNKNOWN = 0, > > PHY_AN_INBAND_OFF = BIT(0), > > PHY_AN_INBAND_ON = BIT(1), > > }; > > Yes. Ok, I'll make this change and wait for more feedback, and if nothing else, will resubmit on Monday.
On Fri, Nov 18, 2022 at 02:01:18AM +0200, Vladimir Oltean wrote: > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 9a3752c0c444..56a431d88dd9 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -761,6 +761,12 @@ struct phy_tdr_config { > }; > #define PHY_PAIR_ALL -1 > > +enum phy_an_inband { > + PHY_AN_INBAND_UNKNOWN = BIT(0), > + PHY_AN_INBAND_OFF = BIT(1), > + PHY_AN_INBAND_ON = BIT(2), > +}; There is another option here: - unknown (basically, PHY driver doesn't implement the function or can't report) - off (PHY driver knows for certain that in-band isn't used) - on (PHY driver knows that in-band is required and must be acknowledged) - on-but-not-required (PHY driver knows that in-band can be used, but the PHY has hardware support for timing out waiting for the in-band acknowledgement - Marvell PHYs support this.) Maybe the fourth state can be indicated by setting both _OFF and _ON ? Given that there's four states, does it make sense for this to be a bitfield?
On Tue, Nov 22, 2022 at 09:21:36AM +0000, Russell King (Oracle) wrote: > > +enum phy_an_inband { > > + PHY_AN_INBAND_UNKNOWN = BIT(0), > > + PHY_AN_INBAND_OFF = BIT(1), > > + PHY_AN_INBAND_ON = BIT(2), > > +}; > > There is another option here: > > - unknown (basically, PHY driver doesn't implement the function or > can't report) > - off (PHY driver knows for certain that in-band isn't used) > - on (PHY driver knows that in-band is required and must be > acknowledged) > - on-but-not-required (PHY driver knows that in-band can be used, but > the PHY has hardware support for timing out waiting for the in-band > acknowledgement - Marvell PHYs support this.) > > Maybe the fourth state can be indicated by setting both _OFF and _ON ? > > Given that there's four states, does it make sense for this to be a > bitfield? Setting both _OFF and _ON in the capability report would already have the meaning that it's configurable via a subsequent call to phy_config_an_inband(). It's really configurable in VSC8514. I suppose introducing PHY_AN_INBAND_ON_TIMEOUT = BIT(3) could make sense as another option for the capability, orthogonal to the other 2. Maybe it would be useful in itself if the MAC cannot support MLO_AN_INBAND, like the Lynx PCS in 2500base-x, and the PHY only reports PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT (hypothetical example). Phylink would pick PHY_AN_INBAND_ON_TIMEOUT. Given that I don't have a use case for this, should I add PHY_AN_INBAND_ON_TIMEOUT to this patch set or should that be done by someone for whom it makes a difference? The relevant implication for this patch set is the function prototype of phy_config_an_enabled(struct phy_device *phydev, bool enabled). It shouldn't take a bool enabled, but an enum phy_an_inband for future extensibility (and reject/ignore PHY_AN_INBAND_UNKNOWN).
On Tue, Nov 22, 2022 at 11:41:31AM +0200, Vladimir Oltean wrote: > Maybe it would be useful in itself if the MAC cannot support MLO_AN_INBAND, > like the Lynx PCS in 2500base-x, and the PHY only reports PHY_AN_INBAND_ON | > PHY_AN_INBAND_ON_TIMEOUT (hypothetical example). Phylink would pick > PHY_AN_INBAND_ON_TIMEOUT. There's a separate but very much related issue which is that phylink doesn't know that the Lynx PCS doesn't support MLO_AN_INBAND with 2500base-x. So it couldn't make the determination to select PHY_AN_INBAND_ON_TIMEOUT rather than PHY_AN_INBAND_ON right now.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index e5b6cb1a77f9..2abbacf2c7cb 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -733,6 +733,31 @@ static int phy_check_link_status(struct phy_device *phydev) return 0; } +/** + * phy_validate_an_inband - validate which in-band autoneg modes are supported + * @phydev: the phy_device struct + * @interface: the MAC-side interface type + * + * Returns @PHY_AN_INBAND_UNKNOWN if it is unknown what in-band autoneg setting + * is required for the given PHY mode, or a bit mask of @PHY_AN_INBAND_OFF (if + * the PHY is able to work with in-band AN turned off) and @PHY_AN_INBAND_ON + * (if it works with the feature turned on). With the Generic PHY driver, the + * result will always be @PHY_AN_INBAND_UNKNOWN. + */ +int phy_validate_an_inband(struct phy_device *phydev, + phy_interface_t interface) +{ + /* We may be called before phy_attach_direct() force-binds the + * generic PHY driver to this device. In that case, report an unknown + * setting rather than -EIO as most other functions do. + */ + if (!phydev->drv || !phydev->drv->validate_an_inband) + return PHY_AN_INBAND_UNKNOWN; + + return phydev->drv->validate_an_inband(phydev, interface); +} +EXPORT_SYMBOL_GPL(phy_validate_an_inband); + /** * _phy_start_aneg - start auto-negotiation for this PHY device * @phydev: the phy_device struct diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 9e4b2dfc98d8..40b7e730fb33 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -2936,10 +2936,24 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy) return -EINVAL; } - if (phylink_phy_no_inband(phy)) - mode = MLO_AN_PHY; - else + /* Select whether to operate in in-band mode or not, based on the + * capability of the PHY in the current link mode. + */ + ret = phy_validate_an_inband(phy, iface); + if (ret == PHY_AN_INBAND_UNKNOWN) { + if (phylink_phy_no_inband(phy)) + mode = MLO_AN_PHY; + else + mode = MLO_AN_INBAND; + + phylink_dbg(pl, + "PHY driver does not report in-band autoneg capability, assuming %s\n", + phylink_autoneg_inband(mode) ? "true" : "false"); + } else if (ret & PHY_AN_INBAND_ON) { mode = MLO_AN_INBAND; + } else { + mode = MLO_AN_PHY; + } config.interface = iface; linkmode_copy(support1, support); diff --git a/include/linux/phy.h b/include/linux/phy.h index 9a3752c0c444..56a431d88dd9 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -761,6 +761,12 @@ struct phy_tdr_config { }; #define PHY_PAIR_ALL -1 +enum phy_an_inband { + PHY_AN_INBAND_UNKNOWN = BIT(0), + PHY_AN_INBAND_OFF = BIT(1), + PHY_AN_INBAND_ON = BIT(2), +}; + /** * struct phy_driver - Driver structure for a particular PHY type * @@ -845,6 +851,15 @@ struct phy_driver { */ int (*config_aneg)(struct phy_device *phydev); + /** + * @validate_an_inband: Report what types of in-band auto-negotiation + * are available for the given PHY interface type. Returns a bit mask + * of type enum phy_an_inband. Returning negative error codes is not + * permitted. + */ + int (*validate_an_inband)(struct phy_device *phydev, + phy_interface_t interface); + /** @aneg_done: Determines the auto negotiation result */ int (*aneg_done)(struct phy_device *phydev); @@ -1540,6 +1555,8 @@ void phy_stop(struct phy_device *phydev); int phy_config_aneg(struct phy_device *phydev); int phy_start_aneg(struct phy_device *phydev); int phy_aneg_done(struct phy_device *phydev); +int phy_validate_an_inband(struct phy_device *phydev, + phy_interface_t interface); int phy_speed_down(struct phy_device *phydev, bool sync); int phy_speed_up(struct phy_device *phydev);
Currently, phylink requires that fwnodes with links to SFP cages have the 'managed = "in-band-status"' property, and based on this, the initial pl->cfg_link_an_mode gets set to MLO_AN_INBAND. However, some PHYs on SFP modules may have broken in-band autoneg, and in that case, phylink selects a pl->cur_link_an_mode which is MLO_AN_PHY, to tell the MAC/PCS side to disable in-band autoneg (link speed/status will come over the MDIO side channel). The check for PHY in-band autoneg capability is currently open-coded based on a PHY ID comparison against the BCM84881. But the same problem will also be need to solved in another case, where syncing in-band autoneg will be desired between the MAC/PCS and an on-board PHY. So the approach needs to be generalized, and eventually what is done for the BCM84881 needs to be replaced with a more generic solution. Add new API to the PHY device structure which allows it to report what it supports in terms of in-band autoneg (whether it can operate with it on, and whether it can operate with it off). The assumption is that there is a Clause 37 compatible state machine in the PHY's PCS, and it requires that the autoneg process completes before the lane transitions to data mode. If we have a mismatch between in-band autoneg modes, the system side link will be broken. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v3->v4: - split the SFP cur_link_an_mode fixup to separate patch (this one) - s/inband_aneg/an_inband/ to be more in line with phylink terminology - clearer documentation, added kerneldocs - don't return -EIO in phy_validate_an_inband(), this breaks with the Generic PHY driver because the expected return code is a bit mask, not a negative integer drivers/net/phy/phy.c | 25 +++++++++++++++++++++++++ drivers/net/phy/phylink.c | 20 +++++++++++++++++--- include/linux/phy.h | 17 +++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-)