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

[Simon Glass <sjg@chromium.org>]

Hi Stefan,

On Tue, 18 Aug 2020 at 08:13, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 18.08.20 15:22, Simon Glass wrote:
> > Hi Marek,
> >
> > +Stefan Roese who wrote the existing code.
>
> Hmmm, I was not Cc'ed.
>
> > On Fri, 7 Aug 2020 at 15:40, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 8/7/20 6:23 PM, Simon Glass wrote:
> >>
> >> [...]
> >>
> >>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> >>>> index efdb0f2905..07b241b6bb 100644
> >>>> --- a/drivers/core/device-remove.c
> >>>> +++ b/drivers/core/device-remove.c
> >>>> @@ -172,14 +172,19 @@ int device_remove(struct udevice *dev, uint flags)
> >>>>          drv = dev->driver;
> >>>>          assert(drv);
> >>>>
> >>>> -       ret = uclass_pre_remove_device(dev);
> >>>> -       if (ret)
> >>>> -               return ret;
> >>>> +       if (!(flags & DM_REMOVE_LATE)) {
> >>>> +               ret = uclass_pre_remove_device(dev);
> >>>> +               if (ret)
> >>>> +                       return ret;
> >>>> +       }
> >>>>
> >>>>          ret = device_chld_remove(dev, NULL, flags);
> >>>>          if (ret)
> >>>>                  goto err;
> >>>>
> >>>> +       if (!(flags & DM_REMOVE_LATE) && (drv->flags & DM_FLAG_REMOVE_LATE))
> >
> > 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?
> >
> > 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)
> >
> > That way we are describing the property of the device rather than what
> > we want to do with it.
> >
> > 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.
> >
> > 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.
> >
> >>>> +               return 0;
> >>>
> >>> Why return here?
> >>
> >> If the DM isn't in a late-remove phase and the driver has remove-late
> >> flag, then the driver should not be removed yet. This is the case for
> >> e.g. a clock driver.
> >>
> >>> Shouldn;'t you use flags_remove() ?
> >>
> >> Please explain ?
> >
> > That function checks the flags so I think you should add your check
> > there rather than adding new code.
> >
> >>
> >>>>          /*
> >>>>           * Remove the device if called with the "normal" remove flag set,
> >>>>           * or if the remove flag matches any of the drivers remove flags
> >>>> diff --git a/drivers/core/root.c b/drivers/core/root.c
> >>>> index 0726be6b79..21f3054125 100644
> >>>> --- a/drivers/core/root.c
> >>>> +++ b/drivers/core/root.c
> >>>> @@ -168,6 +168,7 @@ int dm_init(bool of_live)
> >>>>   int dm_uninit(void)
> >>>>   {
> >>>>          device_remove(dm_root(), DM_REMOVE_NORMAL);
> >>>> +       device_remove(dm_root(), DM_REMOVE_LATE);
> >>>>          device_unbind(dm_root());
> >>>>          gd->dm_root = NULL;
> >>>>
> >>>> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
> >>>>   U_BOOT_DRIVER(root_driver) = {
> >>>>          .name   = "root_driver",
> >>>>          .id     = UCLASS_ROOT,
> >>>> +       .flags  = DM_FLAG_REMOVE_LATE,
> >>>
> >>> Why are you changing this flag for the root device?
> >>
> >> Because root device should only be removed last.
> >
> > OK I see, thanks.
> >
> >>
> >>>>          ACPI_OPS_PTR(&root_acpi_ops)
> >>>>   };
> >>>>
> >>>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> >>>> index c3f1b73cd6..ac474d3ff8 100644
> >>>> --- a/drivers/core/uclass.c
> >>>> +++ b/drivers/core/uclass.c
> >>>> @@ -104,10 +104,28 @@ fail_mem:
> >>>>          return ret;
> >>>>   }
> >>>>
> >>>> -int uclass_destroy(struct uclass *uc)
> >>>> +int uclass_find_device_by_drv_flag(struct uclass *uc, const unsigned int flag,
> >>>> +                                  const bool neg, struct udevice **devp)
> >>>
> >>> Is this intended to be exported? If so, please add a test. If not,
> >>> please make it static. In any case, it needs a full comment as to what
> >>> it does, and args.
> >>
> >> Its internal function, should be static.
> >
> > OK so please add a comment.
> >
> >>
> >>>> +{
> >>>> +       struct udevice *dev;
> >>>> +
> >>>> +       *devp = NULL;
> >>>> +       uclass_foreach_dev(dev, uc) {
> >>>> +               if ((neg && (dev->driver->flags & flag)) ||
> >>>> +                   (!neg && !(dev->driver->flags & flag))) {
> >>>> +                       *devp = dev;
> >>>> +                       return 0;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       return -ENODEV;
> >>>> +}
> >>>> +
> >>>> +int uclass_destroy(struct uclass *uc, unsigned int flag)
> >>>>   {
> >>>>          struct uclass_driver *uc_drv;
> >>>>          struct udevice *dev;
> >>>> +       bool late = flag & DM_REMOVE_LATE;
> >>>>          int ret;
> >>>>
> >>>>          /*
> >>>> @@ -116,15 +134,15 @@ int uclass_destroy(struct uclass *uc)
> >>>>           * unbound (by the recursion in the call to device_unbind() below).
> >>>>           * We can loop until the list is empty.
> >>>>           */
> >>>> -       while (!list_empty(&uc->dev_head)) {
> >>>> -               dev = list_first_entry(&uc->dev_head, struct udevice,
> >>>> -                                      uclass_node);
> >>>> -               ret = device_remove(dev, DM_REMOVE_NORMAL | DM_REMOVE_NO_PD);
> >>>> -               if (ret)
> >>>> +       while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
> >>>> +               ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
> >>>
> >>> What happened to DM_REMOVE_NORMAL?
> >>
> >> See above, bool late, this uclass_destroy() is called twice now. Once
> >> for normal devices, once for late devices, so that the ordering is correct.
> >
> > Actually DM_REMOVE_NO_PD looks broken to me. We have to describe the
> > devices in this function, but it looks like that won't happen as it is
> > written. The docs for uclass_destroy() don't say anything about it
> > sometimes not destroying the uclass.
> >
> > Anyway, hopefully with the changes above, you won't need any changes here.
> >
> >>
> >>>> +               if (ret) {
> >>>>                          return log_msg_ret("remove", ret);
> >>>> +               }
> >>>>                  ret = device_unbind(dev);
> >>>> -               if (ret)
> >>>> +               if (ret) {
> >>>>                          return log_msg_ret("unbind", ret);
> >>>> +               }
> >>>
> >>> Don't need {}
> >>>
> >>>>          }
> >>>>
> >>>>          uc_drv = uc->uc_drv;
> >>>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>>> index 953706cf52..7b1db252bf 100644
> >>>> --- a/include/dm/device.h
> >>>> +++ b/include/dm/device.h
> >>>> @@ -73,6 +73,8 @@ struct driver_info;
> >>>>    */
> >>>>   #define DM_FLAG_REMOVE_WITH_PD_ON      (1 << 13)
> >>>>
> >>>> +#define DM_FLAG_REMOVE_LATE            (1 << 14)
> >>>
> >>> Needs a comment as to what it means.
> >>
> >> It means the driver should be removed after all the non-late drivers.
> >
> > OK, see above. It still needs a comment.
> >
> >>
> >>>> +
> >>>>   /*
> >>>>    * One or multiple of these flags are passed to device_remove() so that
> >>>>    * a selective device removal as specified by the remove-stage and the
> >>>> @@ -95,6 +97,8 @@ enum {
> >>>>
> >>>>          /* Don't power down any attached power domains */
> >>>>          DM_REMOVE_NO_PD         = 1 << 1,
> >>>> +
> >>>> +       DM_REMOVE_LATE          = 1 << 2,
> >>>
> >>> Comment
> >>>
> >>>>   };
> >>>>
> >>>>   /**
> >>>> diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h
> >>>> index 6e3f15c2b0..b5926b0f5c 100644
> >>>> --- a/include/dm/uclass-internal.h
> >>>> +++ b/include/dm/uclass-internal.h
> >>>> @@ -247,8 +247,9 @@ struct uclass *uclass_find(enum uclass_id key);
> >>>>    * Destroy a uclass and all its devices
> >>>>    *
> >>>>    * @uc: uclass to destroy
> >>>> + * @flag: driver flags (DM_REMOVE_NORMAL or DM_REMOVE_LATE)
> >>>>    * @return 0 on success, -ve on error
> >>>>    */
> >>>> -int uclass_destroy(struct uclass *uc);
> >>>> +int uclass_destroy(struct uclass *uc, unsigned int flag);
> >>>
> >>> Why is the flag added here? Does it mean that sometimes it will not
> >>> actually destroy the uclass? It needs a comment.
> >>
> >> Yes, because the drivers in the uclass need to be removed in two steps,
> >> first the ones which can be removed early, and then the rest which need
> >> to be removed late (like clock drivers).
> >
> > It is OK to remove the devices from a uclass in stages, but destroying
> > the uclass should happen once. So far as I understand it, you should
> > not have this flag.
> >
> > Also we need a sandbox test for this new behaviour as it is getting complicated.
> >
> > Stefan, could you perhaps look at a test for the existing flags? We
> > should have some devices with different flags set and check that the
> > correct things happen when calling device_remove() with various flags.
>
> I did send a test at that time. Commit 24f927c527b0 ("dm: test: Add test
> for device removal") adds this test. Is something missing in this test?

Ah yes I thought you did. I missed it, sorry. It is DM_REMOVE_NO_PD
that lacks a test.

Regards,
SImon