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

[Simon Glass]

Hi Marek,

On Sat, 5 Dec 2020 at 08:19, Marek Vasut <marex at denx.de> wrote:
>
> 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.
>

I'm OK with that.

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

Yes I understand that.

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

The normal situation should be to remove everything. Removing only
non-late things (which I want to call 'basic', or something like that)
should be an option, like we have DM_REMOVE_OS_PREPARE.

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

I'm not sure what it is about this use case that I don't understand.
It seems fairly straightforward to me. We have decided to leave
reference counting for another day and all I am really commenting on
is the implementation.

Regards,
Simon