Re: [PATCH net-next v4 2/3] net: phy: Add is_on_sfp_module flag and phy_on_sfp helper

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:

quoted

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 <redacted>
---
 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!