| Message ID | 20250227182454.1998236-3-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: ethtool: netlink: Fix notifications for | expand |
On Thu, 27 Feb 2025 19:24:52 +0100 Maxime Chevallier wrote: > The only relevant user for now is PLCA, and it very likely that we never > ended-up in a situation where the follow-up notif wasn't targeting the > correct PHY as : PLCA uses the ethnl_default_* handlers but it seems to operate on PHYs now. How does the dump work? Shoehorning the PHY support into the ethnl_default_* handlers is starting to look pretty messy.
On Fri, 28 Feb 2025 18:24:40 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 27 Feb 2025 19:24:52 +0100 Maxime Chevallier wrote: > > The only relevant user for now is PLCA, and it very likely that we never > > ended-up in a situation where the follow-up notif wasn't targeting the > > correct PHY as : > > PLCA uses the ethnl_default_* handlers but it seems to operate on PHYs > now. How does the dump work? Shoehorning the PHY support into > the ethnl_default_* handlers is starting to look pretty messy. I agree, that's the less ugly quick solution I could think of :( So maybe we need some generic PHY dump support for all PHY commands ? I should probably re-send patch 1 only to fix the crash, and rework the dump/notify for PHY commands separately if this is OK for you ? Maxime
On Fri, 28 Feb 2025 18:24:40 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 27 Feb 2025 19:24:52 +0100 Maxime Chevallier wrote: > > The only relevant user for now is PLCA, and it very likely that we never > > ended-up in a situation where the follow-up notif wasn't targeting the > > correct PHY as : > > PLCA uses the ethnl_default_* handlers but it seems to operate on PHYs > now. How does the dump work? Shoehorning the PHY support into > the ethnl_default_* handlers is starting to look pretty messy. Thinking more about this, this patch shouldn't have targetted -net in the first place, as no usecase regressed yet (we don't have any multi-PHY PLCA setup that exists). The DUMP keeps working as it did before for that command, in that we dump the PLCA info for netdev->phydev. That DUMP / notify situation needs to be addressed as tome point thouh, before the multi-PHY support through muxes lands (haven't sent it yet, I'm still on the preliminary series for phy_ports). So I think that indeed, I'll resend patch 1, and find a more graceful solution for the phy-targetting commands in a separate series. Maxime
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 734849a57369..6691a8f73bfd 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -663,8 +663,17 @@ static int ethnl_default_done(struct netlink_callback *cb) return 0; } +/* Structure to store context information between a ->set request and the + * follow-up notification. Used only for the ethnl_default ops. + * @phy_index: If the original ->set request had a PHY index, store it in ctx. + */ +struct ethnl_default_notify_ctx { + u32 phy_index; +}; + static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info) { + struct ethnl_default_notify_ctx ctx = {0}; const struct ethnl_request_ops *ops; struct ethnl_req_info req_info = {}; const u8 cmd = info->genlhdr->cmd; @@ -691,6 +700,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info) } dev = req_info.dev; + ctx.phy_index = req_info.phy_index; rtnl_lock(); dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg), @@ -711,7 +721,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info) swap(dev->cfg, dev->cfg_pending); if (!ret) goto out_ops; - ethtool_notify(dev, ops->set_ntf_cmd, NULL); + ethtool_notify(dev, ops->set_ntf_cmd, &ctx); ret = 0; out_ops: @@ -749,6 +759,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = { static void ethnl_default_notify(struct net_device *dev, unsigned int cmd, const void *data) { + const struct ethnl_default_notify_ctx *ctx = data; struct ethnl_reply_data *reply_data; const struct ethnl_request_ops *ops; struct ethnl_req_info *req_info; @@ -776,6 +787,8 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd, req_info->dev = dev; req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS; + if (ctx) + req_info->phy_index = ctx->phy_index; ethnl_init_reply_data(reply_data, ops, dev); ret = ops->prepare_data(req_info, reply_data, &info);
In some situations, it's useful to get some context passed to ethnl notifications, especially when we perform a ->set followed by ethtool_notify(). One such case is when the ->set targets a specific PHY device. The phy_index of the PHY may be coming from the request header, and we want the followup notification to be specific to the phydev we just accessed. This commit leverages the const void *data pointer that's passed to ethtool netlink notifications. In our case, and only for default ethnl ops, lets use that void* pointer to pass a context. The context is filled in the ->set request path, and used in ethnl_default_notify() to populate the req_info with context information. For now, the only thing we pass in the context is the phy_index of the ->set request. The only relevant user for now is PLCA, and it very likely that we never ended-up in a situation where the follow-up notif wasn't targeting the correct PHY as : - This was broken due to the tb[] array being NULL for notifs - There's no upstream-supported scenario (as of today) where we have 2 PHYs that can do PLCA (a BaseT1 feature) on the same netdev. Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands") Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- net/ethtool/netlink.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)