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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Wed, Feb 12, 2025 at 04:49:07PM GMT, Geert Uytterhoeven wrote:
> Hi Bartosz,
> 
> On Tue, 4 Feb 2025 at 14:14, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> 
> > > --- 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.

Thank you both for your insights, Bartosz and Geert. I've just sent v3
(https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@xxxxxxxxxxxxx/)
which retains the current behavior, to not suprise anyone now.
I'm now considering whether we might eventually deprecate the sysfs
interface in the future. Doing so could simplify the codebase and bring it
in line with gpio-sim and gpio-virtuser.

Thanks,

Koichiro

> 
> I2C's new_device is also synchronous.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> 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