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

[Jiri Pirko <jiri@resnulli.us>]

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.



>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.


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