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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jul 09, 2026 at 07:45:20AM +0200, mbloch@nvidia.com wrote:
>
>
>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

Okay.


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

What I ment was "return error".


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

Why to treat it like this? Is there any other example of such flow in
devlink? I don't see the benefit, only potential confusion in very
unlikely case the alloc_workqueue fails. Am I wrong? If not, just bail
out here.



>
>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();
>>> }
>> 
>> [..]
>