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

On 2/17/2021 2:34 AM, Russell King - ARM Linux admin wrote:

On Wed, Feb 17, 2021 at 04:58:26AM +0100, Andrew Lunn wrote:

quoted

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.

quoted

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.

We could be allocating a generic flag today starting from bit 31 and
down and that would certainly be fine without necessarily making my
rework any harder.

The existing issues with phydev->dev_flags as I am sure you are all
aware is that the flags are not name-spaced per driver yet all Ethernet
MAC drivers make assumptions (tg3.c, bcmgenet.c, etc.) and it is not
possible to pass arbitrary configuration settings down the PHY driver,
etc.  I would not hold my breath on this rework as I keep getting sucked
into various issues at work.

FWIW, this series appears to have already been applied.
-- 
Florian