Re: [PATCH net-next V5 4/6] devlink: Apply eswitch mode boot defaults

[Mark Bloch <mbloch@nvidia.com>]

On 08/07/2026 11:59, Jiri Pirko wrote:
> Tue, Jul 07, 2026 at 07:45:25PM +0200, mbloch@nvidia.com wrote:
>> Apply parsed devlink_eswitch_mode= defaults after devlink registration
>> and after successful reload.
>>
>> devl_register() may still be called before the device is ready for an
>> eswitch mode change. Keep the registration path passive and let the
>> regular devl_unlock() path queue the async apply work once the instance
>> is registered and the default is still pending.
>>
>> The queueing path runs while the devlink instance lock is held, so the
>> queued work gets its devlink reference before the caller drops the lock.
>> The worker then takes the devlink instance lock normally and applies the
>> default only if the instance is still registered and the default is still
>> pending.
> 
> This is very code-descriptive. What's the benefit of that?

The point is that there is still a window before the queued work
runs where the user can explicitly set the eswitch mode. If they 
do, the default will no longer be pending, so the worker will skip
applying it.

I'll reword.

> 
> 
>>
>> For successful reloads that performed DRIVER_REINIT, devlink_reload()
>> already holds the devlink instance lock and the driver has completed
>> reload_up(). Clear pending work and apply the default directly from the
>> reload path instead of queueing work.
>>
>> Preserve the user configured mode when it is set before devlink applies
>> the default.
>>
> 
> [..]
> 
> 
>> +void devlink_default_esw_mode_apply_locked(struct devlink *devlink)
>> +{
>> +	const struct devlink_ops *ops = devlink->ops;
>> +	int err;
>> +
>> +	devl_assert_locked(devlink);
>> +
>> +	if (!devlink_default_esw_mode_match(devlink))
>> +		return;
>> +
>> +	if (!ops->eswitch_mode_set) {
>> +		if (!devlink_default_esw_mode_match_all)
>> +			devl_warn(devlink,
>> +				  "devlink_eswitch_mode= selected this device but eswitch mode setting is not supported\n");
>> +		return;
>> +	}
>> +
>> +	err = devlink_eswitch_mode_set(devlink, devlink_default_esw_mode, NULL);
>> +	if (err)
>> +		devl_warn(devlink,
>> +			  "Couldn't apply default eswitch mode, err %d\n",
>> +			  err);
>> +}
>> +
>> +void devlink_default_esw_mode_queue_apply_work(struct devlink *devlink)
> 
> eswitch/esw - we call it "eswitch" consistently everywhere. Why "esw"
> here?

Ack

> 
> 
> 
>> +{
>> +	devl_assert_locked(devlink);
>> +
>> +	if (!devlink_default_esw_mode_enabled || !devlink_default_esw_mode_wq)
>> +		return;
>> +	if (!devlink->default_esw_mode_apply_pending ||
>> +	    !__devl_is_registered(devlink))
>> +		return;
>> +	if (!devlink_try_get(devlink))
>> +		return;
>> +	if (!queue_work(devlink_default_esw_mode_wq,
>> +			&devlink->default_esw_mode_apply_work))
>> +		devlink_put(devlink);
>> +}
>> +
>> +static void devlink_default_esw_mode_apply_work(struct work_struct *work)
>> +{
>> +	struct devlink *devlink;
>> +
>> +	devlink = container_of(work, struct devlink,
>> +			       default_esw_mode_apply_work);
>> +
> 
> What happens if userspace eswitch mode set happens now? Any userspace
> attempt should cancel the default apply. I don't see such mechanism in
> your patches, did I miss it?

devlink_nl_eswitch_set_doit() calls
devlink_default_esw_mode_apply_pending_clear(), which clears the
pending bit.

So if a user sets the eswitch mode before the queued default
work applies it, the worker will see that the default is no longer
pending and will do nothing

> 
> 
> 
>> +	devl_lock(devlink);
>> +
>> +	if (devl_is_registered(devlink) &&
>> +	    devlink->default_esw_mode_apply_pending) {
>> +		devlink_default_esw_mode_apply_locked(devlink);
>> +		devlink->default_esw_mode_apply_pending = false;
>> +	}
>> +
>> +	devl_unlock(devlink);
>> +	devlink_put(devlink);
>> +}
>> +
>> +void devlink_default_esw_mode_instance_init(struct devlink *devlink)
> 
> Why "_instance_"? Care to drop?

Ack

> 
> 
>> +{
>> +	INIT_WORK(&devlink->default_esw_mode_apply_work,
>> +		  devlink_default_esw_mode_apply_work);
>> +	devlink->default_esw_mode_apply_pending = true;
>> +}
>> +
>> +void devlink_default_esw_mode_apply_pending_clear(struct devlink *devlink)
>> +{
>> +	devl_assert_locked(devlink);
>> +
>> +	devlink->default_esw_mode_apply_pending = false;
>> +}
>> +
>> +void devlink_default_esw_mode_instance_cleanup(struct devlink *devlink)
> 
> Why "_instance_"? Care to drop?

Ack

> 
> 
>> +{
>> +	if (cancel_work_sync(&devlink->default_esw_mode_apply_work))
>> +		devlink_put(devlink);
>> +}
>> +
>> static int __init devlink_default_esw_mode_setup(char *str)
>> {
>> 	devlink_default_esw_mode_param = str;
>> @@ -228,10 +325,21 @@ int __init devlink_default_esw_mode_init(void)
>> 		return err;
>> 	}
>>
>> +	devlink_default_esw_mode_wq = alloc_workqueue("devlink_default_esw_mode",
>> +						      WQ_UNBOUND | WQ_MEM_RECLAIM,
>> +						      0);
>> +	if (!devlink_default_esw_mode_wq) {
>> +		devlink_default_esw_mode_param = NULL;
>> +		devlink_default_esw_mode_nodes_clear();
>> +		pr_warn("devlink: devlink_eswitch_mode parameter ignored, failed to allocate workqueue\n");
> 
> Why you don't "return"  here? I think that we don't need to allow the
> case wq is not allocated.

The function returns right after this block. It is not treated
as a valid “workqueue unavailable” mode, the parsed defaults are
cleared, the parameter is ignored, and no default eswitch mode will
be applied.

I kept it as a non critical failure so we do not abort the whole
devlink init just because the default-mode workqueue could not be
allocated.

That said, I can make this more explicit by returning 0 directly
from this error path.

Mark

> 
> 
>> +	}
>> +
>> 	return 0;
>> }
>>
>> void __init devlink_default_esw_mode_cleanup(void)
>> {
>> +	if (devlink_default_esw_mode_wq)
>> +		destroy_workqueue(devlink_default_esw_mode_wq);
>> 	devlink_default_esw_mode_nodes_clear();
>> }
> 
> [..]