| Message ID | 20200702042942.76674-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | net: ethtool: Untangle PHYLIB dependency | expand |
On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote: > Hi all, > > This patch series untangles the ethtool netlink dependency with PHYLIB > which exists because the cable test feature calls directly into PHY > library functions. The approach taken here is to utilize a new set of > net_device_ops function pointers which are automatically set to the PHY > library variants when a network device driver attaches to a PHY device. I'm not sure about the idea of creating a copy of netdev_ops for each device using phylib. First, there would be some overhead (just checked my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is quite frequent pattern of comparing dev->netdev_ops against known constants to check if a network device is of certain type; I can't say for sure if it is also used with devices using phylib in existing code but it feels risky. As the two pointers are universal for all devices, couldn't we simply use one global structure with them like we do for IPv6 (ipv6_stub) or some netfilter modules (e.g. nf_ct_hook)? Michal > Florian Fainelli (4): > net: Add cable test netdevice operations > net: phy: Change cable test arguments to net_device > net: phy: Automatically set-up cable test netdev_ops > net: ethtool: Remove PHYLIB dependency > > drivers/net/phy/phy.c | 18 ++++++++++++++---- > drivers/net/phy/phy_device.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/netdevice.h | 14 ++++++++++++++ > include/linux/phy.h | 10 ++++++---- > net/Kconfig | 1 - > net/ethtool/cabletest.c | 12 ++++++++---- > 6 files changed, 74 insertions(+), 13 deletions(-) > > -- > 2.25.1 >
On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote: > On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote: > > Hi all, > > > > This patch series untangles the ethtool netlink dependency with PHYLIB > > which exists because the cable test feature calls directly into PHY > > library functions. The approach taken here is to utilize a new set of > > net_device_ops function pointers which are automatically set to the PHY > > library variants when a network device driver attaches to a PHY device. > > I'm not sure about the idea of creating a copy of netdev_ops for each > device using phylib. First, there would be some overhead (just checked > my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is > quite frequent pattern of comparing dev->netdev_ops against known > constants to check if a network device is of certain type; I can't say > for sure if it is also used with devices using phylib in existing code > but it feels risky. I agree with Michal here. I don't like this. I think we need phylib to register a set of ops with ethtool when it loads. It would also allow us to clean up phy_ethtool_get_strings(), phy_ethtool_get_sset_count(), phy_ethtool_get_stats(). Andrew
On Thu, 2 Jul 2020 18:34:24 +0200 Andrew Lunn wrote: > On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote: > > On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote: > > > Hi all, > > > > > > This patch series untangles the ethtool netlink dependency with PHYLIB > > > which exists because the cable test feature calls directly into PHY > > > library functions. The approach taken here is to utilize a new set of > > > net_device_ops function pointers which are automatically set to the PHY > > > library variants when a network device driver attaches to a PHY device. > > > > I'm not sure about the idea of creating a copy of netdev_ops for each > > device using phylib. First, there would be some overhead (just checked > > my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is > > quite frequent pattern of comparing dev->netdev_ops against known > > constants to check if a network device is of certain type; I can't say > > for sure if it is also used with devices using phylib in existing code > > but it feels risky. > > I agree with Michal here. I don't like this. > > I think we need phylib to register a set of ops with ethtool when it > loads. It would also allow us to clean up phy_ethtool_get_strings(), > phy_ethtool_get_sset_count(), phy_ethtool_get_stats(). +1
On 7/2/2020 9:35 AM, Jakub Kicinski wrote: > On Thu, 2 Jul 2020 18:34:24 +0200 Andrew Lunn wrote: >> On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote: >>> On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote: >>>> Hi all, >>>> >>>> This patch series untangles the ethtool netlink dependency with PHYLIB >>>> which exists because the cable test feature calls directly into PHY >>>> library functions. The approach taken here is to utilize a new set of >>>> net_device_ops function pointers which are automatically set to the PHY >>>> library variants when a network device driver attaches to a PHY device. >>> >>> I'm not sure about the idea of creating a copy of netdev_ops for each >>> device using phylib. First, there would be some overhead (just checked >>> my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is >>> quite frequent pattern of comparing dev->netdev_ops against known >>> constants to check if a network device is of certain type; I can't say >>> for sure if it is also used with devices using phylib in existing code >>> but it feels risky. >> >> I agree with Michal here. I don't like this. >> >> I think we need phylib to register a set of ops with ethtool when it >> loads. It would also allow us to clean up phy_ethtool_get_strings(), >> phy_ethtool_get_sset_count(), phy_ethtool_get_stats(). > > +1 OK, that makes sense, I will work on that.