[PATCH v2] dm: core: Do not stop uclass iteration on error.

[Simon Glass]

Hi Michal,

On Wed, 17 Aug 2022 at 02:28, Michal Suchanek <msuchanek at suse.de> wrote:
>
> When probing a device fails NULL pointer is returned, and other devices
> cannot be iterated. Skip to next device on error instead.
>
> Fixes: 6494d708bf ("dm: Add base driver model support")
> Signed-off-by: Michal Suchanek <msuchanek at suse.de>
> ---
> v2: Fix up tests
>
> Note: there is seemingly bogus repeated device_remove(parent,
DM_REMOVE_NORMAL);
> but I have no idea what the intent was, and fixing that is out of the
> scope of this patch anyway.

This is to remove child devices that have been probed, so that we get back
to the original state.

>
>  drivers/core/uclass.c | 30 +++++++++++++++++++++---------
>  test/dm/test-fdt.c    | 20 ++++++++++++++++----
>  2 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index 08d9ed82de..ccf7d59141 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -574,16 +574,31 @@ int uclass_get_device_by_phandle(enum uclass_id id,
struct udevice *parent,
>  }
>  #endif
>
> +/* Starting from the given device return first device in the uclass that
probes successfully */
> +static int __uclass_next_device(struct udevice *dev, int ret, struct
udevice **devp)

Can you avoid __ as this is reserved for compiler. Perhaps use a single
underscore?

Please check 80cols

> +{
> +       if (!dev) {
> +               *devp = dev;
> +               return 0;
> +       }

Is this for people that call next after they shouldn't?

> +       while ((ret = uclass_get_device_tail(dev, ret, devp))) {
> +               ret = uclass_find_next_device(&dev);
> +               if (!dev) {
> +                       *devp = dev;
> +                       return 0;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
>  int uclass_first_device(enum uclass_id id, struct udevice **devp)
>  {
> -       struct udevice *dev;
> +       struct udevice *dev = NULL;

Can you drop the NULL assignment? uclass_find_first_device() sets dev to
NULL anyway, as a first step.

>         int ret;
>
> -       *devp = NULL;

Is this safe to remove? If there is nothing, then

>         ret = uclass_find_first_device(id, &dev);
> -       if (!dev)
> -               return 0;
> -       return uclass_get_device_tail(dev, ret, devp);
> +       return __uclass_next_device(dev, ret, devp);
>  }
>
>  int uclass_first_device_err(enum uclass_id id, struct udevice **devp)
> @@ -604,11 +619,8 @@ int uclass_next_device(struct udevice **devp)
>         struct udevice *dev = *devp;
>         int ret;
>
> -       *devp = NULL;
>         ret = uclass_find_next_device(&dev);
> -       if (!dev)
> -               return 0;
> -       return uclass_get_device_tail(dev, ret, devp);
> +       return __uclass_next_device(dev, ret, devp);
>  }

This is a major change in behaviour, so please do update the API docs at
dm/uclass.h

>
>  int uclass_next_device_err(struct udevice **devp)
> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> index 6118ad42ca..165b4f5554 100644
> --- a/test/dm/test-fdt.c
> +++ b/test/dm/test-fdt.c
> @@ -417,16 +417,28 @@ static int dm_test_first_next_device(struct
unit_test_state *uts)
>         pdata = dev_get_plat(dev);
>         pdata->probe_err = -ENOMEM;
>         device_remove(parent, DM_REMOVE_NORMAL);
> -       ut_assertok(uclass_first_device(UCLASS_TEST_PROBE, &dev));
> -       ut_asserteq(-ENOMEM, uclass_next_device(&dev));
> -       ut_asserteq_ptr(dev, NULL);
> +       for (ret = uclass_first_device(UCLASS_TEST_PROBE, &dev), count =
0;
> +            dev;
> +            ret = uclass_next_device(&dev)) {
> +               count++;
> +               parent = dev_get_parent(dev);
> +               }
> +       ut_assertok(ret);
> +       ut_asserteq(3, count);
>
>         /* Now an error on the first one */
>         ut_assertok(uclass_get_device(UCLASS_TEST_PROBE, 0, &dev));
>         pdata = dev_get_plat(dev);
>         pdata->probe_err = -ENOENT;
>         device_remove(parent, DM_REMOVE_NORMAL);
> -       ut_asserteq(-ENOENT, uclass_first_device(UCLASS_TEST_PROBE,
&dev));
> +       for (ret = uclass_first_device(UCLASS_TEST_PROBE, &dev), count =
0;
> +            dev;
> +            ret = uclass_next_device(&dev)) {
> +               count++;
> +               parent = dev_get_parent(dev);
> +               }
> +       ut_assertok(ret);
> +       ut_asserteq(2, count);
>
>         return 0;
>  }
> --
> 2.37.1
>

Regards,
Simon