[PATCH V2] dm: core: Add late driver remove option
Simon Glass
sjg at chromium.org
Tue Aug 18 15:22:06 CEST 2020
Hi Marek,
+Stefan Roese who wrote the existing code.
On Fri, 7 Aug 2020 at 15:40, Marek Vasut <marek.vasut at 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.
Regards,
SImon
More information about the U-Boot
mailing list