Received-SPF: pass (google.com: domain of linux-kernel+bounces-191205-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 27 May 2024 19:10:55 +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: <ZlSwbTwRF6KjPfJ5@shredder>
References: <20240424133023.4150624-5-danieller@nvidia.com>
 <20240429201130.5fad6d05@kernel.org>
 <DM6PR12MB45168DC7D9D9D7A5AE3E2B2DD81A2@DM6PR12MB4516.namprd12.prod.outlook.com>
 <20240430130302.235d612d@kernel.org>
 <ZjH1DCu0rJTL_RYz@shredder>
 <20240501073758.3da76601@kernel.org>
 <DM6PR12MB451687C3C54323473716621ED8EB2@DM6PR12MB4516.namprd12.prod.outlook.com>
 <20240522064519.3e980390@kernel.org>
 <DM6PR12MB451677DBA41EA8A622D3D446D8EB2@DM6PR12MB4516.namprd12.prod.outlook.com>
 <20240522072212.7a21c84b@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240522072212.7a21c84b@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?eHgzcXYrbU14a0pCbXpuRXArUWxlQXVDa0x4OFAveEl2Z1ZWNk1LZHJ5VW96?=
 =?utf-8?B?QnJVL0FtclBXaGxtTUtZRjdlYkc2UHBBZWdUcEhoVDBySEp2cHJaaUlmRmIz?=
 =?utf-8?B?aXdoVExGOEFvVGMvelBZMTBoTi9odDh5a24ya2pTVTFEaUdya1NndXBzSE1E?=
 =?utf-8?B?QmV0aXhsUldZNGJYMFlGRjJXcXZtc0ZwY3FXb3ZKNFBzODZhTHZhMzVKZzg0?=
 =?utf-8?B?Vm9tSHFZWFdVR1F0dDJZd3JXenB1VnZJemp5emhPRkgwSnVieTYwOFZSdnZq?=
 =?utf-8?B?UEdEUVRQb2FpZ1drZmNnNFVuWXBnV0c2UzNKVU9HaGFDNnpSNEd3Zm51NTU2?=
 =?utf-8?B?alBRMFdnZEdQYzhjaHFDVVJzVFp6eFIrc3hseThMTjVSZmZWTFFoeTBqMURB?=
 =?utf-8?B?akJPd3NCNUR1ajI2V21uanY4YWdsS2lUbmZzVFdzR0dSR2t6Z29kSHRMVnNU?=
 =?utf-8?B?dThRTVRDUGRTRkc5a3pNTGVVekNiVy92TVFDUGpBVTF4cXo1RFZoOC85dU1s?=
 =?utf-8?B?YXZ2SVlEWThlNGtnbFVPeGNIL0dMcGsvWXhFWDVyM1JHcktaclVYYVBvYWZG?=
 =?utf-8?B?ZWFTelhHcmRDWmFkR2gwR1VZeG5haG1sUEJNTkhkS1Z0L3J3ZU93QXNaMnhP?=
 =?utf-8?B?RnJicStJSGc1aGN5N3dHakcwZElodnEwa2hFTTdiV1liR1VRYjVvRWQ3UndX?=
 =?utf-8?B?RGpCY29EVWtZSE9kVHRtRWNzMHdGNUJWM1JiRC9uV20vTEF3eFp6U0VQNkZT?=
 =?utf-8?B?L0VkRGVHa3ExeEkvZ0c5NlZybG9vREpSUm1BelBoTkVpeG9WVmprUlFEUTRN?=
 =?utf-8?B?clZLZWZVUFJlazFpR3N6RzRXNHJsYnUrdzlvMlN2dWE3dW9iSEQ3STBYMFBv?=
 =?utf-8?B?Vk1aNXhuVEpLL1h0VVV4TE1OSVFmV2t0LzZNZllwMGhENVdWRlN5dDVTOXQ2?=
 =?utf-8?B?VjJkNGZ6RmNybWlZcE02d2FpVE1QNVlhQVF0MlVtUkNVMDlTVVRPNjg1Yk43?=
 =?utf-8?B?a0ZqSEh4NWFqN0tSTGtnamNsMzA2bU9oa1d4R25pd1JRTldETENmaG5MQml3?=
 =?utf-8?B?OEhmM1RFc05aQ2pxSzFKc2hYU0hZbWJLbE9LaGpKbC9Db2xZVDgrMmwwWlZz?=
 =?utf-8?B?bGRnVzNwQmF2bVVBN3d2djFweTEydzlLZGlGVmRuQmQzcTFwc3F1K1YyN0lp?=
 =?utf-8?B?c0NOcWM5L3NTMyt0Ymt4cnlHK3RtakM1OGxZZFFGajRjeGtkME4vUnpKRWE2?=
 =?utf-8?B?aE5DMWxFdmNwVVpQUFQzRjI1bUt1eTgxVi91U001c3JySUYwcGQvbzVwZkxo?=
 =?utf-8?B?Mnc3SE5JZXM4RVgwb29iNHVLeWlwWHRDWlVuVjJVeHZsZSthZTV5Z0NPNEZ3?=
 =?utf-8?B?enM0R0kyV3dKWmpBaW5KN1hTckJqSWJHcUdwV2laeGVuQ1N4Z082Ri9PaTEy?=
 =?utf-8?B?MVVuaFkxYnF2Q0swMXJQQTBmRXljdkQwM1E3OUVna09XYjhRaHNNY2tQRHV1?=
 =?utf-8?B?UzNaS1pNYXVxaUYwdFc4OWRBYmZzMUJKQWtRRTR5UXJ1eEF3c3RXSVo3WjFD?=
 =?utf-8?B?dGM4MEZBTWtzRm5QRTAycy9aWHNwMjR0VGdGZzViUnBxM1RWRmVGUW1XWUsw?=
 =?utf-8?B?UWtDZDJtMVNuSGZqNVRaekN6S1VoeWFYcjlpQUx0SWRoWEVaWUdhcEkxY1FD?=
 =?utf-8?B?Q1hvc05oa3RBUVpNYmlZTGJGSzlVdk1ZM3F6Ni95cWNsbXZMa0YydTlGYjcx?=
 =?utf-8?B?b3VzaURFU05NbWc1akhBOWN3K3dhVm90NERXQ0FSaWVIcU1wS3kvSm5VdDI2?=
 =?utf-8?B?T093Wk5NQUgyTnluQTVuM1lxdkhvOXFaM2J1b0UzSjVqeHhGcjNCTTFOWFB5?=
 =?utf-8?B?V0FwclU1QkFLczllUENVRkVsRWtiTWM0UW5rMTZkNGtUeGNqN0x5M0xXaEFH?=
 =?utf-8?B?YlE4OTFKN01OWFB1NTAxaklZS0lOREpBcW4xSEN2MUtMTUNuSTBDb0F6Wktm?=
 =?utf-8?B?OXdNUG1Wck9VRzl0UzhxcTFXUnVIdTJPOTdnZ2lUZmR1TnEvalBHWnBMM0tP?=
 =?utf-8?B?ZmlxRlcyUkVJZ1VuUFc3TW91VVI2OG1XMGNIU0pBRzEyMXVMRGUwbEtQL3VQ?=
 =?utf-8?Q?niwit3ZFZYMPrxWuyByutqVHb?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85d0d46b-1d31-4593-66fc-08dc7e67a3e9
X-MS-Exchange-CrossTenant-AuthSource: CY5PR12MB6179.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 May 2024 16:11:17.7571
 (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: y6Vsq5cl+Kuz4W0q9vCXs1fb0STe3AEiBJcWTUiGhs0YWqI3ORc8jHzFG0zu0KC8VrB4+yYjBwV3DhTrzS8X9Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB9076

On Wed, May 22, 2024 at 07:22:12AM -0700, Jakub Kicinski wrote:
> On Wed, 22 May 2024 13:56:11 +0000 Danielle Ratson wrote:
> > > > 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.
> > > 
> > > What's the "relevant event" in this case? Closing of the socket that user had
> > > issued the command on?  
> > 
> > The event should match the below:
> > event == NETLINK_URELEASE && notify->protocol == NETLINK_GENERIC
> > 
> > Then iterate over the list to look for work that matches the dev and portid.
> > The socket doesn’t close until the work is done in that case. 
> 
> Okay, good, yes. I think you can use one of the callbacks I mentioned
> below to achieve the same thing with less complexity than the notifier.

Danielle already has a POC with the notifier and it's not that
complicated. I wasn't aware of the netlink notifier, but we found it
when we tried to understand how other netlink families get notified
about a socket being closed.

Which advantages do you see in the sock_priv_destroy() approach? Are you
against the notifier approach?

> > > Easiest way to "notice" the socket got closed would probably be to add some
> > > info to genl_sk_priv_*(). ->sock_priv_destroy() will get called. But you can also
> > > get a close notification in the family  
> > > ->unbind callback.  

Isn't the unbind callback only for multicast (whereas we are using
unicast)?

> > > 
> > > I'm on the fence whether we should cancel the work. We could just mark the
> > > command as 'no socket present' and stop sending notifications.
> > > Not sure which is better..  
> > 
> > Is there a scenario that we hit this event and won't intend to cancel the work? 
> 
> I think it's up to us. I don't see any legit reason for user space to
> intentionally cancel the flashing. So the only option is that user space
> is either buggy or has crashed, and the socket got closed before
> flashing finished. Right?

We don't think that closing the socket / killing the process mid
flashing is a legitimate scenario. We looked into it in order to avoid
sending unicast notifications to a socket that did not ask for them but
gets them because it was bound to the port ID that was used by the old
socket.

I agree that we don't need to cancel the work and can simply have the
work item stop sending notifications. User space will get an error if it
tries to flash a module that is already being flashed in the background.
WDYT?