[U-Boot] [PATCH 1/4] dm: device_remove: Don't return in device_chld_remove() upon error

[Stefan Roese]

Hi Simon,

sorry for the late replay - just back from vacation....

On 09.04.2017 21:28, Simon Glass wrote:
> Hi Stefan,
>
> On 6 April 2017 at 07:29, Stefan Roese <sr at denx.de> wrote:
>> On my x86 platform I've noticed, that calling dm_uninit() or the new
>> function dm_remove_devices_flags() does not remove the desired device at
>> all. Debugging showed, that the serial uclass returns -EPERM in
>> serial_pre_remove() and this leads to a complete stop of the device
>> removal pretty early, as the serial device is one of the first ones in
>> the DM. Here the dm tree output:
>>
>> => dm tree
>>  Class       Probed   Name
>> ----------------------------------------
>>  root        [ + ]    root_driver
>>  rsa_mod_exp [   ]    |-- mod_exp_sw
>>  serial      [ + ]    |-- serial
>>  rtc         [   ]    |-- rtc
>>  timer       [ + ]    |-- tsc-timer
>>  syscon      [ + ]    |-- pch_pinctrl
>>  ...
>>
>> In this example, device_remove(root) will stop directly after trying to
>> remove the "serial" device.
>>
>> To solve this problem, this patch removes the return upon error check in
>> the device_remove() call in device_chld_remove(). This leads to
>> device_chld_remove() continuing with the device_remove() call to the
>> following child devices.
>
> I think the right solution is to find out why stdio_deregister_dev()
> fails. It is probably because the device is in use within the stdio
> variables.

This is most likely the case, yes.

> Perhaps you need to remove it first?

Not sure if this should / could be done in this general case of
removal of all devices (or all devices matching a remove-flag)?
Please think of dm_uninit() being called. This functions should
have no internal knowledge of usage of devices. One thing I could
do, probably best in a separate patch, is to use the force flag
of stdio_deregister_dev() in serial_pre_remove() to force the
serial device(s) to be removed in this case. What do you think?

>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Bin Meng <bmeng.cn at gmail.com>
>> ---
>>  drivers/core/device-remove.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index cc0043b990..8b46f3343e 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
>>  static int device_chld_remove(struct udevice *dev, uint flags)
>>  {
>>         struct udevice *pos, *n;
>> -       int ret;
>>
>>         assert(dev);
>>
>> -       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
>> -               ret = device_remove(pos, flags);
>> -               if (ret)
>> -                       return ret;
>> -       }
>> +       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
>> +               device_remove(pos, flags);
>
> I think we should keep the error checking here.

The main fix with this patch is, that removing of child devices does
not stop once an error is encountered. Even if an error occurs with
one child-device of a parent, all other child-devices of this
parent should still be removed - or at least tried to.

So what does this error code buy us here? Should I print a log message
here in this error case?

Thanks,
Stefan