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

[Michal Suchánek]

On Thu, Aug 18, 2022 at 11:49:53AM -0600, Simon Glass wrote:
> 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.

Thanks, it makes sense now.

> >
> >  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

whatever

> > +{
> > +       if (!dev) {
> > +               *devp = dev;
> > +               return 0;
> > +       }
> 
> Is this for people that call next after they shouldn't?

This is for the case if there is nothing.

> > +       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

it's the same as uclass_next_device() on the last device.

> 
> >         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

Yes, those need updating as well.

Thanks

Michal