MIME-Version: 1.0
References: <20221229160045.535778-1-brgl@bgdev.pl> <20221229160045.535778-2-brgl@bgdev.pl>
 <CAMuHMdWBQaB2caVB9vKQoqE0r6jAqOidMRmsCXxyixEm+HWPdw@mail.gmail.com> <CAMRc=McHiMXHzeg_B3zEw74yzNcw48jWEDcZRcvPNZ51XiS0kg@mail.gmail.com>
In-Reply-To: <CAMRc=McHiMXHzeg_B3zEw74yzNcw48jWEDcZRcvPNZ51XiS0kg@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 8 Mar 2023 20:51:55 +0100
Message-ID: <CAMuHMdXUSP3Wp0Jei_PpHV1aS6TL0WcUb68qM21aq5NtdR6vww@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] i2c: dev: fix notifier return values
To:     Bartosz Golaszewski <brgl@bgdev.pl>
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: 8BIT
Precedence: bulk

Hi Bartosz,

On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> 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 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@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 *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.

The only error conditions that can happen in i2c_attach_adapter()
are:
  - "Out of device minors" message in get_free_i2c_dev(),
  - WARN_ON(dev == WHITEOUT_DEV) in cdev_add(),
  - Generic -ENOMEM.
Looks like all of the above can be ignored, as they are all unlikely to
happen, and there is nothing to be done to recover...

Note that this is not "called directly from SH".
The SH/R-Mobile SoCs where I noticed the issue are ARM32.
I guess it can happen on other platforms, too, depending on initialization
order...

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