| Message ID | e3165b27-b627-41dd-be8f-51ab848010eb@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: improve phylib EEE handling | expand |
On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote: > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index f711bfd75..8ee047747 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -270,6 +270,7 @@ struct ethtool_keee { > __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); > __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised); > + struct netlink_ext_ack *extack; > u32 tx_lpi_timer; > bool tx_lpi_enabled; > bool eee_active; :S I don't think we have a precedent for passing extack inside the paramter struct. I see 25 .set_eee callbacks, not crazy many. Could you plumb this thru as a separate argument, please?
On Tue, Jan 14, 2025 at 03:00:43PM -0800, Jakub Kicinski wrote: > On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote: > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > index f711bfd75..8ee047747 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -270,6 +270,7 @@ struct ethtool_keee { > > __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > > __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); > > __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised); > > + struct netlink_ext_ack *extack; > > u32 tx_lpi_timer; > > bool tx_lpi_enabled; > > bool eee_active; > > :S I don't think we have a precedent for passing extack inside > the paramter struct. I see 25 .set_eee callbacks, not crazy many. > Could you plumb this thru as a separate argument, please? I was going to wait for this before reworking the phylink based EEE support, but as it looks like this is going to be a while before it's merged, I'll rework my series based on this not being merged and post it non-RFC this afternoon.
On 15.01.2025 00:00, Jakub Kicinski wrote: > On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote: >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index f711bfd75..8ee047747 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -270,6 +270,7 @@ struct ethtool_keee { >> __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); >> __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); >> __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised); >> + struct netlink_ext_ack *extack; >> u32 tx_lpi_timer; >> bool tx_lpi_enabled; >> bool eee_active; > > :S I don't think we have a precedent for passing extack inside > the paramter struct. I see 25 .set_eee callbacks, not crazy many. > Could you plumb this thru as a separate argument, please? I see your point regarding calling convention consistency. Drawback of passing extack as a separate argument is that we would have to do the same extension also to functions in phylib. Affected are phy_ethtool_set_eee and genphy_c45_ethtool_set_eee, because extack is to be used in the latter. Passing extack within struct ethtool_keee we don't have to change the functions in the call chain. So passing extack separately comes at a cost. Is it worth it?
On Wed, 15 Jan 2025 18:46:35 +0100 Heiner Kallweit wrote: > On 15.01.2025 00:00, Jakub Kicinski wrote: > > On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote: > >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > >> index f711bfd75..8ee047747 100644 > >> --- a/include/linux/ethtool.h > >> +++ b/include/linux/ethtool.h > >> @@ -270,6 +270,7 @@ struct ethtool_keee { > >> __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > >> __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); > >> __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised); > >> + struct netlink_ext_ack *extack; > >> u32 tx_lpi_timer; > >> bool tx_lpi_enabled; > >> bool eee_active; > > > > :S I don't think we have a precedent for passing extack inside > > the paramter struct. I see 25 .set_eee callbacks, not crazy many. > > Could you plumb this thru as a separate argument, please? > > I see your point regarding calling convention consistency. > Drawback of passing extack as a separate argument is that we would > have to do the same extension also to functions in phylib. > Affected are phy_ethtool_set_eee and genphy_c45_ethtool_set_eee, > because extack is to be used in the latter. > Passing extack within struct ethtool_keee we don't have to change > the functions in the call chain. So passing extack separately > comes at a cost. Is it worth it? I doubt it will be uglier than stuffing transient pointers into a config struct. But we will only know for sure once the code is written..
On 15.01.2025 00:00, Jakub Kicinski wrote: > On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote: >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index f711bfd75..8ee047747 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -270,6 +270,7 @@ struct ethtool_keee { >> __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); >> __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); >> __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised); >> + struct netlink_ext_ack *extack; >> u32 tx_lpi_timer; >> bool tx_lpi_enabled; >> bool eee_active; > > :S I don't think we have a precedent for passing extack inside > the paramter struct. I see 25 .set_eee callbacks, not crazy many. > Could you plumb this thru as a separate argument, please? Thought about alternatives: struct ethtool_netdev_state may be a good candidate for passing extack to ethtool ops. Code below does this for all "set" ops, as a starting point. This approach may even allow us to remove the extack argument from a number of existing ethtool ops, incl. static functions used within these ops. Would this approach be acceptable? diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 870994cc3..28acb9224 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -1171,12 +1171,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev, * @rss_ctx: XArray of custom RSS contexts * @rss_lock: Protects entries in @rss_ctx. May be taken from * within RTNL. + * @extack: For passing netlink error messages * @wol_enabled: Wake-on-LAN is enabled * @module_fw_flash_in_progress: Module firmware flashing is in progress. */ struct ethtool_netdev_state { struct xarray rss_ctx; struct mutex rss_lock; + struct netlink_ext_ack *extack; unsigned wol_enabled:1; unsigned module_fw_flash_in_progress:1; }; diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index b4c45207f..0cc22c482 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -704,7 +704,10 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info) if (ret < 0) goto out_free_cfg; + dev->ethtool->extack = info->extack; ret = ops->set(&req_info, info); + dev->ethtool->extack = NULL; + if (ret < 0) goto out_ops;
On Sun, 9 Feb 2025 00:22:08 +0100 Heiner Kallweit wrote:
> Would this approach be acceptable?
I don't think so. We considered this for ndos, IIRC, global state can
lead to bugs.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index f711bfd75..8ee047747 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -270,6 +270,7 @@ struct ethtool_keee { __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised); + struct netlink_ext_ack *extack; u32 tx_lpi_timer; bool tx_lpi_enabled; bool eee_active; diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c index bf398973e..6546d7290 100644 --- a/net/ethtool/eee.c +++ b/net/ethtool/eee.c @@ -129,7 +129,7 @@ ethnl_set_eee(struct ethnl_req_info *req_info, struct genl_info *info) { struct net_device *dev = req_info->dev; struct nlattr **tb = info->attrs; - struct ethtool_keee eee = {}; + struct ethtool_keee eee = { .extack = info->extack }; bool mod = false; int ret;
Disabled EEE modes (e.g. because not supported by the MAC) are silently filtered out by phylib's set_eee implementation. For being able to present a hint to the user, expose extack as part of struct ethtool_keee. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/linux/ethtool.h | 1 + net/ethtool/eee.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)