Once a virtuser device is instantiated and actively used, allowing rmdir
for its configfs serves no purpose and can be confusing. Userspace
interacts with the virtual consumer at arbitrary times, meaning it
depends on its existance.

Make the subsystem itself depend on the configfs entry for a virtuser
device while it is in active use.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-virtuser.c | 49 ++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
index c9700c1e4126..45b8f192f860 100644
--- a/drivers/gpio/gpio-virtuser.c
+++ b/drivers/gpio/gpio-virtuser.c
@@ -1533,6 +1533,32 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
 	kfree(dev->lookup_table);
 }
 
+static void
+gpio_virtuser_device_lockup_configfs(struct gpio_virtuser_device *dev, bool lock)
+{
+	struct gpio_virtuser_lookup_entry *entry;
+	struct gpio_virtuser_lookup *lookup;
+	struct configfs_subsystem *subsys;
+
+	subsys = dev->group.cg_subsys;
+
+	/*
+	 * The device only needs to depend on leaf lookup entries. This is
+	 * sufficient to lock up all the configfs entries that the
+	 * instantiated, alive device depends on.
+	 */
+	list_for_each_entry(lookup, &dev->lookup_list, siblings) {
+		list_for_each_entry(entry, &lookup->entry_list, siblings) {
+			if (lock)
+				WARN_ON(configfs_depend_item_unlocked(
+						subsys, &entry->group.cg_item));
+			else
+				configfs_undepend_item_unlocked(
+						&entry->group.cg_item);
+		}
+	}
+}
+
 static ssize_t
 gpio_virtuser_device_config_live_store(struct config_item *item,
 				       const char *page, size_t count)
@@ -1545,15 +1571,24 @@ gpio_virtuser_device_config_live_store(struct config_item *item,
 	if (ret)
 		return ret;
 
-	guard(mutex)(&dev->lock);
+	if (live)
+		gpio_virtuser_device_lockup_configfs(dev, true);
 
-	if (live == gpio_virtuser_device_is_live(dev))
-		return -EPERM;
+	scoped_guard(mutex, &dev->lock) {
+		if (live == gpio_virtuser_device_is_live(dev))
+			ret = -EPERM;
+		else if (live)
+			ret = gpio_virtuser_device_activate(dev);
+		else
+			gpio_virtuser_device_deactivate(dev);
+	}
 
-	if (live)
-		ret = gpio_virtuser_device_activate(dev);
-	else
-		gpio_virtuser_device_deactivate(dev);
+	/*
+	 * Undepend is required only if device disablement (live == 0)
+	 * succeeds or if device enablement (live == 1) fails.
+	 */
+	if (live == !!ret)
+		gpio_virtuser_device_lockup_configfs(dev, false);
 
 	return ret ?: count;
 }
-- 
2.43.0

On Tue, Dec 24, 2024 at 7:08 AM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> Once a virtuser device is instantiated and actively used, allowing rmdir
> for its configfs serves no purpose and can be confusing. Userspace
> interacts with the virtual consumer at arbitrary times, meaning it
> depends on its existance.
>
> Make the subsystem itself depend on the configfs entry for a virtuser
> device while it is in active use.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
>  drivers/gpio/gpio-virtuser.c | 49 ++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
> index c9700c1e4126..45b8f192f860 100644
> --- a/drivers/gpio/gpio-virtuser.c
> +++ b/drivers/gpio/gpio-virtuser.c
> @@ -1533,6 +1533,32 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
>         kfree(dev->lookup_table);
>  }
>
> +static void
> +gpio_virtuser_device_lockup_configfs(struct gpio_virtuser_device *dev, bool lock)
> +{
> +       struct gpio_virtuser_lookup_entry *entry;
> +       struct gpio_virtuser_lookup *lookup;
> +       struct configfs_subsystem *subsys;
> +
> +       subsys = dev->group.cg_subsys;

If there'll be a v2 (patch 1/4 possibly needs it), can you assign it
at declaration? Same for 4/4.

Bart

> +
> +       /*
> +        * The device only needs to depend on leaf lookup entries. This is
> +        * sufficient to lock up all the configfs entries that the
> +        * instantiated, alive device depends on.
> +        */
> +       list_for_each_entry(lookup, &dev->lookup_list, siblings) {
> +               list_for_each_entry(entry, &lookup->entry_list, siblings) {
> +                       if (lock)
> +                               WARN_ON(configfs_depend_item_unlocked(
> +                                               subsys, &entry->group.cg_item));
> +                       else
> +                               configfs_undepend_item_unlocked(
> +                                               &entry->group.cg_item);
> +               }
> +       }
> +}
> +
>  static ssize_t
>  gpio_virtuser_device_config_live_store(struct config_item *item,
>                                        const char *page, size_t count)
> @@ -1545,15 +1571,24 @@ gpio_virtuser_device_config_live_store(struct config_item *item,
>         if (ret)
>                 return ret;
>
> -       guard(mutex)(&dev->lock);
> +       if (live)
> +               gpio_virtuser_device_lockup_configfs(dev, true);
>
> -       if (live == gpio_virtuser_device_is_live(dev))
> -               return -EPERM;
> +       scoped_guard(mutex, &dev->lock) {
> +               if (live == gpio_virtuser_device_is_live(dev))
> +                       ret = -EPERM;
> +               else if (live)
> +                       ret = gpio_virtuser_device_activate(dev);
> +               else
> +                       gpio_virtuser_device_deactivate(dev);
> +       }
>
> -       if (live)
> -               ret = gpio_virtuser_device_activate(dev);
> -       else
> -               gpio_virtuser_device_deactivate(dev);
> +       /*
> +        * Undepend is required only if device disablement (live == 0)
> +        * succeeds or if device enablement (live == 1) fails.
> +        */
> +       if (live == !!ret)
> +               gpio_virtuser_device_lockup_configfs(dev, false);
>
>         return ret ?: count;
>  }
> --
> 2.43.0
>

On Thu, Jan 02, 2025 at 02:21:25PM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 24, 2024 at 7:08 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > Once a virtuser device is instantiated and actively used, allowing rmdir
> > for its configfs serves no purpose and can be confusing. Userspace
> > interacts with the virtual consumer at arbitrary times, meaning it
> > depends on its existance.
> >
> > Make the subsystem itself depend on the configfs entry for a virtuser
> > device while it is in active use.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> >  drivers/gpio/gpio-virtuser.c | 49 ++++++++++++++++++++++++++++++------
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
> > index c9700c1e4126..45b8f192f860 100644
> > --- a/drivers/gpio/gpio-virtuser.c
> > +++ b/drivers/gpio/gpio-virtuser.c
> > @@ -1533,6 +1533,32 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
> >         kfree(dev->lookup_table);
> >  }
> >
> > +static void
> > +gpio_virtuser_device_lockup_configfs(struct gpio_virtuser_device *dev, bool lock)
> > +{
> > +       struct gpio_virtuser_lookup_entry *entry;
> > +       struct gpio_virtuser_lookup *lookup;
> > +       struct configfs_subsystem *subsys;
> > +
> > +       subsys = dev->group.cg_subsys;
> 
> If there'll be a v2 (patch 1/4 possibly needs it), can you assign it
> at declaration? Same for 4/4.

That's a good idea, I'll do so in v2.
Let me also fix a minor typo in the commit message ("existance").

-Koichiro

> 
> Bart
> 
> > +
> > +       /*
> > +        * The device only needs to depend on leaf lookup entries. This is
> > +        * sufficient to lock up all the configfs entries that the
> > +        * instantiated, alive device depends on.
> > +        */
> > +       list_for_each_entry(lookup, &dev->lookup_list, siblings) {
> > +               list_for_each_entry(entry, &lookup->entry_list, siblings) {
> > +                       if (lock)
> > +                               WARN_ON(configfs_depend_item_unlocked(
> > +                                               subsys, &entry->group.cg_item));
> > +                       else
> > +                               configfs_undepend_item_unlocked(
> > +                                               &entry->group.cg_item);
> > +               }
> > +       }
> > +}
> > +
> >  static ssize_t
> >  gpio_virtuser_device_config_live_store(struct config_item *item,
> >                                        const char *page, size_t count)
> > @@ -1545,15 +1571,24 @@ gpio_virtuser_device_config_live_store(struct config_item *item,
> >         if (ret)
> >                 return ret;
> >
> > -       guard(mutex)(&dev->lock);
> > +       if (live)
> > +               gpio_virtuser_device_lockup_configfs(dev, true);
> >
> > -       if (live == gpio_virtuser_device_is_live(dev))
> > -               return -EPERM;
> > +       scoped_guard(mutex, &dev->lock) {
> > +               if (live == gpio_virtuser_device_is_live(dev))
> > +                       ret = -EPERM;
> > +               else if (live)
> > +                       ret = gpio_virtuser_device_activate(dev);
> > +               else
> > +                       gpio_virtuser_device_deactivate(dev);
> > +       }
> >
> > -       if (live)
> > -               ret = gpio_virtuser_device_activate(dev);
> > -       else
> > -               gpio_virtuser_device_deactivate(dev);
> > +       /*
> > +        * Undepend is required only if device disablement (live == 0)
> > +        * succeeds or if device enablement (live == 1) fails.
> > +        */
> > +       if (live == !!ret)
> > +               gpio_virtuser_device_lockup_configfs(dev, false);
> >
> >         return ret ?: count;
> >  }
> > --
> > 2.43.0
> >