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