Received-SPF: pass (google.com: domain of linux-kernel+bounces-165012-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 1 May 2024 10:53:48 +0300
From: Ido Schimmel <idosch@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Danielle Ratson <danieller@nvidia.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"sdf@google.com" <sdf@google.com>,
	"kory.maincent@bootlin.com" <kory.maincent@bootlin.com>,
	"maxime.chevallier@bootlin.com" <maxime.chevallier@bootlin.com>,
	"vladimir.oltean@nxp.com" <vladimir.oltean@nxp.com>,
	"przemyslaw.kitszel@intel.com" <przemyslaw.kitszel@intel.com>,
	"ahmed.zaki@intel.com" <ahmed.zaki@intel.com>,
	"richardcochran@gmail.com" <richardcochran@gmail.com>,
	"shayagr@amazon.com" <shayagr@amazon.com>,
	"paul.greenwalt@intel.com" <paul.greenwalt@intel.com>,
	"jiri@resnulli.us" <jiri@resnulli.us>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	mlxsw <mlxsw@nvidia.com>, Petr Machata <petrm@nvidia.com>
Subject: Re: [PATCH net-next v5 04/10] ethtool: Add flashing transceiver
 modules' firmware notifications ability
Message-ID: <ZjH1DCu0rJTL_RYz@shredder>
References: <20240424133023.4150624-1-danieller@nvidia.com>
 <20240424133023.4150624-5-danieller@nvidia.com>
 <20240429201130.5fad6d05@kernel.org>
 <DM6PR12MB45168DC7D9D9D7A5AE3E2B2DD81A2@DM6PR12MB4516.namprd12.prod.outlook.com>
 <20240430130302.235d612d@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240430130302.235d612d@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?eMQI/qSR3p4GxTjzW457Vyjko9JYJJk8Sffw5A7AsM20iFHweGYis+GtBcuD?=
 =?us-ascii?Q?7klKfW/OvWZlbBJKBpsdhMNzTENFwRqbQh6mUZc31sJfd1VvxNjVXclMIm2X?=
 =?us-ascii?Q?sre53Jdh8tAKmpxL/Uf8aTtA1akVRqp/9rF2yqpND7EcH7yxXrHFprgf2jnl?=
 =?us-ascii?Q?25K+F4NKfW3Tq6x/geOACPVoNeNGvFOzrrn9QgredgdbBvNSJzPDCAwNkNv9?=
 =?us-ascii?Q?jgdIS9OcJKakpPYKwDR+Qy/NANo2/9D00XJymE88i79GZ9JQjqN50zF5lzPO?=
 =?us-ascii?Q?MyM9KtS5BgnW4LrNq59Gpg2Gj3vrHW5v5jYKVnADck72DVSkLAagS5DzrnBI?=
 =?us-ascii?Q?jIxz/bP2XK22oXC2IBukWBezin/woUmZkV9IKQQjtTpWfM/voF9YkOxbRdin?=
 =?us-ascii?Q?elLFT1YqmUAU/jJVBoanjj5xs6fhH692EERTG7pqb2qSs2kfQ7LQkTjlBtWH?=
 =?us-ascii?Q?3OJn9Kl0707C7WVAPyn7fblFF3BLwvQqFoFN6MWYDSXlbIlQs3rbl82JMuNR?=
 =?us-ascii?Q?g/ylT2qWHdDpzKTU3L6fBdm0FLApZVJb9TfGZGvRn3Y7rMQHSBK2/womf0D/?=
 =?us-ascii?Q?CxFd21G4gfV1diLQjZdQlLnm4Q9WLKQnSw0uBlCgpRi0tax4/EZMcNzoCI6C?=
 =?us-ascii?Q?KswZA7HDFmMwpy1OotvlM/f4+9naCm33FXGs29/M/AkmmltHnWb6OrbxoCbS?=
 =?us-ascii?Q?uHkDaK0ivckcmKvQpEz9tBDSaVGP/65UvAvr+mLNvSBYoigSaDLKZBw0CJjt?=
 =?us-ascii?Q?4d+ibwpFpXx9Byq/WI0j6zaeptPS4SuUiVb6s2syrRSOl2EmjsyfDfky6zU9?=
 =?us-ascii?Q?m5AxjYMIJFMZx6S4Z0Q+9W8tp7139726sMsi+WYs2FNz6RE1/yVsIhsGSt4b?=
 =?us-ascii?Q?IG29ai/dolnXmxIwmBCV2/bQr1rq5Foj+i51OQO3ti8dw+EMUfa7c9NAFmxv?=
 =?us-ascii?Q?thh3H220D5fvaS7OSGuzhaqB85d5OwPQvGiUD1/34bzUkPlkS1rHnalz8MT/?=
 =?us-ascii?Q?uxN15ZUW2xj5EyXdhU0ZoFi4q904MHlL1SonJaJk+6e5fpHSgniug4fEbgHL?=
 =?us-ascii?Q?o6GbtG4uhnlCf+LWlt/NUgsOWbHZrTetEnY/9Wujif39xbs+fOSzKijKJ2l0?=
 =?us-ascii?Q?MiLVEN0+uGeHcLiqziP4p+bJFU/LjoHgBmfJahmsO4+c0jPANi8Y4yyAzOps?=
 =?us-ascii?Q?OWUPmLmGatXRg3WUussAWQiXGySUIemN6w1mrz2MQNAF4LT/I4gtEoqqbBFM?=
 =?us-ascii?Q?HEcatW+2ysL6VKqlzZPNH3GouVNb7aSrCv1d3DlJJmuACblF+MmPPDBAxdbp?=
 =?us-ascii?Q?jeIj47dHMB01bJx7N+PxthlEfVouodo5D8+0oC1h4FyhPf7Acewamo45BtR1?=
 =?us-ascii?Q?rlYoFTm8lYMjH7PRMJZwO7rTOS2lUlOEklOGuzqnDP8BbgzvqphfmnooY7r6?=
 =?us-ascii?Q?3PoVqCKMGWMYLPEfcYzM+nogupDL4FTrM8Fkg16LMDu8cDitnsQhWpMqXU6Y?=
 =?us-ascii?Q?LdPBw5riy42ZFB+GIcBVkge2f7SDqWXW8lzTnR2NOyYg5UdiY/ClQRzxFiaS?=
 =?us-ascii?Q?DmECkIxuAP0i+X5ZiXZ4EyVbchL8MiN2pIi5TIJ4?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 340797cf-a6dc-4e41-67d3-08dc69b3d907
X-MS-Exchange-CrossTenant-AuthSource: CY5PR12MB6179.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 07:53:56.0847
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: po0bdkXovr+M9paw/Rvck9eUZkC2niScOSmyvbueoFME5jAEZJ7g2HVvePuZa/C8X7zM6yfY40rSoNu903Hrvw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB9071

On Tue, Apr 30, 2024 at 01:03:02PM -0700, Jakub Kicinski wrote:
> 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.

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?