Message-ID: <20171217192642.GX10595@n2100.armlinux.org.uk>
Date:   Sun, 17 Dec 2017 19:26:42 +0000
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access
 into netdev core ethtool

On Sun, Dec 17, 2017 at 07:29:22PM +0100, Andrew Lunn wrote:
> On Sun, Dec 17, 2017 at 02:48:27PM +0000, Russell King wrote:
> > Provide a pointer to the SFP bus in struct net_device, so that the
> > ethtool module EEPROM methods can access the SFP directly, rather
> > than needing every user to provide a hook for it.
> > 
> > Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> > ---
> > Questions:
> > 1. Is it worth adding a pointer to struct net_device for these two
> >    methods, rather than having multiple duplicate veneers to vector
> >    the ethtool module EEPROM ioctls through to the SFP bus layer?
> > 
> > 2. Should this allow network/phy drivers to override the default -
> >    the code is currently structured to allow phy drivers to override
> >    network drivers implementations, which seems the wrong way around.
> 
> Hi Russell
> 
> Looking at drivers which implement reading the EEPROM, very few of
> them expose the i2c bus, as a linux i2c bus. They seem to send
> commands off to the firmware, and have it return a block of data.  So
> converting to using the generic SFP code is not going to be too easy.
> 
> Probably a low hanging fruit is to expose a few library like functions
> for parsing the EEPROM data. As you said, there seems to be a few bugs
> in the drivers with respect to actually interpreting the data. So
> having one central implementation, without bugs, would be good.
> 
> Rather than adding the sfp bus to net_device, i think phylink will get
> more use. And the default implementation of these methods can look at
> the phylink to see if there is an sfp device.

You can't layer phylink on top of phylib on top of phylink for the
situation where we have a SFP cage connected to a PHY - that's the
problem.

SFP needs to know what is happening with the net device, and when
to enable or disable the laser, and although there's notifiers for
the netdev up/down, using that is far from ideal in this case - to
do so, SFP would need the reverse phandle in DT so it knows which
network device its associated with.  I've already been there with
a previous iteration of the SFP code.

> We are unlikely to be
> able to replace phydev with phylink, but maybe all new 10Gbps PHY and
> fibre modules not hidden behind firmware could use phylink? So having
> phylink in net_device could make sense. There has been a move to
> remove phydev from the drivers private structure and use the one in
> net_device. Maybe we should do the same for phylink?

I would suggest you read the patch that adds SFP support to the 88x3310
PHY driver - that case makes no use of phylink.  As I mention above,
it's not possible to layer phylink on top of phylib.  Not only would
that lead to nested locks, but phy drivers do not have the knowledge
necessary to know when to make various phylink calls, as phylib
drivers have no clue when the network device comes up/goes down for
example.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Powered by blists - more mailing lists