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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

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

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