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

[Simon Glass]

Hi Marek,

On Sun, 15 Nov 2020 at 06:19, Marek Vasut <marex at denx.de> wrote:
>
> On 11/9/20 1:21 AM, Simon Glass wrote:
> > Hi Marek,
>
> [...]
>
> >> 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.

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

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

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

Regards,
Simon