MIME-Version: 1.0
References: <20221229160045.535778-1-brgl@bgdev.pl> <20221229160045.535778-2-brgl@bgdev.pl>
 <CAMuHMdWBQaB2caVB9vKQoqE0r6jAqOidMRmsCXxyixEm+HWPdw@mail.gmail.com>
In-Reply-To: <CAMuHMdWBQaB2caVB9vKQoqE0r6jAqOidMRmsCXxyixEm+HWPdw@mail.gmail.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Wed, 8 Mar 2023 20:33:36 +0100
Message-ID: <CAMRc=McHiMXHzeg_B3zEw74yzNcw48jWEDcZRcvPNZ51XiS0kg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] i2c: dev: fix notifier return values
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Mar 8, 2023 at 5:58=E2=80=AFPM Geert Uytterhoeven <geert@linux-m68k=
.org> wrote:
>
> Hi Bartosz,
>
> On Thu, Dec 29, 2022 at 5:12=E2=80=AFPM Bartosz Golaszewski <brgl@bgdev.p=
l> wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We have a set of return values that notifier callbacks can return. They
> > should not return 0, error codes or anything other than those predefine=
d
> > 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@linaro.org>
>
> 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 *d=
ev, void *dummy)
> >         int res;
> >
> >         if (dev->type !=3D &i2c_adapter_type)
> > -               return 0;
> > +               return NOTIFY_DONE;
> >         adap =3D to_i2c_adapter(dev);
> >
> >         i2c_dev =3D 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 =3D THIS_MODULE;
> > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *d=
ev, void *dummy)
> >                 goto err_put_i2c_dev;
> >
> >         pr_debug("adapter [%s] registered as minor %d\n", adap->name, a=
dap->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