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

[Jakub Kicinski]

On Tue, 30 Apr 2024 18:11:18 +0000 Danielle Ratson wrote:
> > 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?

Make sure you have read the netlink intro, it should help fill in some
gaps I won't explicitly cover:
https://docs.kernel.org/next/userspace-api/netlink/intro.html

"True" notifications will have pid = 0 and seq = 0, while replies to
commands have those fields populated based on the request.

pid identifies the socket where the message should be delivered.
ethnl_multicast() assumes that it's zero (since it's designed to work
for notifications) and sends the message to all sockets subscribed to 
a multicast / notification group (ETHNL_MCGRP_MONITOR).

So that's the background. What you're doing isn't incorrect but I think
it'd be better if we didn't use the multicast group here, and sent the
messages as a reply - just to the socket which requested the flashing.
Still asynchronously, we just need to save the right pid and seq to use.

Two reasons for this:
 1) convenience, the user space socket won't have to subscribe to 
    the multicast group
 2) the multicast group is really intended for notifying about
    _configuration changes_ done to the system. If there is a daemon
    listening on that group, there's a very high chance it won't care
    about progress of the flashing. Maybe we can send a single
    notification that flashing has been completed but not "progress
    updates"

I think it should work.

> > > +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?

It'd still be better to splice the two strings and the comma directly
to the skb, rather than on the stack using a function which doesn't
check the bounds of the buffer :S