Re: [PATCH v2 1/2] i2c: dev: fix notifier return values

[Geert Uytterhoeven]

Hi Bartosz,

On Wed, Mar 8, 2023 at 8:51 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > >
> > > > We have a set of return values that notifier callbacks can return. They
> > > > should not return 0, error codes or anything other than those predefined
> > > > values. Make the i2c character device's callback return NOTIFY_DONE or
> > > > NOTIFY_OK depending on the situation.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >
> > > Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> > > dev: fix notifier return values") in v6.3-rc1.
> > >
> > > On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> > > On R-Car Gen4, they are still present, as all I2C adapters are
> > > initialized after i2cdev.
> > >
> > > > --- a/drivers/i2c/i2c-dev.c
> > > > +++ b/drivers/i2c/i2c-dev.c
> > > > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > >         int res;
> > > >
> > > >         if (dev->type != &i2c_adapter_type)
> > > > -               return 0;
> > > > +               return NOTIFY_DONE;
> > > >         adap = to_i2c_adapter(dev);
> > > >
> > > >         i2c_dev = get_free_i2c_dev(adap);
> > > >         if (IS_ERR(i2c_dev))
> > > > -               return PTR_ERR(i2c_dev);
> > > > +               return NOTIFY_DONE;
> > > >
> > > >         cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> > > >         i2c_dev->cdev.owner = THIS_MODULE;
> > > > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > >                 goto err_put_i2c_dev;
> > > >
> > > >         pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > > > -       return 0;
> > > > +       return NOTIFY_OK;
> > >
> > > Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> > > notifiers (called from i2cdev_notifier_call()), but also called from
> > > i2c_dev_init():
> > >
> > >         /* Bind to already existing adapters right away */
> > >         i2c_for_each_dev(NULL, i2cdev_attach_adapter);
> > >
> > > and i2c_dev_exit():
> > >
> > >         i2c_for_each_dev(NULL, i2cdev_detach_adapter);
> > >
> > > As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> > > value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> > > processing.
> > >
> > > In i2c_dev_init(), this leads to a failure in registering any
> > > already existing i2c adapters after the first one, causing missing
> > > /dev/i2c-* entries.
> > >
> > > In i2c_dev_exit(), this leads to a failure unregistering any but the
> > > first i2c adapter.
> > >
> > > As there is no one-to-one mapping from error codes to notify codes,
> > > I think this cannot just be handled inside i2cdev_notifier_call() :-(
> >
> > Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
> > that SH can call it directly while notifiers would call it indirectly
> > through the wrapper?
>
> That would be a wrapper that ignores the NOTIFY_* return
> value, and always returns zero? I.e. we can no longer return an
> error.  I guess that's OK, as i2c_dev_init() doesn't take any
> action based on the returned error code anyway.

This works, so I've sent a fix
https://lore.kernel.org/r/03a8cd13af352c4d990bc70b72df4915b9fa2874.1678347776.git.geert+renesas@xxxxxxxxx

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