On Tue, 27 Sept 2022 at 15:38, Michal Suchanek <msucha...@suse.de> wrote:
>
> When probing a device fails NULL pointer is returned, and following
> devices in uclass list cannot be iterated. Skip to next device on error
> instead.
>
> With that the only condition under which these simple iteration
> functions return error is when the dm is not initialized at uclass_get
> time. This is not all that interesting, change return type to void.
>
> Fixes: 6494d708bf ("dm: Add base driver model support")
> Signed-off-by: Michal Suchanek <msucha...@suse.de>
> ---
> v2: - Fix up tests
> v3: - Fix up API doc
> - Correctly forward error from uclass_get
> - Do not return an error when last device fails to probe
> - Drop redundant initialization
> - Wrap at 80 columns
> v4: - change return value to void
> - further simplify iteration
> ---
> drivers/core/uclass.c | 30 ++++++++++++++++++------------
> include/dm/uclass.h | 13 ++++++-------
> test/dm/core.c | 10 ++++------
> test/dm/test-fdt.c | 27 ++++++++++++++++++++-------
> 4 files changed, 48 insertions(+), 32 deletions(-)
>
Reviewed-by: Simon Glass <s...@chromium.org>
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index b7d11bdd23..6dec6a3973 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -574,28 +574,34 @@ int uclass_get_device_by_phandle(enum uclass_id id,
> struct udevice *parent,
> }
> #endif
>
> -int uclass_first_device(enum uclass_id id, struct udevice **devp)
> +/*
> + * Starting from the given device return first device in the uclass that
device, return the first
> + * probes successfully.
Please describe the args here.
> + */
> +static void _uclass_next_device(struct udevice *dev, struct udevice **devp)
> +{
> + for (; dev; uclass_find_next_device(&dev)) {
> + if (!device_probe(dev))
> + break;
> + }
> + *devp = dev;
> +}
> +
> +void uclass_first_device(enum uclass_id id, struct udevice **devp)
> {
> struct udevice *dev;
> int ret;
>
> - *devp = NULL;
> ret = uclass_find_first_device(id, &dev);
> - if (!dev)
> - return 0;
> - return uclass_get_device_tail(dev, ret, devp);
> + _uclass_next_device(dev, devp);
> }
>
> -int uclass_next_device(struct udevice **devp)
> +void 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);
> + uclass_find_next_device(&dev);
> + _uclass_next_device(dev, devp);
> }
>
> int uclass_first_device_err(enum uclass_id id, struct udevice **devp)
> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index b1c016ef9f..ee15c92063 100644
> --- a/include/dm/uclass.h
> +++ b/include/dm/uclass.h
> @@ -320,32 +320,31 @@ int uclass_get_device_by_driver(enum uclass_id id,
> const struct driver *drv,
> * uclass_first_device() - Get the first device in a uclass
> *
> * The device returned is probed if necessary, and ready for use
> + * Devices that fail to probe are skipped
> *
> * This function is useful to start iterating through a list of devices which
> * are functioning correctly and can be probed.
> *
> * @id: Uclass ID to look up
> * @devp: Returns pointer to the first device in that uclass if no error
> - * occurred, or NULL if there is no first device, or an error occurred with
> - * that device.
> - * Return: 0 if OK (found or not found), other -ve on error
> + * occurred, or NULL if there is no usable device
> */
> -int uclass_first_device(enum uclass_id id, struct udevice **devp);
> +void uclass_first_device(enum uclass_id id, struct udevice **devp);
>
> /**
> * uclass_next_device() - Get the next device in a uclass
> *
> * The device returned is probed if necessary, and ready for use
> + * Devices that fail to probe are skipped
> *
> * This function is useful to iterate through a list of devices which
> * are functioning correctly and can be probed.
> *
> * @devp: On entry, pointer to device to lookup. On exit, returns pointer
> * to the next device in the uclass if no error occurred, or NULL if there is
> - * no next device, or an error occurred with that next device.
> - * Return: 0 if OK (found or not found), other -ve on error
> + * no next device
> */
> -int uclass_next_device(struct udevice **devp);
> +void uclass_next_device(struct udevice **devp);
>
> /**
> * uclass_first_device_err() - Get the first device in a uclass
> diff --git a/test/dm/core.c b/test/dm/core.c
> index 84eb76ed5f..7f3f8d183b 100644
> --- a/test/dm/core.c
> +++ b/test/dm/core.c
> @@ -1078,11 +1078,10 @@ static int dm_test_uclass_devices_get(struct
> unit_test_state *uts)
> struct udevice *dev;
> int ret;
>
> - for (ret = uclass_first_device(UCLASS_TEST, &dev);
> + for (ret = uclass_first_device_check(UCLASS_TEST, &dev);
> dev;
> - ret = uclass_next_device(&dev)) {
> + ret = uclass_next_device_check(&dev)) {
> ut_assert(!ret);
> - ut_assert(dev);
> ut_assert(device_active(dev));
> }
>
> @@ -1112,11 +1111,10 @@ static int dm_test_uclass_devices_get_by_name(struct
> unit_test_state *uts)
> * this will fail on checking condition: testdev == finddev, since the
> * uclass_get_device_by_name(), returns the first device by given
> name.
> */
> - for (ret = uclass_first_device(UCLASS_TEST_FDT, &testdev);
> + for (ret = uclass_first_device_check(UCLASS_TEST_FDT, &testdev);
> testdev;
> - ret = uclass_next_device(&testdev)) {
> + ret = uclass_next_device_check(&testdev)) {
> ut_assertok(ret);
> - ut_assert(testdev);
> ut_assert(device_active(testdev));
>
> findret = uclass_get_device_by_name(UCLASS_TEST_FDT,
> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> index 6118ad42ca..7cbc595c51 100644
> --- a/test/dm/test-fdt.c
> +++ b/test/dm/test-fdt.c
> @@ -403,13 +403,12 @@ static int dm_test_first_next_device(struct
> unit_test_state *uts)
> int ret;
>
> /* There should be 4 devices */
> - for (ret = uclass_first_device(UCLASS_TEST_PROBE, &dev), count = 0;
> + for (uclass_first_device(UCLASS_TEST_PROBE, &dev), count = 0;
> dev;
> - ret = uclass_next_device(&dev)) {
> + uclass_next_device(&dev)) {
> count++;
> parent = dev_get_parent(dev);
> }
> - ut_assertok(ret);
> ut_asserteq(4, count);
>
> /* Remove them and try again, with an error on the second one */
> @@ -417,16 +416,30 @@ 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_check(UCLASS_TEST_PROBE, &dev),
> + count = 0;
> + dev;
> + ret = uclass_next_device_check(&dev)) {
> + if (!ret)
> + count++;
> + else
> + ut_asserteq(-ENOMEM, ret);
> + parent = dev_get_parent(dev);
> + }
> + 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 (uclass_first_device(UCLASS_TEST_PROBE, &dev), count = 0;
> + dev;
> + uclass_next_device(&dev)) {
> + count++;
> + parent = dev_get_parent(dev);
> + }
> + ut_asserteq(2, count);
>
> return 0;
> }
> --
> 2.37.3
>
