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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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 <kuba@kernel.org> wrote:

> On Fri,  7 Jun 2024 09:18:20 +0200 Maxime Chevallier wrote:
> > +                                                it's parent PHY through an SFP  
> 
> its

good catch, thanks !

> 
> > +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));  
> 
> > +	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.

> > +	return ret;
> > +}
> > +  
> 
> double new line at the end

I'll fix that.

Thanks,

Maxime