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

[Simon Glass]

Hi Stefan,

On 19 April 2017 at 03:27, Stefan Roese <sr at denx.de> wrote:
> 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?

I think that sounds reasonable. I don't know this area very well though.

>
>>>
>>> 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?

Yes just a debug() will do. I see what you are saying, I'm just
nervous of dropping error checking since devices really should remove
cleanly. Hopefully as above you can fix the error?

Regards,
Simon