Re: [PATCH 3/4] gpio: virtuser: lock up configfs that an instantiated device depends on

[Bartosz Golaszewski]

On Tue, Dec 24, 2024 at 7:08 AM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
>  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
>