All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org
Subject: Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool
Date: Sun, 17 Dec 2017 19:29:22 +0100	[thread overview]
Message-ID: <20171217182922.GB29596@lunn.ch> (raw)
In-Reply-To: <E1eQaEt-000329-8b@rmk-PC.armlinux.org.uk>

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@armlinux.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. 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?

    Andrew

  parent reply	other threads:[~2017-12-17 18:29 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-17 14:48 [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool Russell King
2017-12-17 17:06 ` Russell King - ARM Linux
2017-12-17 18:29 ` Andrew Lunn [this message]
2017-12-17 19:26   ` Russell King - ARM Linux
2018-03-19 19:20 ` Florian Fainelli
2018-03-19 20:39   ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171217182922.GB29596@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=rmk+kernel@armlinux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.