From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Tue, 18 Aug 2020 16:13:24 +0200 Subject: [PATCH V2] dm: core: Add late driver remove option In-Reply-To: References: <20200802150640.114716-1-marek.vasut+renesas@gmail.com> <882acd59-cb1c-1f9d-140f-3cd2d59eda42@gmail.com> Message-ID: <8e9a6704-5e6f-a6f1-cbc4-7aa85f2f8044@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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