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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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 <robert.hancock@calian.com>
> > ---
> >  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!