On Thu, 9 Mar 2023 at 08:45, Geert Uytterhoeven <geert+renesas@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
