Re: [PATCH v5] gpio: consumer: new virtual driver

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Aug 17, 2023 at 2:38 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Aug 17, 2023 at 02:14:04PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 17, 2023 at 12:03 PM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > On Tue, Aug 15, 2023 at 08:56:50PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > > +     struct gpio_consumer_device *dev = lookup->parent;
> > > > +
> > > > +     guard(mutex)(&dev->lock);
> > > > +
> > > > +     return sprintf(page, "%s\n", lookup->key);
>
> (1)
>
> ...
>
> > > > +static ssize_t
> > > > +gpio_consumer_lookup_config_offset_show(struct config_item *item, char *page)
> > > > +{
> > > > +     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
> > > > +     struct gpio_consumer_device *dev = lookup->parent;
> > > > +     unsigned int offset;
> > > > +
> > > > +     scoped_guard(mutex, &dev->lock)
> > > > +             offset = lookup->offset;
> > > > +
> > > > +     return sprintf(page, "%d\n", offset);
> > >
> > > Consistently it can be simplified same way
> > >
> > >         guard(mutex)(&dev->lock);
> > >
> > >         return sprintf(page, "%d\n", lookup->offset);
> > >
> > > BUT. Thinking about this more. With guard() we put sprintf() inside the lock,
> > > which is suboptimal from runtime point of view. So, I think now that all these
> > > should actually use scoped_guard() rather than guard().
> > >
> >
> > Precisely why I used a scoped guard here. Same elsewhere.
>
> So the 1) has to be amended then.

No! lookup->key is a string stored in the lookup struct BUT protected
by the consumer device's lock (it must be or else the lookup could get
removed while it's being modified). Unless you want to duplicate the
string in order to release the mutex earlier, it has to be locked
until sprintf() returns.

>
> > > > +}
>
> ...
>
> > > > +     enum gpio_lookup_flags flags;
> > > > +
> > > > +     flags = gpio_consumer_lookup_get_flags(item);
> > >
> > > This is perfectly one line < 80 characters.
> >
> > There's nothing wrong with setting the variable on another line though.
>
> Why do we need 3 LoCs instead of a single one? Do you increase your line
> statistics? :-) I really would like to know the rationale behind this.
>

No reason. It's just that churning out new patch versions changing
minute coding style details is not conducive to "getting stuff done".
At some point the code is *good enough*. It will never be perfect.

Bart