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

[Mark Bloch <mbloch@nvidia.com>]

On 09/07/2026 12:46, Jiri Pirko wrote:
> 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.

As I'm dropping the work queue it doesn't matter. I'll treat failed
parsing as error then.

Mark

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