| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a3b7907a-d7b7-093a-20c5-21beb982e825@gmail.com>
Date: Mon, 19 Mar 2018 12:20:32 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Russell King <rmk+kernel@...linux.org.uk>,
Andrew Lunn <andrew@...n.ch>,
"David S. Miller" <davem@...emloft.net>
Cc: netdev@...r.kernel.org
Subject: Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access
into netdev core ethtool
On 12/17/2017 06:48 AM, 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?
Considering the negative diffstat and the fact that it solves real
problems for you, I would say yes.
>
> 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.
This would be nice, but at this point, I would defer until we have all
major PHYLINK, SFP et al. users merged in tree so we have a good
understanding and view of the different possible combinations that might
exist. Then we can try to define an interface allowing network drivers
more flexibility.
If that seems like an appropriate course of action, do you mind
resubmitting this as non RFC?
>
> drivers/net/phy/phylink.c | 28 ----------------------------
> drivers/net/phy/sfp-bus.c | 6 ++----
> include/linux/netdevice.h | 3 +++
> include/linux/phylink.h | 3 ---
> net/core/ethtool.c | 7 +++++++
> 5 files changed, 12 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index db5d5726ced9..0f59d7149a61 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1247,34 +1247,6 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
> }
> EXPORT_SYMBOL_GPL(phylink_ethtool_set_pauseparam);
>
> -int phylink_ethtool_get_module_info(struct phylink *pl,
> - struct ethtool_modinfo *modinfo)
> -{
> - int ret = -EOPNOTSUPP;
> -
> - WARN_ON(!lockdep_rtnl_is_held());
> -
> - if (pl->sfp_bus)
> - ret = sfp_get_module_info(pl->sfp_bus, modinfo);
> -
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(phylink_ethtool_get_module_info);
> -
> -int phylink_ethtool_get_module_eeprom(struct phylink *pl,
> - struct ethtool_eeprom *ee, u8 *buf)
> -{
> - int ret = -EOPNOTSUPP;
> -
> - WARN_ON(!lockdep_rtnl_is_held());
> -
> - if (pl->sfp_bus)
> - ret = sfp_get_module_eeprom(pl->sfp_bus, ee, buf);
> -
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(phylink_ethtool_get_module_eeprom);
> -
> /**
> * phylink_ethtool_get_eee_err() - read the energy efficient ethernet error
> * counter
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 1356dba0d9d3..4d61099b1357 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -321,6 +321,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
> }
> if (bus->started)
> bus->socket_ops->start(bus->sfp);
> + bus->netdev->sfp_bus = bus;
> bus->registered = true;
> return 0;
> }
> @@ -335,6 +336,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
> if (bus->phydev && ops && ops->disconnect_phy)
> ops->disconnect_phy(bus->upstream);
> }
> + bus->netdev->sfp_bus = NULL;
> bus->registered = false;
> }
>
> @@ -350,8 +352,6 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
> */
> int sfp_get_module_info(struct sfp_bus *bus, struct ethtool_modinfo *modinfo)
> {
> - if (!bus->registered)
> - return -ENOIOCTLCMD;
> return bus->socket_ops->module_info(bus->sfp, modinfo);
> }
> EXPORT_SYMBOL_GPL(sfp_get_module_info);
> @@ -370,8 +370,6 @@ EXPORT_SYMBOL_GPL(sfp_get_module_info);
> int sfp_get_module_eeprom(struct sfp_bus *bus, struct ethtool_eeprom *ee,
> u8 *data)
> {
> - if (!bus->registered)
> - return -ENOIOCTLCMD;
> return bus->socket_ops->module_eeprom(bus->sfp, ee, data);
> }
> EXPORT_SYMBOL_GPL(sfp_get_module_eeprom);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ef789e1d679e..99a0a155c319 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -57,6 +57,7 @@ struct device;
> struct phy_device;
> struct dsa_port;
>
> +struct sfp_bus;
> /* 802.11 specific */
> struct wireless_dev;
> /* 802.15.4 specific */
> @@ -1644,6 +1645,7 @@ enum netdev_priv_flags {
> * @priomap: XXX: need comments on this one
> * @phydev: Physical device may attach itself
> * for hardware timestamping
> + * @sfp_bus: attached &struct sfp_bus structure.
> *
> * @qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
> * @qdisc_running_key: lockdep class annotating Qdisc->running seqcount
> @@ -1922,6 +1924,7 @@ struct net_device {
> struct netprio_map __rcu *priomap;
> #endif
> struct phy_device *phydev;
> + struct sfp_bus *sfp_bus;
> struct lock_class_key *qdisc_tx_busylock;
> struct lock_class_key *qdisc_running_key;
> bool proto_down;
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index bd137c273d38..618fa5e83564 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -211,9 +211,6 @@ void phylink_ethtool_get_pauseparam(struct phylink *,
> struct ethtool_pauseparam *);
> int phylink_ethtool_set_pauseparam(struct phylink *,
> struct ethtool_pauseparam *);
> -int phylink_ethtool_get_module_info(struct phylink *, struct ethtool_modinfo *);
> -int phylink_ethtool_get_module_eeprom(struct phylink *,
> - struct ethtool_eeprom *, u8 *);
> int phylink_get_eee_err(struct phylink *);
> int phylink_ethtool_get_eee(struct phylink *, struct ethtool_eee *);
> int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *);
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f8fcf450a36e..86a6b3d05116 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -22,6 +22,7 @@
> #include <linux/bitops.h>
> #include <linux/uaccess.h>
> #include <linux/vmalloc.h>
> +#include <linux/sfp.h>
> #include <linux/slab.h>
> #include <linux/rtnetlink.h>
> #include <linux/sched/signal.h>
> @@ -2217,6 +2218,9 @@ static int __ethtool_get_module_info(struct net_device *dev,
> const struct ethtool_ops *ops = dev->ethtool_ops;
> struct phy_device *phydev = dev->phydev;
>
> + if (dev->sfp_bus)
> + return sfp_get_module_info(dev->sfp_bus, modinfo);
> +
> if (phydev && phydev->drv && phydev->drv->module_info)
> return phydev->drv->module_info(phydev, modinfo);
>
> @@ -2251,6 +2255,9 @@ static int __ethtool_get_module_eeprom(struct net_device *dev,
> const struct ethtool_ops *ops = dev->ethtool_ops;
> struct phy_device *phydev = dev->phydev;
>
> + if (dev->sfp_bus)
> + return sfp_get_module_eeprom(dev->sfp_bus, ee, data);
> +
> if (phydev && phydev->drv && phydev->drv->module_eeprom)
> return phydev->drv->module_eeprom(phydev, ee, data);
>
>
--
Florian
Powered by blists - more mailing lists