[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

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

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/core/uclass.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 21dc696..c47ff56 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -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] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[Peter Robinson]

On Tue, Jun 20, 2017 at 10:49 PM, Rob Clark <robdclark@gmail.com> wrote:
> 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


> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/core/uclass.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index 21dc696..c47ff56 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -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

[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[Rob Clark]

On Wed, Jun 21, 2017 at 6:23 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 10:49 PM, Rob Clark <robdclark@gmail.com> wrote:
>> 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)

BR,
-R


> Peter
>
> [1] https://lists.denx.de/pipermail/u-boot/2017-June/296015.html
>
>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  drivers/core/uclass.c | 24 +++++++++++++++++++++---
>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>> index 21dc696..c47ff56 100644
>> --- a/drivers/core/uclass.c
>> +++ b/drivers/core/uclass.c
>> @@ -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

[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[Simon Glass]

Hi Rob,

On 21 June 2017 at 04:52, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Jun 21, 2017 at 6:23 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 10:49 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> 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)

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

[...]

[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[Rob Clark]

On Thu, Jul 13, 2017 at 3:10 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Rob,
>
> On 21 June 2017 at 04:52, Rob Clark <robdclark@gmail.com> wrote:
>> On Wed, Jun 21, 2017 at 6:23 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
>>> On Tue, Jun 20, 2017 at 10:49 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>> 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)
>
> 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.

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.

BR,
-R

> In general, though, errors should not be ignored.
>
> Regards,
> Simon
>
> [...]

[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[Simon Glass]

Hi Rob,

On 19 July 2017 at 10:53, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Jul 13, 2017 at 3:10 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Rob,
>>
>> On 21 June 2017 at 04:52, Rob Clark <robdclark@gmail.com> wrote:
>>> On Wed, Jun 21, 2017 at 6:23 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
>>>> On Tue, Jun 20, 2017 at 10:49 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>>> 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)
>>
>> 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.
>
> 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.

Regards,
Simon

[U-Boot] [PATCH] dm: core: don't fail to iterate if first one fails to probe

[Rob Clark]

On Thu, Aug 3, 2017 at 11:24 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Rob,
>
> On 19 July 2017 at 10:53, Rob Clark <robdclark@gmail.com> wrote:
>> On Thu, Jul 13, 2017 at 3:10 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Rob,
>>>
>>> On 21 June 2017 at 04:52, Rob Clark <robdclark@gmail.com> wrote:
>>>> On Wed, Jun 21, 2017 at 6:23 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
>>>>> On Tue, Jun 20, 2017 at 10:49 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>>>> 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)
>>>
>>> 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.
>>
>> 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.
>

fwiw, efi_loader in the mean time switched to
uclass_first_device_check().. so I guess we don't really need this
patch.  (I've dropped it for now since my stack of WIP patches is
pretty big already.)

BR,
-R