[PATCH V3] dm: core: Add late driver remove option

[Marek Vasut]

On 11/18/20 3:37 PM, Simon Glass wrote:

Hi,

[...]

>>>> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
>>>> index 1206e306db..f9091a3d41 100644
>>>> --- a/arch/arm/lib/bootm.c
>>>> +++ b/arch/arm/lib/bootm.c
>>>> @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake)
>>>>            * of DMA operation or releasing device internal buffers.
>>>>            */
>>>>           dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>>>> +       dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);
>>>
>>> Please see my previous comments about the naming and semantics. I'm
>>> repeating them here:
>>>
>>> Firstly I think we should use a different name. 'Late' doesn't really
>>> tell me anything.
>>>
>>> If I understand it correctly, this is supposed to be after OS_PREPARE
>>> but before booting the OS?
>>
>> No. The drivers which are marked as remove-late should be removed late,
>> after all the normal drivers were removed. The example in the commit
>> message explains where this is needed -- first remove mmc driver to set
>> eMMC out of HS mode and change the bus clock and after that remove clock
>> driver ; the clock driver is still required during the removal of the
>> mmc driver.
>>
>>> I think we need to separate the flag names as they are too similar.
>>>
>>> I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some
>>> term that explains that this is a device used by many others)
>>>
>>> If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL
>>> or something like that?
>>
>> This makes no sense to me.
> 
> I don't want the word 'late'. Then we'll end up with 'later' and
> 'fairly late', etc. and it will get out of hand. We need a word that
> describes what is special about the devices, not when to do stuff with
> them.

If there is a need for "later" , then we will need some more complex 
code. I suggest we cross that bridge when we come to it.

>>> That way we are describing the property of the device rather than what
>>> we want to do with it.
>>
>> The device is not critical or vital, it just needs to be torn down late.
> 
> What is it about the device that requires it to be torn down 'late'?

For example clock driver needs to be turn down after mmc controller, 
since the mmc controller might need to reconfigure the card in .remove 
callback, for which it still needs to clock to be active. That's the 
usecase I have on real hardware.

>>> Note also the semantics of what is going on here. The idea of the
>>> existing OS_PREPARE and ACTIVE_DMA flags is that the default for
>>> device_remove() is to remove everything, but if you provide a flag
>>> then it just removes those things. Your flag is the opposite to that,
>>> which is why you are changing so much code.
>>
>> I obviously cannot remove everything, see the example above.
> 
> Do you understand what I am saying about inverting the flag?

No, please elaborate.

>>> So I think we could change this to DM_REMOVE_NON_BASIC and make that a
>>> separate step before we do a final remove with flags of 0.
>>
>> [...]
>>
>>>> @@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts)
>>>>    int dm_leak_check_end(struct unit_test_state *uts)
>>>>    {
>>>>           struct mallinfo end;
>>>> -       int id, diff;
>>>> +       int i, id, diff;
>>>>
>>>>           /* Don't delete the root class, since we started with that */
>>>> -       for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {
>>>> -               struct uclass *uc;
>>>> -
>>>> -               uc = uclass_find(id);
>>>> -               if (!uc)
>>>> -                       continue;
>>>> -               ut_assertok(uclass_destroy(uc));
>>>> +       for (i = 0; i < 2; i++) {
>>>> +               for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {
>>>> +                       struct uclass *uc;
>>>> +
>>>> +                       uc = uclass_find(id);
>>>> +                       if (!uc)
>>>> +                               continue;
>>>> +                       ut_assertok(uclass_destroy(uc,
>>>> +                                   i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));
>>>> +               }
>>>
>>> Why do we need to destroy the classes in two steps? I understand
>>> removing devices, but destroying the uclasses seems like it could stay
>>> as it is?
>>
>> Because if you destroy clock uclass before mmc uclass, then you cannot
>> remove the mmc drivers and reconfigure the bus clock anymore in the
>> remove callback. So that needs to be done in two steps.
> 
> Yes I understand that. All of my comments are about the implementation
> rather than the problem you are solving. See the existing DM_REMOVE_
> flags for examples.
> 
> If you like, I could have a try at this. Perhaps it is not as
> straightforward as I imagine.

I think it would be a good idea if you looked at the use case for this 
first.