Re: [PATCH net-next v13 07/13] net: ethtool: Introduce a command to list PHYs on an interface

Hello Jakub,

Sorry about the late reply on that one, I'll follow-up with a new
version shortly.

On Thu, 13 Jun 2024 18:18:12 -0700
Jakub Kicinski [off-list ref] wrote:

On Fri,  7 Jun 2024 09:18:20 +0200 Maxime Chevallier wrote:

quoted

+                                                it's parent PHY through an SFP  

its

good catch, thanks !

quoted

+static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
+				   struct nlattr **tb)
+{
+	struct phy_link_topology *topo = req_base->dev->link_topo;
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_device_node *pdn;
+
+	if (!req_base->phydev)
+		return 0;
+
+	if (!topo)
+		return 0;
+
+	pdn = xa_load(&topo->phys, req_base->phydev->phyindex);
+	memcpy(&req_info->pdn, pdn, sizeof(*pdn));  

quoted

+	xa_for_each_start(&dev->link_topo->phys, ctx->phy_index, pdn, ctx->phy_index) {
+		ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_PHY_GET_REPLY);
+		if (!ehdr) {
+			ret = -EMSGSIZE;
+			break;
+		}
+
+		ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_PHY_HEADER);
+		if (ret < 0) {
+			genlmsg_cancel(skb, ehdr);
+			break;
+		}
+
+		memcpy(&pri->pdn, pdn, sizeof(*pdn));  

Why do you copy the pdn each time 🤔️

Now that you mention that, I think I don't need to. This dates back
from when I was trying to use the ethnl helpers, where I would keep a
copy of the pdn in the request context, between the parse / reply_size
/ fill_reply steps. I then used the same fill_reply in the dump path.

Now that the code has moved away from the ethnl scaffolding I can
indeed rework that so I can use the actual pdn and not a copy of it.

Thanks for pointing it out.

quoted

+	return ret;
+}
+  

double new line at the end

I'll fix that.

Thanks,

Maxime