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