[PATCH V2] dm: core: Add late driver remove option
Stefan Roese
sr at denx.de
Tue Aug 18 16:13:24 CEST 2020
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 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.
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?
Thanks,
Stefan
More information about the U-Boot
mailing list