Date:   Wed, 17 Feb 2021 12:21:53 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>
Cc:     Robert Hancock <robert.hancock@...ian.com>, hkallweit1@...il.com,
        davem@...emloft.net, kuba@...nel.org,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org
Subject: 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:
>> 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@...ian.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.

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

Powered by blists - more mailing lists