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

[Simon Glass]

Hi Stefan,

On 2 May 2017 at 05:45, Stefan Roese <sr at denx.de> wrote:
> Hi Simon,
>
>
> On 02.05.2017 13:27, Simon Glass wrote:
>>
>> On 1 May 2017 at 00:14, Stefan Roese <sr at denx.de> wrote:
>>>
>>> Hi Simon,
>>>
>>>
>>> On 29.04.2017 02:26, Simon Glass wrote:
>>>>
>>>>
>>>> On 24 April 2017 at 01:48, 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.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>>>> ---
>>>>> v2:
>>>>> - Add debug() output in error case
>>>>>
>>>>>  drivers/core/device-remove.c | 10 ++++++++--
>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>>
>>>> I thought that your change to force removal of the stdio dev would
>>>> make this change unnecessary?
>>>
>>>
>>>
>>> Yes, the force removal made this change unnecessary in this specific
>>> case. But...
>>>
>>>> I really would rather fix the root cause if we can.
>>>
>>>
>>>
>>> ... the current implementation to exit the loop over all children
>>> upon error and not remove the remaining children is wrong IMO. All
>>> devices should at least be tried to get removed, even if one fails
>>> to get removed. This is what this patch makes sure of.
>>
>>
>> Yes I see that, but not being able to remove is actually an error. In
>> the normal course of events, a device that will not remove itself is
>> likely buggy.
>
>
> Isn't it enough then to just print an error message in this case
> in this loop - change debug() to printf() in this current patch
> version? Then "users" of this code will be aware of such remove
> failures and can take appropriate actions (fix bug etc in their
> setup).

Possibly, but programmatically it becomes impossible to detect a
failure. Say the USB fails to stop its DMA, we might want to reboot
rather than continue to boot Linux and crash.

>
>> What do you think about adding a new remove flag to indicate that
>> failures should be skipped?
>
>
> I'm a bit afraid that this makes the code overly complex. But if
> you prefer to have it this way, then I can come up with such a
> version as well. Just let me know.

I don't think the complexity is too great. It does need a new test.
But I think we should hold the line on error checking even with
remove(), by default.

>
> Thanks,
> Stefan

Regards,
Simon