MIME-Version: 1.0
References: <03a8cd13af352c4d990bc70b72df4915b9fa2874.1678347776.git.geert+renesas@glider.be>
In-Reply-To: <03a8cd13af352c4d990bc70b72df4915b9fa2874.1678347776.git.geert+renesas@glider.be>
From:   Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Date:   Thu, 9 Mar 2023 09:30:29 +0100
Message-ID: <CACMJSevqE8NC-=so7W_Xge-yw7w+La0dJTqx0z9u_AYt-7gtuQ@mail.gmail.com>
Subject: Re: [PATCH] i2c: dev: Fix bus callback return values
To:     Geert Uytterhoeven <geert+renesas@glider.be>
Cc:     Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 9 Mar 2023 at 08:45, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>
> The i2cdev_{at,de}tach_adapter() callbacks are used for two purposes:
>   1. As notifier callbacks, when (un)registering I2C adapters created or
>      destroyed after i2c_dev_init(),
>   2. As bus iterator callbacks, for registering already existing
>      adapters from i2c_dev_init(), and for cleanup.
>
> Unfortunately both use cases expect different return values: the former
> expects NOTIFY_* return codes, while the latter expects zero or error
> codes, and aborts in case of error.
>
> Hence in case 2, as soon as i2cdev_{at,de}tach_adapter() returns
> (non-zero) NOTIFY_OK, the bus iterator aborts.  This causes (a) only the
> first already existing adapter to be registered, leading to missing
> /dev/i2c-* entries, and (b) a failure to unregister all but the first
> I2C adapter during cleanup.
>
> Fix this by introducing separate callbacks for the bus iterator,
> wrapping the notifier functions, and always returning succes.
> Any errors inside these callback functions are unlikely to happen, and
> are fatal anyway.
>
> Fixes: cddf70d0bce71c2a ("i2c: dev: fix notifier return values")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Seen on r8a7740/armadillo and r8a73a4/ape6evm, where the i2c-shmobile
> adapters are probed before i2c_dev_init().
> Not seen on r8a779g0/white-hawk, where all I2C adapters are probed after
> i2c_dev_init().
>
>  drivers/i2c/i2c-dev.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 107623c4cc14aaf9..95a0b63ac560cf33 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -646,7 +646,7 @@ static void i2cdev_dev_release(struct device *dev)
>         kfree(i2c_dev);
>  }
>
> -static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> +static int i2cdev_attach_adapter(struct device *dev)
>  {
>         struct i2c_adapter *adap;
>         struct i2c_dev *i2c_dev;
> @@ -685,7 +685,7 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
>         return NOTIFY_DONE;
>  }
>
> -static int i2cdev_detach_adapter(struct device *dev, void *dummy)
> +static int i2cdev_detach_adapter(struct device *dev)
>  {
>         struct i2c_adapter *adap;
>         struct i2c_dev *i2c_dev;
> @@ -711,9 +711,9 @@ static int i2cdev_notifier_call(struct notifier_block *nb, unsigned long action,
>
>         switch (action) {
>         case BUS_NOTIFY_ADD_DEVICE:
> -               return i2cdev_attach_adapter(dev, NULL);
> +               return i2cdev_attach_adapter(dev);
>         case BUS_NOTIFY_DEL_DEVICE:
> -               return i2cdev_detach_adapter(dev, NULL);
> +               return i2cdev_detach_adapter(dev);
>         }
>
>         return NOTIFY_DONE;
> @@ -725,6 +725,18 @@ static struct notifier_block i2cdev_notifier = {
>
>  /* ------------------------------------------------------------------------- */
>
> +static int __init i2c_dev_attach_adapter(struct device *dev, void *dummy)
> +{
> +       i2cdev_attach_adapter(dev);
> +       return 0;
> +}
> +
> +static int __exit i2c_dev_detach_adapter(struct device *dev, void *dummy)
> +{
> +       i2cdev_detach_adapter(dev);
> +       return 0;
> +}
> +
>  /*
>   * module load/unload record keeping
>   */
> @@ -752,7 +764,7 @@ static int __init i2c_dev_init(void)
>                 goto out_unreg_class;
>
>         /* Bind to already existing adapters right away */
> -       i2c_for_each_dev(NULL, i2cdev_attach_adapter);
> +       i2c_for_each_dev(NULL, i2c_dev_attach_adapter);
>
>         return 0;
>
> @@ -768,7 +780,7 @@ static int __init i2c_dev_init(void)
>  static void __exit i2c_dev_exit(void)
>  {
>         bus_unregister_notifier(&i2c_bus_type, &i2cdev_notifier);
> -       i2c_for_each_dev(NULL, i2cdev_detach_adapter);
> +       i2c_for_each_dev(NULL, i2c_dev_detach_adapter);
>         class_destroy(i2c_dev_class);
>         unregister_chrdev_region(MKDEV(I2C_MAJOR, 0), I2C_MINORS);
>  }
> --
> 2.34.1
>

Thanks for addressing it.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>