| Message ID | 20240701-b4-dp83869-sfp-v1-6-a71d6d0ad5f8@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: dp83869: Add support for downstream SFP cages | expand |
> + if (dp83869->mod_phy) { > + ret = phy_read_status(dp83869->mod_phy); > + if (ret) > + return ret; Locking? When phylib does this in phy_check_link_status(), we have: lockdep_assert_held(&phydev->lock); I don't see anything which takes the downstreams PHY lock. Is this also introducing race conditions? What happens if the link just went down? phy_check_link_status() takes actions. Will they still happen when phylib next talks to the downstream PHY? It is probably safer to call phy_check_link_status() than phy_read_status(). Andrew
On lundi 1 juillet 2024 19:09:40 UTC+2 Andrew Lunn wrote: > > + if (dp83869->mod_phy) { > > + ret = phy_read_status(dp83869->mod_phy); > > + if (ret) > > + return ret; > > Locking? When phylib does this in phy_check_link_status(), we have: > > lockdep_assert_held(&phydev->lock); > > I don't see anything which takes the downstreams PHY lock. > > Is this also introducing race conditions? What happens if the link > just went down? phy_check_link_status() takes actions. Will they still > happen when phylib next talks to the downstream PHY? It is probably > safer to call phy_check_link_status() than phy_read_status(). Given that the phylib state machine will call phy_check_link_status() itself, I think that this call to phy_read_status() could be dropped entirely and that dp83869_read_status() could just directly read mod_phy->{link,speed,duplex}. This raises the question of a potential race condition when reading mod_phy->{link, speed, duplex}. I haven't seen any kind of locking used in other parts of the net subsystem when reading these parameters. Thanks,
On Tue, Jul 02, 2024 at 11:04:37AM +0200, Romain Gantois wrote: > This raises the question of a potential race condition when reading > mod_phy->{link, speed, duplex}. I haven't seen any kind of locking used in > other parts of the net subsystem when reading these parameters. Drivers access these under phydev->lock due to a callback from phylib which will be made from the state machine which holds that lock. This includes phylink.
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index a07ec1be84baf..843af90667d41 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -43,6 +43,7 @@ #define DP83869_IO_MUX_CFG 0x0170 #define DP83869_OP_MODE 0x01df #define DP83869_FX_CTRL 0x0c00 +#define DP83869_FX_STS 0x0c01 #define DP83869_SW_RESET BIT(15) #define DP83869_SW_RESTART BIT(14) @@ -72,6 +73,10 @@ /* This is the same bit mask as the BMCR so re-use the BMCR default */ #define DP83869_FX_CTRL_DEFAULT MII_DP83869_BMCR_DEFAULT +/* FX_STS bits */ +#define DP83869_FX_LSTATUS BIT(2) +#define DP83869_FX_ANEGCOMPLETE BIT(5) + /* CFG1 bits */ #define DP83869_CFG1_DEFAULT (ADVERTISE_1000HALF | \ ADVERTISE_1000FULL | \ @@ -160,7 +165,8 @@ struct dp83869_private { static int dp83869_read_status(struct phy_device *phydev) { struct dp83869_private *dp83869 = phydev->priv; - int ret; + int ret, old_link = phydev->link; + u32 status; ret = genphy_read_status(phydev); if (ret) @@ -176,6 +182,38 @@ static int dp83869_read_status(struct phy_device *phydev) } } + if (dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) { + /* check if SGMII link is up */ + status = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_STS); + + phydev->link = status & DP83869_FX_LSTATUS ? 1 : 0; + phydev->autoneg_complete = status & DP83869_FX_ANEGCOMPLETE ? 1 : 0; + + if (!phydev->autoneg_complete) { + phydev->link = 0; + } else if (phydev->link && !old_link) { + /* It seems like link status and duplex resolved from + * SGMII autonegotiation are incorrectly reported in + * the fiber link partner capabilities register and in + * the PHY status register. If there is a handle to the + * downstream PHY, read link parameters from it. If + * not, fallback to unknown. + */ + + if (dp83869->mod_phy) { + ret = phy_read_status(dp83869->mod_phy); + if (ret) + return ret; + + phydev->speed = dp83869->mod_phy->speed; + phydev->duplex = dp83869->mod_phy->duplex; + } else { + phydev->speed = SPEED_UNKNOWN; + phydev->duplex = DUPLEX_UNKNOWN; + } + } + } + return 0; }
In RGMII-SGMII bridge mode, the DP83869HM PHY can perform an SGMII autonegotiation with a downstream link partner. In this operational mode, the standard link and autonegotiation status bits do not report correct values, the extended fiber status register must be checked. Link parameters should theoretically be obtainable from the DP83869 registers after SGMII autonegotiation has completed. However, for unknown reasons, this is not the case, and the link partner fiber capabilities register will incorrectly report a remote fault even if the SGMII autonegotiation was successful. Modify the read_status() callback of the DP83869 driver to check the fiber status register in RGMII-SGMII bridge mode. On link up, obtain the link parameters from the downstream SFP PHY driver if possible. If not, set speed and duplex to "unknown". Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> --- drivers/net/phy/dp83869.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)