On Wed, Feb 17, 2021 at 04:58:26AM +0100, Andrew Lunn wrote: > On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote: > > Add a flag and helper function to indicate that a PHY device is part of > > an SFP module, which is set on attach. This can be used by PHY drivers > > to handle SFP-specific quirks or behavior. > > > > Signed-off-by: Robert Hancock <[email protected]> > > --- > > drivers/net/phy/phy_device.c | 2 ++ > > include/linux/phy.h | 11 +++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index 05261698bf74..d6ac3ed38197 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct > > phy_device *phydev, > > > > if (phydev->sfp_bus_attached) > > dev->sfp_bus = phydev->sfp_bus; > > + else if (dev->sfp_bus) > > + phydev->is_on_sfp_module = true; > > I get lost here. It this correct? > > We have setups with two PHY. The marvell10g PHY can play the role of > media converter. It converts the signal from the MAC into signals > which can be connected to an SFP cage. > > And then inside the cage, we can have a copper module with a second > PHY. It is this second PHY which we need to indicate is_on_sfp_module > true.
We don't support that setup, at least at the moment. There's no support for stacking PHYs precisely because we have the net_device structure containing a pointer to the PHY (which will be the first PHY in the chain.) That has the effect of making the second PHY inaccessible to the normal network APIs. > We probably want to media convert PHYs LEDs to work, since those > possible are connected to the front panel. So this needs to be > specific to the SFP module PHY, and i'm not sure it is. Russell? The main reason I'm hessitant with using the net_device structure to detect this is that we know that phylink has been converted to work without the net_device structure - it will be NULL. This includes attaching the "primary" PHY - phylib will be given a NULL net_device. The good news is that if a SFP cage is attempted to be attached in that situation, phylink will oops in phylink_sfp_attach(). So it isn't something that we support. However, the point is that we can end up in situations where phylib has a NULL net_device. Florian mentioned in response to one of my previous emails that he's working on sorting out the phylib dev_flags - I think we should wait for that to be resolved first, so we can allocate a dev_flag (or maybe we can do that already today?) that says "this PHY is part of a SFP module". That will give us a clearly defined positive condition that is more maintainable into the future. I'm just worrying that if we sort out phylink_sfp_*tach() (which could be trivial), if we introduce new dependencies into phylib on the network device, we're moving backwards. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
