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

["Michal Suchánek" <msuchanek@suse.de>]

Hello,

On Thu, Aug 04, 2022 at 01:22:57PM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Thu, 4 Aug 2022 at 11:59, Michal Suchanek <msuchanek@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@suse.de>
> > ---
> >  drivers/core/uclass.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> Are you able to create a test for this, e.g. in test/dm/core.c or test-fdt.c ?
> 
> Some sandbox devices can be made to give an error, so the test can
> check the logic.
> 
> Strangely enough, this is actually a very big change, so we need to be careful.

Right, I can have a look at doing some tests.

Knowing what the problem is designig a test should not be too difficult.

It does not make the change any smaller but it can at least prevent
regressions.

Not sure when I get to it, though.

Thanks

Michal
> 
> >
> > 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)
> > +{
> > +       if (!dev) {
> > +               *devp = dev;
> > +               return 0;
> > +       }
> > +       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;
> >         int ret;
> >
> > -       *devp = NULL;
> >         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);
> >  }
> >
> >  int uclass_next_device_err(struct udevice **devp)
> > --
> > 2.37.1
> >
> 
> Regards,
> Simon