Hi Stefan,
On Tue, 18 Aug 2020 at 08:13, Stefan Roese <s...@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.va...@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
