RE: [PATCH net-next v5 04/10] ethtool: Add flashing transceiver modules' firmware notifications ability

[Danielle Ratson]

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Tuesday, 30 April 2024 6:12
> To: Danielle Ratson <danieller@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; corbet@xxxxxxx; linux@xxxxxxxxxxxxxxx;
> sdf@xxxxxxxxxx; kory.maincent@xxxxxxxxxxx;
> maxime.chevallier@xxxxxxxxxxx; vladimir.oltean@xxxxxxx;
> przemyslaw.kitszel@xxxxxxxxx; ahmed.zaki@xxxxxxxxx;
> richardcochran@xxxxxxxxx; shayagr@xxxxxxxxxx;
> paul.greenwalt@xxxxxxxxx; jiri@xxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; mlxsw <mlxsw@xxxxxxxxxx>; Petr Machata
> <petrm@xxxxxxxxxx>; Ido Schimmel <idosch@xxxxxxxxxx>
> Subject: Re: [PATCH net-next v5 04/10] ethtool: Add flashing transceiver
> modules' firmware notifications ability
> 
> On Wed, 24 Apr 2024 16:30:17 +0300 Danielle Ratson wrote:
> > +	hdr = ethnl_bcastmsg_put(skb,
> ETHTOOL_MSG_MODULE_FW_FLASH_NTF);
> > +	if (!hdr)
> > +		goto err_skb;
> 
> Do we want to blast it to all listeners or treat it as an async reply?
> We can save the seq and portid of the original requester and use reply, I
> think.

I am sorry, I am not sure I understood what you meant here... it should be an async reply, but not sure I understood your suggestion.
Can you explain please?
Thanks!
 
> 
> > +	ret = ethnl_fill_reply_header(skb, dev,
> > +
> ETHTOOL_A_MODULE_FW_FLASH_HEADER);
> > +	if (ret < 0)
> > +		goto err_skb;
> > +
> > +	if (nla_put_u32(skb, ETHTOOL_A_MODULE_FW_FLASH_STATUS,
> status))
> > +		goto err_skb;
> > +
> > +	if (status_msg &&
> > +	    nla_put_string(skb,
> ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG,
> > +			   status_msg))
> > +		goto err_skb;
> > +
> > +	if (nla_put_u64_64bit(skb, ETHTOOL_A_MODULE_FW_FLASH_DONE,
> done,
> > +			      ETHTOOL_A_MODULE_FW_FLASH_PAD))
> 
> nla_put_uint()
> 
> > +		goto err_skb;
> > +
> > +	if (nla_put_u64_64bit(skb, ETHTOOL_A_MODULE_FW_FLASH_TOTAL,
> total,
> > +			      ETHTOOL_A_MODULE_FW_FLASH_PAD))
> 
> nla_put_uint()
> 
> > +		goto err_skb;
> > +
> > +	genlmsg_end(skb, hdr);
> > +	ethnl_multicast(skb, dev);
> > +	return;
> > +
> > +err_skb:
> > +	nlmsg_free(skb);
> > +}
> > +
> > +void ethnl_module_fw_flash_ntf_err(struct net_device *dev,
> > +				   char *err_msg, char *sub_err_msg) {
> > +	char status_msg[120];
> > +
> > +	if (sub_err_msg)
> > +		sprintf(status_msg, "%s, %s.", err_msg, sub_err_msg);
> > +	else
> > +		sprintf(status_msg, "%s.", err_msg);
> 
> Hm, printing in the dot, and assuming sizeof err_msg + sub_err < 116 is a bit
> surprising. But I guess you have a reason...
> 
> Maybe pass them separately to ethnl_module_fw_flash_ntf() then you can
> nla_reserve() the right amount of space and sprintf() directly into the skb?

I can get rid of the dot actually, would it be ok like that?

> 
> > +	ethnl_module_fw_flash_ntf(dev,
> ETHTOOL_MODULE_FW_FLASH_STATUS_ERROR,
> > +				  status_msg, 0, 0);