MIME-Version: 1.0
References: <20221229160045.535778-1-brgl@bgdev.pl> <20221229160045.535778-2-brgl@bgdev.pl>
In-Reply-To: <20221229160045.535778-2-brgl@bgdev.pl>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 8 Mar 2023 17:58:25 +0100
Message-ID: <CAMuHMdWBQaB2caVB9vKQoqE0r6jAqOidMRmsCXxyixEm+HWPdw@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 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() :-(

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