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: Wednesday, 1 May 2024 17:38
> To: Ido Schimmel <idosch@xxxxxxxxxx>
> Cc: Danielle Ratson <danieller@xxxxxxxxxx>; 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>
> Subject: Re: [PATCH net-next v5 04/10] ethtool: Add flashing transceiver
> modules' firmware notifications ability
> 
> On Wed, 1 May 2024 10:53:48 +0300 Ido Schimmel wrote:
> > We can try to use unicast, but the current design is influenced by
> > devlink firmware flash (see __devlink_flash_update_notify()) and
> > ethtool cable testing (see ethnl_cable_test_started() and
> > ethnl_cable_test_finished()), both of which use multicast
> > notifications although the latter does not update about progress.
> >
> > Do you want us to try the unicast approach or be consistent with the
> > above examples?
> 
> We are charting a bit of a new territory here, you're right that the precedents
> point in the direction of multicast.
> The unicast is harder to get done on the kernel side (we should probably also
> check that the socket pid didn't get reused, stop sending the notifications
> when original socket gets closed?) It will require using pretty much all the
> pieces of advanced netlink infra we have, I'm happy to explain more, but I'll
> also understand if you prefer to stick to multicast.

Hi Jakub,

Following our discussion, I wanted to see if you are ok with the idea below:

1. Add a new unicast function to netlink.c:
void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd)

2. Use it in the notification function instead of the multicast previously used along with genlmsg_unicast().
'portid' and 'seq' taken from genl_info(), are added to the struct ethtool_module_fw_flash, which is accessible from the work item.

3. Create a global list that holds nodes from type struct ethtool_module_fw_flash() and add it as a field in the struct ethtool_module_fw_flash.
Before scheduling a work, a new node is added to the list.

4. Add a new netlink notifier that when the relevant event takes place, deletes the node from the list, wait until the end of the work item, with cancel_work_sync() and free allocations.

Thanks,
Danielle