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

[Bartosz Golaszewski]

On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Bartosz,
>
> 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?

Bart