| Message ID | 20240205-fix-device-links-overlays-v2-0-5344f8c79d57@analog.com |
|---|---|
| Headers | show |
| Series | fix DT overlays when device links are released | expand |
On Mon, Feb 5, 2024 at 1:09 PM Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > From: Nuno Sa <nuno.sa@analog.com> > > Let's use a dedicated queue for devlinks since releasing a link happens > asynchronously but some code paths, like DT overlays, have some > expectations regarding the of_node when being removed (the refcount must > be 1). Given how devlinks are released that cannot be assured. Hence, add a > dedicated queue so that it's easy to sync against devlinks removal. > > While at it, make sure to explicitly include <linux/workqueue.h>. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> > --- > drivers/base/core.c | 32 ++++++++++++++++++++++++++++---- > include/linux/fwnode.h | 1 + > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 14d46af40f9a..4bb9c10489ed 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -31,6 +31,7 @@ > #include <linux/swiotlb.h> > #include <linux/sysfs.h> > #include <linux/dma-map-ops.h> /* for dma_default_coherent */ > +#include <linux/workqueue.h> > > #include "base.h" > #include "physical_location.h" > @@ -44,6 +45,7 @@ static bool fw_devlink_is_permissive(void); > static void __fw_devlink_link_to_consumers(struct device *dev); > static bool fw_devlink_drv_reg_done; > static bool fw_devlink_best_effort; > +static struct workqueue_struct *devlink_release_queue __ro_after_init; > > /** > * __fwnode_link_add - Create a link between two fwnode_handles. > @@ -235,6 +237,12 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode, > __fw_devlink_pickup_dangling_consumers(child, new_sup); > } > > +void fwnode_links_flush_queue(void) > +{ > + if (devlink_release_queue) > + flush_workqueue(devlink_release_queue); > +} > + > static DEFINE_MUTEX(device_links_lock); > DEFINE_STATIC_SRCU(device_links_srcu); > > @@ -531,9 +539,13 @@ static void devlink_dev_release(struct device *dev) > * It may take a while to complete this work because of the SRCU > * synchronization in device_link_release_fn() and if the consumer or > * supplier devices get deleted when it runs, so put it into the "long" > - * workqueue. > + * devlink workqueue (in case we could allocate one). > + * > */ > - queue_work(system_long_wq, &link->rm_work); > + if (devlink_release_queue) > + queue_work(devlink_release_queue, &link->rm_work); > + else > + device_link_release_fn(&link->rm_work); > } > > static struct class devlink_class = { > @@ -636,10 +648,22 @@ static int __init devlink_class_init(void) > return ret; > > ret = class_interface_register(&devlink_class_intf); > - if (ret) > + if (ret) { > class_unregister(&devlink_class); > + return ret; > + } > > - return ret; > + /* > + * Using a dedicated queue for devlinks since releasing a link happens > + * asynchronously but some code paths, like DT overlays, have some > + * expectations regarding the of_node when being removed (the refcount > + * must be 1). Given how devlinks are released that cannot be assured. > + * Hence, add a dedicated queue so that it's easy to sync against > + * devlinks removal. > + */ > + devlink_release_queue = alloc_workqueue("devlink_release", 0, 0); > + > + return 0; > } > postcore_initcall(devlink_class_init); > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index 2a72f55d26eb..017b170e9903 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -213,5 +213,6 @@ extern bool fw_devlink_is_strict(void); > int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup); > void fwnode_links_purge(struct fwnode_handle *fwnode); > void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode); > +void fwnode_links_flush_queue(void); > > #endif > > -- > 2.43.0 >
On Mon, 2024-02-05 at 14:35 +0200, Andy Shevchenko wrote: > On Mon, Feb 05, 2024 at 01:09:32PM +0100, Nuno Sa via B4 Relay wrote: > > From: Nuno Sa <nuno.sa@analog.com> > > > > Let's use a dedicated queue for devlinks since releasing a link happens > > asynchronously but some code paths, like DT overlays, have some > > expectations regarding the of_node when being removed (the refcount must > > be 1). Given how devlinks are released that cannot be assured. Hence, add a > > dedicated queue so that it's easy to sync against devlinks removal. > > > > While at it, make sure to explicitly include <linux/workqueue.h>. > > ... > > > +++ b/include/linux/fwnode.h > > @@ -213,5 +213,6 @@ extern bool fw_devlink_is_strict(void); > > int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup); > > void fwnode_links_purge(struct fwnode_handle *fwnode); > > void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode); > > +void fwnode_links_flush_queue(void); > > I am not sure if you have seen my comment against v1. > I did received it like 30min ago... > I find the namespace a bit messy for devlinks. And to me seems the best place > for this line is to be before fwnode_links_purge(). > TBH, I'm not really keen on sending a v3 just for that (unless I'm asked otherwise). But If I have (still missing DT guys feedback), I'll do as you suggested. - Nuno Sá
Link to RFC: * https://lore.kernel.org/lkml/20240123-fix-device-links-overlays-v1-1-9e4f6acaab6c@analog.com/ v1: * https://lore.kernel.org/r/20240202-fix-device-links-overlays-v1-0-f9fd1404c8e2@analog.com Changes in v2: * Don't error out in case alloc_workqueue() fails. Devlinks can still work and we'll then release them synchronously. I'm pasting again the link of the first time I exposed the issue where one can see the resulps (big splat) of failing DT assumption: https://lore.kernel.org/linux-devicetree/20230511151047.1779841-1-nuno.sa@analog.com/ --- Nuno Sa (2): driver: core: add dedicated workqueue for devlink removal of: dynamic: flush devlinks workqueue before destroying the changeset drivers/base/core.c | 32 ++++++++++++++++++++++++++++---- drivers/of/dynamic.c | 8 ++++++++ include/linux/fwnode.h | 1 + 3 files changed, 37 insertions(+), 4 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240123-fix-device-links-overlays-5422e033a09b -- Thanks! - Nuno Sá