Re: [PATCH v2 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Bartosz,

On Tue, 4 Feb 2025 at 14:14, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> > For aggregators initialized via configfs, write 1 to 'live' waits for
> > probe completion and returns an error if the probe fails, unlike the
> > legacy sysfs interface, which is asynchronous.
> >
> > Since users control the liveness of the aggregator device and might be
> > editting configurations while 'live' is 0, deferred probing is both
> > unnatural and unsafe.
> >
> > Cancel deferred probe for purely configfs-based aggregators when probe
> > fails.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -1313,7 +1313,6 @@ static struct attribute *gpio_aggregator_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(gpio_aggregator);
> >
> > -
> >  /*
> >   *  GPIO Aggregator platform device
> >   */
> > @@ -1342,8 +1341,22 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
> >
> >         for (i = 0; i < n; i++) {
> >                 descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> > -               if (IS_ERR(descs[i]))
> > +               if (IS_ERR(descs[i])) {
> > +                       /*
> > +                        * Deferred probing is not suitable when the aggregator
> > +                        * is created by userspace. They should just retry later
> > +                        * whenever they like. For device creation via sysfs,
> > +                        * error is propagated without overriding for backward
> > +                        * compatibility. .prevent_deferred_probe is kept unset
> > +                        * for other cases.
> > +                        */
> > +                       if (!init_via_sysfs && !dev_of_node(dev) &&
> > +                           descs[i] == ERR_PTR(-EPROBE_DEFER)) {
> > +                               pr_warn("Deferred probe canceled for creation by userspace.\n");
> > +                               return -ENODEV;
> > +                       }
> >                         return PTR_ERR(descs[i]);
> > +               }
> >         }
> >
> >         features = (uintptr_t)device_get_match_data(dev);
>
> Geert, what do you think about making the sysfs interface synchronous
> instead? I would argue it's actually more logical as the user will
> instinctively expect the aggregator to be ready after write() to
> new_device returns.

On one hand, I agree that it would make some scenarios simpler, and
let us propagate an error code to the sysfs writer in case of failure.

On the other hand, it would change user behavior. Currently people can
configure a GPIO aggregator, and load the driver module for the parent
gpiochip later, relying on deferred probing to bring up everything
when it is ready.

I2C's new_device is also synchronous.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds