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

[Jiri Pirko <jiri@resnulli.us>]

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?


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



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



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


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


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


>+	}
>+
> 	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();
> }

[..]