dm: core: don't fail to iterate if first one fails to probe

efi_disk_register() would try to iterate all the blk devices. But if the first one in the list failed to probe, uclass_first_device() would return NULL and no attempt would be made to register the remaining devices. Also uclass_next_device() needs the same fix.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/core/uclass.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

drivers/core/uclass.c +21 -4

index 21dc696..c47ff56 100644
@@ -458,14 +458,23 @@ int uclass_get_device_by_phandle(enum uclass_id id, struct udevice *parent,
 
 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);
+       ret = uclass_get_device_tail(dev, ret, devp);
+       if (ret && dev) {
+               /* we have a device, but it failed to probe, move on and
+                * try the next one.
+                */
+               ret = uclass_next_device(&dev);
+               if (!ret)
+                       *devp = dev;
+       }
+       return ret;
 }
 
 int uclass_first_device_err(enum uclass_id id, struct udevice **devp)

@@ -490,7 +499,16 @@ int uclass_next_device(struct udevice **devp)
        ret = uclass_find_next_device(&dev);
        if (!dev)
                return 0;
-       return uclass_get_device_tail(dev, ret, devp);
+       ret = uclass_get_device_tail(dev, ret, devp);
+       if (ret && (dev != *devp)) {
+               /* we have a device, but it failed to probe, move on and
+                * try the next one.
+                */
+               ret = uclass_next_device(&dev);
+               if (!ret)
+                       *devp = dev;
+       }
+       return ret;
 }
 
 int uclass_bind_device(struct udevice *dev)
--

2.9.4

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Quoted 4 lines · Rob Clark

efi_disk_register() would try to iterate all the blk devices. But if the first one in the list failed to probe, uclass_first_device() would return NULL and no attempt would be made to register the remaining devices. Also uclass_next_device() needs the same fix.

This looks related/similar to the "efi_loader: disk: iterate only over valid block devices" patch [1]

Peter

[1] https://lists.denx.de/pipermail/u-boot/2017-June/296015.html

Quoted 60 lines

Signed-off-by Rob Clark <[email protected]>

---
 drivers/core/uclass.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

drivers/core/uclass.c +21 -4

index 21dc696..c47ff56 100644
@@ -458,14 +458,23 @@ int uclass_get_device_by_phandle(enum uclass_id id, struct udevice *parent,
 
 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);
+       ret = uclass_get_device_tail(dev, ret, devp);
+       if (ret && dev) {
+               /* we have a device, but it failed to probe, move on and
+                * try the next one.
+                */
+               ret = uclass_next_device(&dev);
+               if (!ret)
+                       *devp = dev;
+       }
+       return ret;
 }
 
 int uclass_first_device_err(enum uclass_id id, struct udevice **devp)

@@ -490,7 +499,16 @@ int uclass_next_device(struct udevice **devp)
        ret = uclass_find_next_device(&dev);
        if (!dev)
                return 0;
-       return uclass_get_device_tail(dev, ret, devp);
+       ret = uclass_get_device_tail(dev, ret, devp);
+       if (ret && (dev != *devp)) {
+               /* we have a device, but it failed to probe, move on and
+                * try the next one.
+                */
+               ret = uclass_next_device(&dev);
+               if (!ret)
+                       *devp = dev;
+       }
+       return ret;
 }
 
 int uclass_bind_device(struct udevice *dev)
--

2.9.4

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Quoted 9 lines · Peter Robinson

Quoted 4 lines · Rob Clark

efi_disk_register() would try to iterate all the blk devices. But if the first one in the list failed to probe, uclass_first_device() would return NULL and no attempt would be made to register the remaining devices. Also uclass_next_device() needs the same fix.

This looks related/similar to the "efi_loader: disk: iterate only over valid block devices" patch [1]

hmm, I don't see uclass_first_device_check() which I guess must be part of another in-flight patch? I don't mind too much *how* we fix it, as long as it works. (Although it seems more sensible to just make uclass_first_device()/etc dtrt rather than introducing another function that actually works properly.. but that is only my $0.02)

Hi Rob,

Quoted 11 lines · Rob Clark

Quoted 9 lines · Peter Robinson

Quoted 4 lines · Rob Clark

efi_disk_register() would try to iterate all the blk devices. But if the first one in the list failed to probe, uclass_first_device() would return NULL and no attempt would be made to register the remaining devices. Also uclass_next_device() needs the same fix.

This looks related/similar to the "efi_loader: disk: iterate only over valid block devices" patch [1]

Quoted 5 lines

hmm, I don't see uclass_first_device_check() which I guess must be part of another in-flight patch? I don't mind too much *how* we fix it, as long as it works. (Although it seems more sensible to just make uclass_first_device()/etc dtrt rather than introducing another function that actually works properly.. but that is only my $0.02)

That patch is now in mainline. It was delayed for a release as I did not get any reviews and so wasn't sure if it was needed.

I believe the common case is wanting to stop on error, since it means something is wrong. The case of continuing onto another device when the first one fails is unusual. We should always check return codes and return errors when something is wrong.

I believe this case is happening because the device is removable and therefore returns an error. Is that right? If so then we should return a single error, such as -ENOMEDIUM so that the caller can ignore that error, or consider it OK.

In general, though, errors should not be ignored.

Regards,
Simon

[...]

Quoted 28 lines · Simon Glass

Hi Rob,

Quoted 11 lines · Rob Clark

Quoted 9 lines · Peter Robinson

Quoted 4 lines · Rob Clark

efi_disk_register() would try to iterate all the blk devices. But if the first one in the list failed to probe, uclass_first_device() would return NULL and no attempt would be made to register the remaining devices. Also uclass_next_device() needs the same fix.

This looks related/similar to the "efi_loader: disk: iterate only over valid block devices" patch [1]

Quoted 5 lines

hmm, I don't see uclass_first_device_check() which I guess must be part of another in-flight patch? I don't mind too much *how* we fix it, as long as it works. (Although it seems more sensible to just make uclass_first_device()/etc dtrt rather than introducing another function that actually works properly.. but that is only my $0.02)

That patch is now in mainline. It was delayed for a release as I did not get any reviews and so wasn't sure if it was needed.

I believe the common case is wanting to stop on error, since it means something is wrong. The case of continuing onto another device when the first one fails is unusual. We should always check return codes and return errors when something is wrong.

Quoted 4 lines

I believe this case is happening because the device is removable and therefore returns an error. Is that right? If so then we should return a single error, such as -ENOMEDIUM so that the caller can ignore that error, or consider it OK.

yes, iirc, the first one ended up being sd-card (which was removed).. I'll have to check if driver is properly returning -ENOMEDIUM. In which case maybe the right thing to do is for the iterators skip over just that single error code, since it isn't really an "unusual" error.

Hi Rob,

Quoted 34 lines · Rob Clark

Quoted 32 lines · Simon Glass

Hi Rob,

Quoted 11 lines · Rob Clark

Quoted 9 lines · Peter Robinson

Quoted 4 lines · Rob Clark

efi_disk_register() would try to iterate all the blk devices. But if the first one in the list failed to probe, uclass_first_device() would return NULL and no attempt would be made to register the remaining devices. Also uclass_next_device() needs the same fix.

This looks related/similar to the "efi_loader: disk: iterate only over valid block devices" patch [1]

Quoted 5 lines

hmm, I don't see uclass_first_device_check() which I guess must be part of another in-flight patch? I don't mind too much *how* we fix it, as long as it works. (Although it seems more sensible to just make uclass_first_device()/etc dtrt rather than introducing another function that actually works properly.. but that is only my $0.02)

That patch is now in mainline. It was delayed for a release as I did not get any reviews and so wasn't sure if it was needed.

I believe the common case is wanting to stop on error, since it means something is wrong. The case of continuing onto another device when the first one fails is unusual. We should always check return codes and return errors when something is wrong.

I believe this case is happening because the device is removable and therefore returns an error. Is that right? If so then we should return a single error, such as -ENOMEDIUM so that the caller can ignore that error, or consider it OK.

Quoted 4 lines

yes, iirc, the first one ended up being sd-card (which was removed).. I'll have to check if driver is properly returning -ENOMEDIUM. In which case maybe the right thing to do is for the iterators skip over just that single error code, since it isn't really an "unusual" error.

OK that sounds better. Suppressing errors should be done with care.