Re: [PATCH net-next V5 6/6] net/mlx5: Apply devlink eswitch mode boot default on probe

[Mark Bloch <mbloch@nvidia.com>]

On 09/07/2026 12:52, Jiri Pirko wrote:
> Thu, Jul 09, 2026 at 08:00:19AM +0200, mbloch@nvidia.com wrote:
>>
>>
>> On 08/07/2026 11:34, Jiri Pirko wrote:
>>> Tue, Jul 07, 2026 at 07:45:27PM +0200, mbloch@nvidia.com wrote:
>>>> Apply devlink_eswitch_mode= boot defaults for mlx5 after the initial
>>>> probe finishes device initialization while holding the devlink instance
>>>> lock.
>>>>
>>>> At this point the devlink instance is registered and mlx5 can perform an
>>>> eswitch mode change. Calling devl_apply_default_esw_mode() also clears
>>>> any pending default apply work queued by devl_register(), so the queued
>>>> work will not apply the same default again.
>>>>
>>>> Keep this call in mlx5_init_one() rather than the lower-level
>>>> devl-locked init helper. That helper is also used by devlink reload, and
>>>> devlink core already applies the boot default after a successful
>>>> DRIVER_REINIT reload.
>>>>
>>>> Signed-off-by: Mark Bloch <mbloch@nvidia.com>
>>>> ---
>>>> drivers/net/ethernet/mellanox/mlx5/core/main.c | 13 +++++++++++++
>>>> 1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>>>> index 643b4aac2033..0712efea74cc 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>>>> @@ -1392,6 +1392,17 @@ static void mlx5_unload(struct mlx5_core_dev *dev)
>>>> 	mlx5_free_bfreg(dev, &dev->priv.bfreg);
>>>> }
>>>>
>>>> +static void mlx5_devl_apply_default_esw_mode(struct mlx5_core_dev *dev)
>>>> +{
>>>> +	struct devlink *devlink = priv_to_devlink(dev);
>>>> +
>>>> +	if (!MLX5_ESWITCH_MANAGER(dev))
>>>> +		return;
>>>> +
>>>> +	devl_assert_locked(devlink);
>>>> +	devl_apply_default_esw_mode(devlink);
>>>> +}
>>>> +
>>>> int mlx5_init_one_devl_locked(struct mlx5_core_dev *dev)
>>>> {
>>>> 	bool light_probe = mlx5_dev_is_lightweight(dev);
>>>> @@ -1471,6 +1482,8 @@ int mlx5_init_one(struct mlx5_core_dev *dev)
>>>> 	err = mlx5_init_one_devl_locked(dev);
>>>> 	if (err)
>>>> 		devl_unregister(devlink);
>>>> +	else
>>>> +		mlx5_devl_apply_default_esw_mode(dev);
>>>
>>> I don't understand why this patch is needed at all. Just leave the job
>>> to the devlink core, no? That was the point to not pollute drivers with
>>> code like this. Is it some kind of leftover?
>>
>> It was discussed with Jakub here:
>> https://lore.kernel.org/all/20260611085440.4fe36bf2@kernel.org/
>>
>> The main reason is timing. If the default is applied only by devlink
>> core, it has to wait until the driver drops the devlink lock.
> 
> I don't follow.
> 
> <quote>
> 	devl_lock(devlink);
> 	if (dev->shd) {
> 		err = devl_nested_devlink_set(dev->shd, devlink);
> 		if (err)
> 			goto unlock;
> 	}
> 	devl_register(devlink);
> 	err = mlx5_init_one_devl_locked(dev);
> 	if (err)
> 		devl_unregister(devlink);
> unlock:
> 	devl_unlock(devlink);
> </quote>
> 
> devlink lock is droped right after.

Earlier versions had the fw reset / recovery flows covered as well.
The regular probe/init in mlx5 need some changes in mlx5 so I can
move it earlier, left the API just to make sure we are aligned and
it could be used be a later patches.

Anyway, I guess I can drop this as well and continue with followup
patches just to keep forward progress.

> 
> 
> 
>> For mlx5, that usually happens very late in the init sequence. I
>> wanted drivers to be able to apply the default as soon as the driver
>> is ready for it, because on NICs with a DPU the host PF can remain
>> stuck until the ECPF moves to switchdev.
>>
>> This API is also useful beyond the initial devlink registration path.
>> Follow-up patches will use it for driver controlled paths that are
>> not covered by the devlink core, such as recovery and FW reset.
>>
>> There is also a race window where userspace may take the devlink lock
>> before the core gets a chance to apply the default. Letting the driver
>> explicitly apply the default at the right point avoids that scenario.
>>
>> Thinking about this again, maybe the simpler approach is to apply the
>> default from devl_unlock(). That would avoid the whole workqueue
>> infra.
>>
>> I avoided doing that earlier because applying a default mode as a
>> side effect of devl_unlock() feels a bit odd. But compared to adding
>> dedicated workqueue handling maybe it is the lesser evil here.
>>
>> What do you think?
> 
> I was under impression that you need the work to resolve nested locking.
> If not, drop it, sure.

I think that should be fine. Once the instance is registered and the
devlink lock is dropped, the driver should be ready for normal devlink
operations anyway, since a user could trigger the same path at that
point, so we just don't drop the lock and call the driver directly,
should work.

The only caveat I can think of is a driver holding some other lock
while dropping the devlink lock, and then trying to take that same
lock again from the mode-setting callback. But that sounds like a
driver bug rather than something devlink should work around.

I’ll drop the workqueue approach and rework this for v6 :)

Mark

> 
> 
>>
>> About the extra API, I still think it's useful and would like to keep
>> it if possible.
>>
>> Mark
>>
>>
>>>
>>>
>>>
>>>> unlock:
>>>> 	devl_unlock(devlink);
>>>> 	return err;
>>>> -- 
>>>> 2.43.0
>>>>
>>