| Message ID | 20221118000124.2754581-6-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, 72 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote:
> + if (pl->config->sync_an_inband && !phy_on_sfp(phy)) {
Hmm, this phy_on_sfp() is new to me, and looking at the git history, I
really don't think this does what it claims to do. This returns the
status of phydev->is_on_sfp_module, which is set by this code:
phydev->phy_link_change = phy_link_change;
if (dev) {
phydev->attached_dev = dev;
dev->phydev = phydev;
if (phydev->sfp_bus_attached)
dev->sfp_bus = phydev->sfp_bus;
else if (dev->sfp_bus)
phydev->is_on_sfp_module = true;
}
... which is very wrong. "dev" here is the net_device, and a net_device
will have its sfp_bus member set when there is a SFP cage present,
which may be behind a off-SFP PHY.
This means that when a PHY is attached by the network driver in their
ndo_open, if there is a SFP bus on the interface (such as on the
Macchiatobin board), the above will set is_on_sfp_module true for the
on-board PHY even though it is not in the SFP module.
Essentially, commit b834489bcecc is incorrect, and needs to be fixed
before use is made of phy_on_sfp() outside of the broadcom driver.
On Fri, Nov 18, 2022 at 10:09:35AM +0000, Russell King (Oracle) wrote: > On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote: > > + if (pl->config->sync_an_inband && !phy_on_sfp(phy)) { > > Hmm, this phy_on_sfp() is new to me, and looking at the git history, I > really don't think this does what it claims to do. This returns the > status of phydev->is_on_sfp_module, which is set by this code: > > phydev->phy_link_change = phy_link_change; > if (dev) { > phydev->attached_dev = dev; > dev->phydev = phydev; > > if (phydev->sfp_bus_attached) > dev->sfp_bus = phydev->sfp_bus; > else if (dev->sfp_bus) > phydev->is_on_sfp_module = true; > } > > ... which is very wrong. "dev" here is the net_device, and a net_device > will have its sfp_bus member set when there is a SFP cage present, > which may be behind a off-SFP PHY. > > This means that when a PHY is attached by the network driver in their > ndo_open, if there is a SFP bus on the interface (such as on the > Macchiatobin board), the above will set is_on_sfp_module true for the > on-board PHY even though it is not in the SFP module. > > Essentially, commit b834489bcecc is incorrect, and needs to be fixed > before use is made of phy_on_sfp() outside of the broadcom driver. IIUC, you're saying that if there is an SFP cage after an on-board PHY X (presumably set using phy_sfp_attach()), then PHY X will be declared as having phydev->is_on_sfp_module = true despite being on-board? I don't have such a setup to experiment with. Looking at armada-8040-mcbin.dts, it's these PHYs, right? &cp0_xmdio { status = "okay"; phy0: ethernet-phy@0 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0>; sfp = <&sfp_eth0>; }; phy8: ethernet-phy@8 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <8>; sfp = <&sfp_eth1>; }; }; &cp0_eth0 { status = "okay"; /* Network PHY */ phy = <&phy0>; phy-mode = "10gbase-r"; }; &cp1_eth0 { status = "okay"; /* Network PHY */ phy = <&phy8>; phy-mode = "10gbase-r"; }; But in this case, I believe that phy_sfp_attach() will set phydev->sfp_bus_attached = true, and this will make the code go through the first "if" branch and not through the "else" (IOW, the code excludes the on-board PHYs from the logic)? Or are you describing some timing/ordering issue which makes this not be the case (something like the sfp_upstream_ops :: attach() of the on-board PHY being called later than the phy_attach_direct())? Could you help me better understand why the code will not enter through the "if" in this case but will enter through the "else"?
On Fri, Nov 18, 2022 at 01:25:20PM +0200, Vladimir Oltean wrote: > On Fri, Nov 18, 2022 at 10:09:35AM +0000, Russell King (Oracle) wrote: > > On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote: > > > + if (pl->config->sync_an_inband && !phy_on_sfp(phy)) { > > > > Hmm, this phy_on_sfp() is new to me, and looking at the git history, I > > really don't think this does what it claims to do. This returns the > > status of phydev->is_on_sfp_module, which is set by this code: > > > > phydev->phy_link_change = phy_link_change; > > if (dev) { > > phydev->attached_dev = dev; > > dev->phydev = phydev; > > > > if (phydev->sfp_bus_attached) > > dev->sfp_bus = phydev->sfp_bus; > > else if (dev->sfp_bus) > > phydev->is_on_sfp_module = true; > > } > > > > ... which is very wrong. "dev" here is the net_device, and a net_device > > will have its sfp_bus member set when there is a SFP cage present, > > which may be behind a off-SFP PHY. > > > > This means that when a PHY is attached by the network driver in their > > ndo_open, if there is a SFP bus on the interface (such as on the > > Macchiatobin board), the above will set is_on_sfp_module true for the > > on-board PHY even though it is not in the SFP module. > > > > Essentially, commit b834489bcecc is incorrect, and needs to be fixed > > before use is made of phy_on_sfp() outside of the broadcom driver. > > IIUC, you're saying that if there is an SFP cage after an on-board PHY > X (presumably set using phy_sfp_attach()), then PHY X will be declared > as having phydev->is_on_sfp_module = true despite being on-board? Having looked more deeply, I don't think it's the problem I thought it was, so you're all good with using phy_on_sfp(). Sorry for the noise.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 2abbacf2c7cb..37cdd9afd7e1 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -758,6 +758,32 @@ int phy_validate_an_inband(struct phy_device *phydev, } EXPORT_SYMBOL_GPL(phy_validate_an_inband); +/** + * phy_config_an_inband - modify in-band autoneg setting + * @phydev: the phy_device struct + * @interface: the MAC-side interface type + * @enabled: selects whether in-band autoneg is used or not + * + * Configures the PHY to enable or disable in-band autoneg for the given + * interface type. @enabled can be passed as true only if the bit mask returned + * by @phy_validate_an_inband() contains @PHY_AN_INBAND_ON, and false only if + * it contains @PHY_AN_INBAND_OFF. + * + * Returns 0 on success, negative error otherwise. + */ +int phy_config_an_inband(struct phy_device *phydev, phy_interface_t interface, + bool enabled) +{ + if (!phydev->drv) + return -EIO; + + if (!phydev->drv->config_an_inband) + return -EOPNOTSUPP; + + return phydev->drv->config_an_inband(phydev, interface, enabled); +} +EXPORT_SYMBOL_GPL(phy_config_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 598f5feb661e..ca3facc4f1a7 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1691,6 +1691,18 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, return ret; } + if (pl->config->sync_an_inband && !phy_on_sfp(phy)) { + bool use_inband = phylink_autoneg_inband(pl->cur_link_an_mode); + + ret = phy_config_an_inband(phy, interface, use_inband); + if (ret && ret != -EOPNOTSUPP) { + phylink_err(pl, + "failed to configure PHY in-band autoneg: %pe\n", + ERR_PTR(ret)); + return ret; + } + } + phy->phylink = pl; phy->phy_link_change = phylink_phy_change; diff --git a/include/linux/phy.h b/include/linux/phy.h index 56a431d88dd9..6f8d5765cf0c 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -860,6 +860,14 @@ struct phy_driver { int (*validate_an_inband)(struct phy_device *phydev, phy_interface_t interface); + /** + * @config_an_inband: Enable or disable in-band auto-negotiation for + * the system-side interface if the PHY operates in a mode that + * requires it: (Q)SGMII, USXGMII, 1000Base-X, etc. + */ + int (*config_an_inband)(struct phy_device *phydev, + phy_interface_t interface, bool enabled); + /** @aneg_done: Determines the auto negotiation result */ int (*aneg_done)(struct phy_device *phydev); @@ -1557,6 +1565,8 @@ 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_config_an_inband(struct phy_device *phydev, phy_interface_t interface, + bool enabled); int phy_speed_down(struct phy_device *phydev, bool sync); int phy_speed_up(struct phy_device *phydev);
Currently Linux has no control over whether a MAC-to-PHY interface uses in-band signaling or not, even though phylink has the managed = "in-band-status"; property which denotes that the MAC expects in-band signaling to be used. The problem is that if the in-band signaling is configurable in both the PHY and the MAC, there is a risk that they are out of sync unless phylink manages them both (setting in PHY may have come from out-of-reset value, or from bootloader configuration). Most in-band autoneg state machines follow IEEE 802.3 clause 37, which means that they will not change the operating mode of the SERDES lane from control to data mode unless in-band AN completed successfully. Therefore traffic will not work. To ensure that systems operate under full control of this particular setting and not depend on what some other entity has done, let's introduce a new PHY driver method for configuring in-band autoneg. The first user will be phylink; the main PHY library does not call phy_config_inband_autoneg(), because it does not know what to configure it to. Presumably, individual non-phylink drivers can also call phy_config_inband_autoneg() individually. To avoid regressions with board device trees which may rely on fragile mechanisms, gate the phy_config_inband_autoneg() call with the bool sync_an_inband opt-in. Also skip doing it for PHYs on SFP modules. I don't see a need for those, so don't risk making a change there. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v3->v4: - s/inband_aneg/an_inband/ - clearer comments, added kerneldocs - opt-in via phylink_config :: sync_an_inband drivers/net/phy/phy.c | 26 ++++++++++++++++++++++++++ drivers/net/phy/phylink.c | 12 ++++++++++++ include/linux/phy.h | 10 ++++++++++ 3 files changed, 48 insertions(+)