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

[Simon Glass]

Hi Michal,

On Tue, 30 Aug 2022 at 04:23, Michal Suchánek <msuchanek at suse.de> wrote:
>
> On Sat, Aug 27, 2022 at 07:52:27PM -0600, Simon Glass wrote:
> > Hi Michal,
> >
> > On Fri, 19 Aug 2022 at 14:23, 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")
> >
> > I think you should drop this as you are doing a change of behaviour,
> > not fixing a bug!
>
> You can hardly fix a bug without a change in behavior.
>
> These functions are used for iterating devices, and are not iterating
> devices. That's clearly a bug.

If it were clear I would have changed this long ago. The new way you
have this function ignores errors, so they cannot be reported.

We should almost always report errors, which is why I think your
methods should be named differently.

>
> > > Signed-off-by: Michal Suchanek <msuchanek at 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
> > > ---
> > >  drivers/core/uclass.c | 32 ++++++++++++++++++++++++--------
> > >  include/dm/uclass.h   | 13 ++++++++-----
> > >  test/dm/test-fdt.c    | 20 ++++++++++++++++----
> > >  3 files changed, 48 insertions(+), 17 deletions(-)
> >
> > Unfortunately this still fails one test. Try 'make qcheck' to see it -
> > it is ethernet.
>
> I will look at that.
>
> > I actually think you should create new functions for this feature,
> > e.g.uclass_first_device_ok(), since it makes it impossible to see what
> > when wrong with a device in the middle.
> >
> > I have long had all this in my mind. One idea for a future change is
> > to return the error, but set dev, so that the caller knows there is a
> > device, which failed. When we are at the end, dev is set to NULL.
>
> We already have uclass_first_device_check() and
> uclass_next_device_check() to iterate all devices, including broken
> ones, and getting the errors as well.
>
> That's for the case you want all the details, and these are for the case
> you just want to get devices and don't care about the details.
>
> That's AFAICT as much as this iteration interface can provide, and we
> have both cases covered.

I see three cases:
- want to see the next device, returning the error if it cannot be
probed - uclass_first_device()
- want to get the next device that can successfully probe - your new functions
- want to see each device, along with any errors - uclass_first_device_check()

Regards,
Simon