The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
introduces a workqueue to release the consumer and supplier devices used
in the devlink.
In the job queued, devices are release and in turn, when all the
references to these devices are dropped, the release function of the
device itself is called.
Nothing is present to provide some synchronisation with this workqueue
in order to ensure that all ongoing releasing operations are done and
so, some other operations can be started safely.
For instance, in the following sequence:
1) of_platform_depopulate()
2) of_overlay_remove()
During the step 1, devices are released and related devlinks are removed
(jobs pushed in the workqueue).
During the step 2, OF nodes are destroyed but, without any
synchronisation with devlink removal jobs, of_overlay_remove() can raise
warnings related to missing of_node_put():
ERROR: memory leak, expected refcount 1 instead of 2
Indeed, the missing of_node_put() call is going to be done, too late,
from the workqueue job execution.
Introduce device_link_wait_removal() to offer a way to synchronize
operations waiting for the end of devlink removals (i.e. end of
workqueue jobs).
Also, as a flushing operation is done on the workqueue, the workqueue
used is moved from a system-wide workqueue to a local one.
Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
Cc: [email protected]
Signed-off-by: Herve Codina <[email protected]>
---
drivers/base/core.c | 26 +++++++++++++++++++++++---
include/linux/device.h | 1 +
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index d5f4e4aac09b..48b28c59c592 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,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 *device_link_wq;
/**
* __fwnode_link_add - Create a link between two fwnode_handles.
@@ -532,12 +533,26 @@ 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.
+ * supplier devices get deleted when it runs, so put it into the
+ * dedicated workqueue.
*/
- queue_work(system_long_wq, &link->rm_work);
+ queue_work(device_link_wq, &link->rm_work);
}
+/**
+ * device_link_wait_removal - Wait for ongoing devlink removal jobs to terminate
+ */
+void device_link_wait_removal(void)
+{
+ /*
+ * devlink removal jobs are queued in the dedicated work queue.
+ * To be sure that all removal jobs are terminated, ensure that any
+ * scheduled work has run to completion.
+ */
+ flush_workqueue(device_link_wq);
+}
+EXPORT_SYMBOL_GPL(device_link_wait_removal);
+
static struct class devlink_class = {
.name = "devlink",
.dev_groups = devlink_groups,
@@ -4099,9 +4114,14 @@ int __init devices_init(void)
sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
if (!sysfs_dev_char_kobj)
goto char_kobj_err;
+ device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
+ if (!device_link_wq)
+ goto wq_err;
return 0;
+ wq_err:
+ kobject_put(sysfs_dev_char_kobj);
char_kobj_err:
kobject_put(sysfs_dev_block_kobj);
block_kobj_err:
diff --git a/include/linux/device.h b/include/linux/device.h
index 1795121dee9a..d7d8305a72e8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1249,6 +1249,7 @@ void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+void device_link_wait_removal(void);
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
--
2.43.0
On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> introduces a workqueue to release the consumer and supplier devices used
> in the devlink.
> In the job queued, devices are release and in turn, when all the
> references to these devices are dropped, the release function of the
> device itself is called.
>
> Nothing is present to provide some synchronisation with this workqueue
> in order to ensure that all ongoing releasing operations are done and
> so, some other operations can be started safely.
>
> For instance, in the following sequence:
> 1) of_platform_depopulate()
> 2) of_overlay_remove()
>
> During the step 1, devices are released and related devlinks are removed
> (jobs pushed in the workqueue).
> During the step 2, OF nodes are destroyed but, without any
> synchronisation with devlink removal jobs, of_overlay_remove() can raise
> warnings related to missing of_node_put():
> ERROR: memory leak, expected refcount 1 instead of 2
>
> Indeed, the missing of_node_put() call is going to be done, too late,
> from the workqueue job execution.
>
> Introduce device_link_wait_removal() to offer a way to synchronize
> operations waiting for the end of devlink removals (i.e. end of
> workqueue jobs).
> Also, as a flushing operation is done on the workqueue, the workqueue
> used is moved from a system-wide workqueue to a local one.
>
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: [email protected]
> Signed-off-by: Herve Codina <[email protected]>
> ---
With the below addressed:
Reviewed-by: Nuno Sa <[email protected]>
> drivers/base/core.c | 26 +++++++++++++++++++++++---
> include/linux/device.h | 1 +
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d5f4e4aac09b..48b28c59c592 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -44,6 +44,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 *device_link_wq;
>
> /**
> * __fwnode_link_add - Create a link between two fwnode_handles.
> @@ -532,12 +533,26 @@ 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.
> + * supplier devices get deleted when it runs, so put it into the
> + * dedicated workqueue.
> */
> - queue_work(system_long_wq, &link->rm_work);
> + queue_work(device_link_wq, &link->rm_work);
> }
>
> +/**
> + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> terminate
> + */
> +void device_link_wait_removal(void)
> +{
> + /*
> + * devlink removal jobs are queued in the dedicated work queue.
> + * To be sure that all removal jobs are terminated, ensure that any
> + * scheduled work has run to completion.
> + */
> + flush_workqueue(device_link_wq);
> +}
> +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> +
> static struct class devlink_class = {
> .name = "devlink",
> .dev_groups = devlink_groups,
> @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> if (!sysfs_dev_char_kobj)
> goto char_kobj_err;
> + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> + if (!device_link_wq)
> + goto wq_err;
>
I can't still agree with this. Why not doing it in devlink_class_init()? This is
devlink specific so it makes complete sense to me.
Note that this maybe the 3/4 time I'm arguing in here. If you don't agree please
tell me why and I may agree with you :).
(and sorry if you already said something about it and I missed)
- Nuno Sá
>
On Wed, 6 Mar 2024 09:50:02 +0100
Herve Codina <[email protected]> wrote:
> The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> introduces a workqueue to release the consumer and supplier devices used
> in the devlink.
> In the job queued, devices are release and in turn, when all the
> references to these devices are dropped, the release function of the
> device itself is called.
>
> Nothing is present to provide some synchronisation with this workqueue
> in order to ensure that all ongoing releasing operations are done and
> so, some other operations can be started safely.
>
> For instance, in the following sequence:
> 1) of_platform_depopulate()
> 2) of_overlay_remove()
>
> During the step 1, devices are released and related devlinks are removed
> (jobs pushed in the workqueue).
> During the step 2, OF nodes are destroyed but, without any
> synchronisation with devlink removal jobs, of_overlay_remove() can raise
> warnings related to missing of_node_put():
> ERROR: memory leak, expected refcount 1 instead of 2
>
> Indeed, the missing of_node_put() call is going to be done, too late,
> from the workqueue job execution.
>
> Introduce device_link_wait_removal() to offer a way to synchronize
> operations waiting for the end of devlink removals (i.e. end of
> workqueue jobs).
> Also, as a flushing operation is done on the workqueue, the workqueue
> used is moved from a system-wide workqueue to a local one.
>
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: [email protected]
> Signed-off-by: Herve Codina <[email protected]>
Tested-by: Luca Ceresoli <[email protected]>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > introduces a workqueue to release the consumer and supplier devices used
> > in the devlink.
> > In the job queued, devices are release and in turn, when all the
> > references to these devices are dropped, the release function of the
> > device itself is called.
> >
> > Nothing is present to provide some synchronisation with this workqueue
> > in order to ensure that all ongoing releasing operations are done and
> > so, some other operations can be started safely.
> >
> > For instance, in the following sequence:
> > 1) of_platform_depopulate()
> > 2) of_overlay_remove()
> >
> > During the step 1, devices are released and related devlinks are removed
> > (jobs pushed in the workqueue).
> > During the step 2, OF nodes are destroyed but, without any
> > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > warnings related to missing of_node_put():
> > ERROR: memory leak, expected refcount 1 instead of 2
> >
> > Indeed, the missing of_node_put() call is going to be done, too late,
> > from the workqueue job execution.
> >
> > Introduce device_link_wait_removal() to offer a way to synchronize
> > operations waiting for the end of devlink removals (i.e. end of
> > workqueue jobs).
> > Also, as a flushing operation is done on the workqueue, the workqueue
> > used is moved from a system-wide workqueue to a local one.
> >
> > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > Cc: [email protected]
> > Signed-off-by: Herve Codina <[email protected]>
> > ---
>
> With the below addressed:
>
> Reviewed-by: Nuno Sa <[email protected]>
>
> > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > include/linux/device.h | 1 +
> > 2 files changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d5f4e4aac09b..48b28c59c592 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -44,6 +44,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 *device_link_wq;
> >
> > /**
> > * __fwnode_link_add - Create a link between two fwnode_handles.
> > @@ -532,12 +533,26 @@ 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.
> > + * supplier devices get deleted when it runs, so put it into the
> > + * dedicated workqueue.
> > */
> > - queue_work(system_long_wq, &link->rm_work);
> > + queue_work(device_link_wq, &link->rm_work);
> > }
> >
> > +/**
> > + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> > terminate
> > + */
> > +void device_link_wait_removal(void)
> > +{
> > + /*
> > + * devlink removal jobs are queued in the dedicated work queue.
> > + * To be sure that all removal jobs are terminated, ensure that any
> > + * scheduled work has run to completion.
> > + */
> > + flush_workqueue(device_link_wq);
> > +}
> > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > +
> > static struct class devlink_class = {
> > .name = "devlink",
> > .dev_groups = devlink_groups,
> > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > if (!sysfs_dev_char_kobj)
> > goto char_kobj_err;
> > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > + if (!device_link_wq)
> > + goto wq_err;
> >
>
> I can't still agree with this. Why not doing it in devlink_class_init()? This is
> devlink specific so it makes complete sense to me.
If you do that in devlink_class_init() and it fails, you essentially
cause the creation of every device link to fail. IOW, you try to live
without device links and pretend that it is all OK. That won't get
you very far, especially on systems where DT is used.
Doing it here, if it fails, you prevent the driver model from working
at all (because one of its necessary components is unavailable), which
arguably is a better choice.
On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <[email protected]> wrote:
>
> The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> introduces a workqueue to release the consumer and supplier devices used
> in the devlink.
> In the job queued, devices are release and in turn, when all the
> references to these devices are dropped, the release function of the
> device itself is called.
>
> Nothing is present to provide some synchronisation with this workqueue
> in order to ensure that all ongoing releasing operations are done and
> so, some other operations can be started safely.
>
> For instance, in the following sequence:
> 1) of_platform_depopulate()
> 2) of_overlay_remove()
>
> During the step 1, devices are released and related devlinks are removed
> (jobs pushed in the workqueue).
> During the step 2, OF nodes are destroyed but, without any
> synchronisation with devlink removal jobs, of_overlay_remove() can raise
> warnings related to missing of_node_put():
> ERROR: memory leak, expected refcount 1 instead of 2
>
> Indeed, the missing of_node_put() call is going to be done, too late,
> from the workqueue job execution.
>
> Introduce device_link_wait_removal() to offer a way to synchronize
> operations waiting for the end of devlink removals (i.e. end of
> workqueue jobs).
> Also, as a flushing operation is done on the workqueue, the workqueue
> used is moved from a system-wide workqueue to a local one.
>
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
No, it is not fixed by this patch.
In fact, the only possibly observable effect of this patch is the
failure when the allocation of device_link_wq fails AFAICS.
> Cc: [email protected]
So why?
> Signed-off-by: Herve Codina <[email protected]>
> ---
> drivers/base/core.c | 26 +++++++++++++++++++++++---
> include/linux/device.h | 1 +
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d5f4e4aac09b..48b28c59c592 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -44,6 +44,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 *device_link_wq;
>
> /**
> * __fwnode_link_add - Create a link between two fwnode_handles.
> @@ -532,12 +533,26 @@ 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.
> + * supplier devices get deleted when it runs, so put it into the
> + * dedicated workqueue.
> */
> - queue_work(system_long_wq, &link->rm_work);
> + queue_work(device_link_wq, &link->rm_work);
> }
>
> +/**
> + * device_link_wait_removal - Wait for ongoing devlink removal jobs to terminate
> + */
> +void device_link_wait_removal(void)
> +{
> + /*
> + * devlink removal jobs are queued in the dedicated work queue.
> + * To be sure that all removal jobs are terminated, ensure that any
> + * scheduled work has run to completion.
> + */
> + flush_workqueue(device_link_wq);
> +}
> +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> +
> static struct class devlink_class = {
> .name = "devlink",
> .dev_groups = devlink_groups,
> @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> if (!sysfs_dev_char_kobj)
> goto char_kobj_err;
> + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> + if (!device_link_wq)
> + goto wq_err;
>
> return 0;
>
> + wq_err:
> + kobject_put(sysfs_dev_char_kobj);
> char_kobj_err:
> kobject_put(sysfs_dev_block_kobj);
> block_kobj_err:
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1795121dee9a..d7d8305a72e8 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1249,6 +1249,7 @@ void device_link_del(struct device_link *link);
> void device_link_remove(void *consumer, struct device *supplier);
> void device_links_supplier_sync_state_pause(void);
> void device_links_supplier_sync_state_resume(void);
> +void device_link_wait_removal(void);
>
> /* Create alias, so I can be autoloaded. */
> #define MODULE_ALIAS_CHARDEV(major,minor) \
> --
On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmailcom> wrote:
> > >
> > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > introduces a workqueue to release the consumer and supplier devices used
> > > > in the devlink.
> > > > In the job queued, devices are release and in turn, when all the
> > > > references to these devices are dropped, the release function of the
> > > > device itself is called.
> > > >
> > > > Nothing is present to provide some synchronisation with this workqueue
> > > > in order to ensure that all ongoing releasing operations are done and
> > > > so, some other operations can be started safely.
> > > >
> > > > For instance, in the following sequence:
> > > > 1) of_platform_depopulate()
> > > > 2) of_overlay_remove()
> > > >
> > > > During the step 1, devices are released and related devlinks are removed
> > > > (jobs pushed in the workqueue).
> > > > During the step 2, OF nodes are destroyed but, without any
> > > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > > warnings related to missing of_node_put():
> > > > ERROR: memory leak, expected refcount 1 instead of 2
> > > >
> > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > from the workqueue job execution.
> > > >
> > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > operations waiting for the end of devlink removals (i.e. end of
> > > > workqueue jobs).
> > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > used is moved from a system-wide workqueue to a local one.
> > > >
> > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > Cc: [email protected]
> > > > Signed-off-by: Herve Codina <[email protected]>
> > > > ---
> > >
> > > With the below addressed:
> > >
> > > Reviewed-by: Nuno Sa <[email protected]>
> > >
> > > > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > > > include/linux/device.h | 1 +
> > > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -44,6 +44,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 *device_link_wq;
> > > >
> > > > /**
> > > > * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > @@ -532,12 +533,26 @@ 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.
> > > > + * supplier devices get deleted when it runs, so put it into the
> > > > + * dedicated workqueue.
> > > > */
> > > > - queue_work(system_long_wq, &link->rm_work);
> > > > + queue_work(device_link_wq, &link->rm_work);
> > > > }
> > > >
> > > > +/**
> > > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> > > > terminate
> > > > + */
> > > > +void device_link_wait_removal(void)
> > > > +{
> > > > + /*
> > > > + * devlink removal jobs are queued in the dedicated work queue.
> > > > + * To be sure that all removal jobs are terminated, ensure that any
> > > > + * scheduled work has run to completion.
> > > > + */
> > > > + flush_workqueue(device_link_wq);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > +
> > > > static struct class devlink_class = {
> > > > .name = "devlink",
> > > > .dev_groups = devlink_groups,
> > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > > if (!sysfs_dev_char_kobj)
> > > > goto char_kobj_err;
> > > > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > + if (!device_link_wq)
> > > > + goto wq_err;
> > > >
> > >
> > > I can't still agree with this. Why not doing it in devlink_class_init()?
> > > This is
> > > devlink specific so it makes complete sense to me.
> >
> > If you do that in devlink_class_init() and it fails, you essentially
> > cause the creation of every device link to fail. IOW, you try to live
> > without device links and pretend that it is all OK. That won't get
> > you very far, especially on systems where DT is used.
> >
> > Doing it here, if it fails, you prevent the driver model from working
> > at all (because one of its necessary components is unavailable), which
> > arguably is a better choice.
>
> That makes sense but then the only thing I still don't fully get is why we have
> a separate devlink_class_init() initcall for registering the devlink class
> (which can also fail)...
Well, I haven't added it. :-)
> What I take from the above is that we should fail the
> driver model if one of it's fundamental components fails so I would say we
> should merge devlink_class_init() with device_init() otherwise it's a bit
> confusing (at least to me) and gives the idea that it's ok for the driver model
> to exist without the links (unless I'm missing some other reason for the devlink
> init function).
+1
Feel free to send a patch along these lines, chances are that it will
be popular. ;-)
On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <[email protected]> wrote:
> >
> > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > introduces a workqueue to release the consumer and supplier devices used
> > > in the devlink.
> > > In the job queued, devices are release and in turn, when all the
> > > references to these devices are dropped, the release function of the
> > > device itself is called.
> > >
> > > Nothing is present to provide some synchronisation with this workqueue
> > > in order to ensure that all ongoing releasing operations are done and
> > > so, some other operations can be started safely.
> > >
> > > For instance, in the following sequence:
> > > 1) of_platform_depopulate()
> > > 2) of_overlay_remove()
> > >
> > > During the step 1, devices are released and related devlinks are removed
> > > (jobs pushed in the workqueue).
> > > During the step 2, OF nodes are destroyed but, without any
> > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > warnings related to missing of_node_put():
> > > ERROR: memory leak, expected refcount 1 instead of 2
> > >
> > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > from the workqueue job execution.
> > >
> > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > operations waiting for the end of devlink removals (i.e. end of
> > > workqueue jobs).
> > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > used is moved from a system-wide workqueue to a local one.
> > >
> > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > Cc: [email protected]
> > > Signed-off-by: Herve Codina <[email protected]>
> > > ---
> >
> > With the below addressed:
> >
> > Reviewed-by: Nuno Sa <[email protected]>
> >
> > > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > > include/linux/device.h | 1 +
> > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index d5f4e4aac09b..48b28c59c592 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -44,6 +44,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 *device_link_wq;
> > >
> > > /**
> > > * __fwnode_link_add - Create a link between two fwnode_handles.
> > > @@ -532,12 +533,26 @@ 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.
> > > + * supplier devices get deleted when it runs, so put it into the
> > > + * dedicated workqueue.
> > > */
> > > - queue_work(system_long_wq, &link->rm_work);
> > > + queue_work(device_link_wq, &link->rm_work);
> > > }
> > >
> > > +/**
> > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> > > terminate
> > > + */
> > > +void device_link_wait_removal(void)
> > > +{
> > > + /*
> > > + * devlink removal jobs are queued in the dedicated work queue.
> > > + * To be sure that all removal jobs are terminated, ensure that any
> > > + * scheduled work has run to completion.
> > > + */
> > > + flush_workqueue(device_link_wq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > +
> > > static struct class devlink_class = {
> > > .name = "devlink",
> > > .dev_groups = devlink_groups,
> > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > if (!sysfs_dev_char_kobj)
> > > goto char_kobj_err;
> > > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > + if (!device_link_wq)
> > > + goto wq_err;
> > >
> >
> > I can't still agree with this. Why not doing it in devlink_class_init()?
> > This is
> > devlink specific so it makes complete sense to me.
>
> If you do that in devlink_class_init() and it fails, you essentially
> cause the creation of every device link to fail. IOW, you try to live
> without device links and pretend that it is all OK. That won't get
> you very far, especially on systems where DT is used.
>
> Doing it here, if it fails, you prevent the driver model from working
> at all (because one of its necessary components is unavailable), which
> arguably is a better choice.
That makes sense but then the only thing I still don't fully get is why we have
a separate devlink_class_init() initcall for registering the devlink class
(which can also fail)... What I take from the above is that we should fail the
driver model if one of it's fundamental components fails so I would say we
should merge devlink_class_init() with device_init() otherwise it's a bit
confusing (at least to me) and gives the idea that it's ok for the driver model
to exist without the links (unless I'm missing some other reason for the devlink
init function).
- Nuno Sá
On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <[email protected]> wrote:
> >
> > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <[email protected]> wrote:
> > > >
> > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > introduces a workqueue to release the consumer and supplier devices
> > > > > used
> > > > > in the devlink.
> > > > > In the job queued, devices are release and in turn, when all the
> > > > > references to these devices are dropped, the release function of the
> > > > > device itself is called.
> > > > >
> > > > > Nothing is present to provide some synchronisation with this workqueue
> > > > > in order to ensure that all ongoing releasing operations are done and
> > > > > so, some other operations can be started safely.
> > > > >
> > > > > For instance, in the following sequence:
> > > > > 1) of_platform_depopulate()
> > > > > 2) of_overlay_remove()
> > > > >
> > > > > During the step 1, devices are released and related devlinks are
> > > > > removed
> > > > > (jobs pushed in the workqueue).
> > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > raise
> > > > > warnings related to missing of_node_put():
> > > > > ERROR: memory leak, expected refcount 1 instead of 2
> > > > >
> > > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > > from the workqueue job execution.
> > > > >
> > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > workqueue jobs).
> > > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > > used is moved from a system-wide workqueue to a local one.
> > > > >
> > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > Cc: [email protected]
> > > > > Signed-off-by: Herve Codina <[email protected]>
> > > > > ---
> > > >
> > > > With the below addressed:
> > > >
> > > > Reviewed-by: Nuno Sa <[email protected]>
> > > >
> > > > > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > > > > include/linux/device.h | 1 +
> > > > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > --- a/drivers/base/core.c
> > > > > +++ b/drivers/base/core.c
> > > > > @@ -44,6 +44,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 *device_link_wq;
> > > > >
> > > > > /**
> > > > > * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > @@ -532,12 +533,26 @@ 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.
> > > > > + * supplier devices get deleted when it runs, so put it into the
> > > > > + * dedicated workqueue.
> > > > > */
> > > > > - queue_work(system_long_wq, &link->rm_work);
> > > > > + queue_work(device_link_wq, &link->rm_work);
> > > > > }
> > > > >
> > > > > +/**
> > > > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs
> > > > > to
> > > > > terminate
> > > > > + */
> > > > > +void device_link_wait_removal(void)
> > > > > +{
> > > > > + /*
> > > > > + * devlink removal jobs are queued in the dedicated work queue.
> > > > > + * To be sure that all removal jobs are terminated, ensure that
> > > > > any
> > > > > + * scheduled work has run to completion.
> > > > > + */
> > > > > + flush_workqueue(device_link_wq);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > +
> > > > > static struct class devlink_class = {
> > > > > .name = "devlink",
> > > > > .dev_groups = devlink_groups,
> > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > > > if (!sysfs_dev_char_kobj)
> > > > > goto char_kobj_err;
> > > > > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > + if (!device_link_wq)
> > > > > + goto wq_err;
> > > > >
> > > >
> > > > I can't still agree with this. Why not doing it in devlink_class_init()?
> > > > This is
> > > > devlink specific so it makes complete sense to me.
> > >
> > > If you do that in devlink_class_init() and it fails, you essentially
> > > cause the creation of every device link to fail. IOW, you try to live
> > > without device links and pretend that it is all OK. That won't get
> > > you very far, especially on systems where DT is used.
> > >
> > > Doing it here, if it fails, you prevent the driver model from working
> > > at all (because one of its necessary components is unavailable), which
> > > arguably is a better choice.
> >
> > That makes sense but then the only thing I still don't fully get is why we
> > have
> > a separate devlink_class_init() initcall for registering the devlink class
> > (which can also fail)...
>
> Well, I haven't added it. :-)
>
> > What I take from the above is that we should fail the
> > driver model if one of it's fundamental components fails so I would say we
> > should merge devlink_class_init() with device_init() otherwise it's a bit
> > confusing (at least to me) and gives the idea that it's ok for the driver
> > model
> > to exist without the links (unless I'm missing some other reason for the
> > devlink
> > init function).
>
> +1
>
> Feel free to send a patch along these lines, chances are that it will
> be popular. ;-)
I was actually thinking about that but I think I encountered the reason why we
have it like this... devices_init() is called from driver_init() and there we
have:
..
devices_init();
buses_init();
classes_init();
..
So classes are initialized after devices which means we can't really do
class_register(&devlink_class) from devices_init(). Unless, of course, we re-
order things in driver_init() but that would be a questionable change at the
very least.
So, while I agree with what you've said, I'm still not sure if mixing devlink
stuff between devices_init() and devlink_class_init() is the best thing to do
given that we already have the case where devlink_class_init() can fail while
the driver model is up.
- Nuno Sá
On Wed, Mar 6, 2024 at 3:08 PM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> > On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <[email protected]> wrote:
> > >
> > > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <[email protected]> wrote:
> > > > >
> > > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > introduces a workqueue to release the consumer and supplier devices
> > > > > > used
> > > > > > in the devlink.
> > > > > > In the job queued, devices are release and in turn, when all the
> > > > > > references to these devices are dropped, the release function of the
> > > > > > device itself is called.
> > > > > >
> > > > > > Nothing is present to provide some synchronisation with this workqueue
> > > > > > in order to ensure that all ongoing releasing operations are done and
> > > > > > so, some other operations can be started safely.
> > > > > >
> > > > > > For instance, in the following sequence:
> > > > > > 1) of_platform_depopulate()
> > > > > > 2) of_overlay_remove()
> > > > > >
> > > > > > During the step 1, devices are released and related devlinks are
> > > > > > removed
> > > > > > (jobs pushed in the workqueue).
> > > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > > raise
> > > > > > warnings related to missing of_node_put():
> > > > > > ERROR: memory leak, expected refcount 1 instead of 2
> > > > > >
> > > > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > > > from the workqueue job execution.
> > > > > >
> > > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > > workqueue jobs).
> > > > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > > > used is moved from a system-wide workqueue to a local one.
> > > > > >
> > > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > Cc: [email protected]
> > > > > > Signed-off-by: Herve Codina <[email protected]>
> > > > > > ---
> > > > >
> > > > > With the below addressed:
> > > > >
> > > > > Reviewed-by: Nuno Sa <[email protected]>
> > > > >
> > > > > > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > > > > > include/linux/device.h | 1 +
> > > > > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > > --- a/drivers/base/core.c
> > > > > > +++ b/drivers/base/core.c
> > > > > > @@ -44,6 +44,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 *device_link_wq;
> > > > > >
> > > > > > /**
> > > > > > * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > > @@ -532,12 +533,26 @@ 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.
> > > > > > + * supplier devices get deleted when it runs, so put it into the
> > > > > > + * dedicated workqueue.
> > > > > > */
> > > > > > - queue_work(system_long_wq, &link->rm_work);
> > > > > > + queue_work(device_link_wq, &link->rm_work);
> > > > > > }
> > > > > >
> > > > > > +/**
> > > > > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs
> > > > > > to
> > > > > > terminate
> > > > > > + */
> > > > > > +void device_link_wait_removal(void)
> > > > > > +{
> > > > > > + /*
> > > > > > + * devlink removal jobs are queued in the dedicated work queue.
> > > > > > + * To be sure that all removal jobs are terminated, ensure that
> > > > > > any
> > > > > > + * scheduled work has run to completion.
> > > > > > + */
> > > > > > + flush_workqueue(device_link_wq);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > > +
> > > > > > static struct class devlink_class = {
> > > > > > .name = "devlink",
> > > > > > .dev_groups = devlink_groups,
> > > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > > sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > > > > if (!sysfs_dev_char_kobj)
> > > > > > goto char_kobj_err;
> > > > > > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > > + if (!device_link_wq)
> > > > > > + goto wq_err;
> > > > > >
> > > > >
> > > > > I can't still agree with this. Why not doing it in devlink_class_init()?
> > > > > This is
> > > > > devlink specific so it makes complete sense to me.
> > > >
> > > > If you do that in devlink_class_init() and it fails, you essentially
> > > > cause the creation of every device link to fail. IOW, you try to live
> > > > without device links and pretend that it is all OK. That won't get
> > > > you very far, especially on systems where DT is used.
> > > >
> > > > Doing it here, if it fails, you prevent the driver model from working
> > > > at all (because one of its necessary components is unavailable), which
> > > > arguably is a better choice.
> > >
> > > That makes sense but then the only thing I still don't fully get is why we
> > > have
> > > a separate devlink_class_init() initcall for registering the devlink class
> > > (which can also fail)...
> >
> > Well, I haven't added it. :-)
> >
> > > What I take from the above is that we should fail the
> > > driver model if one of it's fundamental components fails so I would say we
> > > should merge devlink_class_init() with device_init() otherwise it's a bit
> > > confusing (at least to me) and gives the idea that it's ok for the driver
> > > model
> > > to exist without the links (unless I'm missing some other reason for the
> > > devlink
> > > init function).
> >
> > +1
> >
> > Feel free to send a patch along these lines, chances are that it will
> > be popular. ;-)
>
> I was actually thinking about that but I think I encountered the reason why we
> have it like this... devices_init() is called from driver_init() and there we
> have:
>
> ...
>
> devices_init();
> buses_init();
> classes_init();
>
> ...
>
> So classes are initialized after devices which means we can't really do
> class_register(&devlink_class) from devices_init(). Unless, of course, we re-
> order things in driver_init() but that would be a questionable change at the
> very least.
>
> So, while I agree with what you've said, I'm still not sure if mixing devlink
> stuff between devices_init() and devlink_class_init() is the best thing to do
> given that we already have the case where devlink_class_init() can fail while
> the driver model is up.
So why don't you make devlink_class_init() do a BUG() on failure
instead of returning an error? IMO crashing early is better than
crashing later or otherwise failing in a subtle way due to a missed
dependency.
On Wed, 2024-03-06 at 15:37 +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 6, 2024 at 3:08 PM Nuno Sá <[email protected]> wrote:
> >
> > On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <[email protected]> wrote:
> > > >
> > > > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > introduces a workqueue to release the consumer and supplier
> > > > > > > devices
> > > > > > > used
> > > > > > > in the devlink.
> > > > > > > In the job queued, devices are release and in turn, when all the
> > > > > > > references to these devices are dropped, the release function of
> > > > > > > the
> > > > > > > device itself is called.
> > > > > > >
> > > > > > > Nothing is present to provide some synchronisation with this
> > > > > > > workqueue
> > > > > > > in order to ensure that all ongoing releasing operations are done
> > > > > > > and
> > > > > > > so, some other operations can be started safely.
> > > > > > >
> > > > > > > For instance, in the following sequence:
> > > > > > > 1) of_platform_depopulate()
> > > > > > > 2) of_overlay_remove()
> > > > > > >
> > > > > > > During the step 1, devices are released and related devlinks are
> > > > > > > removed
> > > > > > > (jobs pushed in the workqueue).
> > > > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > > > raise
> > > > > > > warnings related to missing of_node_put():
> > > > > > > ERROR: memory leak, expected refcount 1 instead of 2
> > > > > > >
> > > > > > > Indeed, the missing of_node_put() call is going to be done, too
> > > > > > > late,
> > > > > > > from the workqueue job execution.
> > > > > > >
> > > > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > > > workqueue jobs).
> > > > > > > Also, as a flushing operation is done on the workqueue, the
> > > > > > > workqueue
> > > > > > > used is moved from a system-wide workqueue to a local one.
> > > > > > >
> > > > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > Cc: [email protected]
> > > > > > > Signed-off-by: Herve Codina <[email protected]>
> > > > > > > ---
> > > > > >
> > > > > > With the below addressed:
> > > > > >
> > > > > > Reviewed-by: Nuno Sa <[email protected]>
> > > > > >
> > > > > > > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > > > > > > include/linux/device.h | 1 +
> > > > > > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > > > --- a/drivers/base/core.c
> > > > > > > +++ b/drivers/base/core.c
> > > > > > > @@ -44,6 +44,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 *device_link_wq;
> > > > > > >
> > > > > > > /**
> > > > > > > * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > > > @@ -532,12 +533,26 @@ 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.
> > > > > > > + * supplier devices get deleted when it runs, so put it into
> > > > > > > the
> > > > > > > + * dedicated workqueue.
> > > > > > > */
> > > > > > > - queue_work(system_long_wq, &link->rm_work);
> > > > > > > + queue_work(device_link_wq, &link->rm_work);
> > > > > > > }
> > > > > > >
> > > > > > > +/**
> > > > > > > + * device_link_wait_removal - Wait for ongoing devlink removal
> > > > > > > jobs
> > > > > > > to
> > > > > > > terminate
> > > > > > > + */
> > > > > > > +void device_link_wait_removal(void)
> > > > > > > +{
> > > > > > > + /*
> > > > > > > + * devlink removal jobs are queued in the dedicated work
> > > > > > > queue.
> > > > > > > + * To be sure that all removal jobs are terminated, ensure
> > > > > > > that
> > > > > > > any
> > > > > > > + * scheduled work has run to completion.
> > > > > > > + */
> > > > > > > + flush_workqueue(device_link_wq);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > > > +
> > > > > > > static struct class devlink_class = {
> > > > > > > .name = "devlink",
> > > > > > > .dev_groups = devlink_groups,
> > > > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > > > sysfs_dev_char_kobj = kobject_create_and_add("char",
> > > > > > > dev_kobj);
> > > > > > > if (!sysfs_dev_char_kobj)
> > > > > > > goto char_kobj_err;
> > > > > > > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > > > + if (!device_link_wq)
> > > > > > > + goto wq_err;
> > > > > > >
> > > > > >
> > > > > > I can't still agree with this. Why not doing it in
> > > > > > devlink_class_init()?
> > > > > > This is
> > > > > > devlink specific so it makes complete sense to me.
> > > > >
> > > > > If you do that in devlink_class_init() and it fails, you essentially
> > > > > cause the creation of every device link to fail. IOW, you try to live
> > > > > without device links and pretend that it is all OK. That won't get
> > > > > you very far, especially on systems where DT is used.
> > > > >
> > > > > Doing it here, if it fails, you prevent the driver model from working
> > > > > at all (because one of its necessary components is unavailable), which
> > > > > arguably is a better choice.
> > > >
> > > > That makes sense but then the only thing I still don't fully get is why
> > > > we
> > > > have
> > > > a separate devlink_class_init() initcall for registering the devlink
> > > > class
> > > > (which can also fail)...
> > >
> > > Well, I haven't added it. :-)
> > >
> > > > What I take from the above is that we should fail the
> > > > driver model if one of it's fundamental components fails so I would say
> > > > we
> > > > should merge devlink_class_init() with device_init() otherwise it's a
> > > > bit
> > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > driver
> > > > model
> > > > to exist without the links (unless I'm missing some other reason for the
> > > > devlink
> > > > init function).
> > >
> > > +1
> > >
> > > Feel free to send a patch along these lines, chances are that it will
> > > be popular. ;-)
> >
> > I was actually thinking about that but I think I encountered the reason why
> > we
> > have it like this... devices_init() is called from driver_init() and there
> > we
> > have:
> >
> > ...
> >
> > devices_init();
> > buses_init();
> > classes_init();
> >
> > ...
> >
> > So classes are initialized after devices which means we can't really do
> > class_register(&devlink_class) from devices_init(). Unless, of course, we
> > re-
> > order things in driver_init() but that would be a questionable change at the
> > very least.
> >
> > So, while I agree with what you've said, I'm still not sure if mixing
> > devlink
> > stuff between devices_init() and devlink_class_init() is the best thing to
> > do
> > given that we already have the case where devlink_class_init() can fail
> > while
> > the driver model is up.
>
> So why don't you make devlink_class_init() do a BUG() on failure
> instead of returning an error? IMO crashing early is better than
> crashing later or otherwise failing in a subtle way due to a missed
> dependency.
Well, I do agree with that... Maybe that's something that Herve can sneak in
this patch? Otherwise, I can later (after this one is applied) send a patch for
it.
- Nuno Sá
On Wed, 2024-03-06 at 16:01 +0100, Herve Codina wrote:
> Hi Nuno,
>
> On Wed, 06 Mar 2024 15:50:44 +0100
> Nuno Sá <[email protected]> wrote:
>
> ...
> > > > > >
> > > > > > That makes sense but then the only thing I still don't fully get is
> > > > > > why
> > > > > > we
> > > > > > have
> > > > > > a separate devlink_class_init() initcall for registering the devlink
> > > > > > class
> > > > > > (which can also fail)...
> > > > >
> > > > > Well, I haven't added it. :-)
> > > > >
> > > > > > What I take from the above is that we should fail the
> > > > > > driver model if one of it's fundamental components fails so I would
> > > > > > say
> > > > > > we
> > > > > > should merge devlink_class_init() with device_init() otherwise it's
> > > > > > a
> > > > > > bit
> > > > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > > > driver
> > > > > > model
> > > > > > to exist without the links (unless I'm missing some other reason for
> > > > > > the
> > > > > > devlink
> > > > > > init function).
> > > > >
> > > > > +1
> > > > >
> > > > > Feel free to send a patch along these lines, chances are that it will
> > > > > be popular. ;-)
> > > >
> > > > I was actually thinking about that but I think I encountered the reason
> > > > why
> > > > we
> > > > have it like this... devices_init() is called from driver_init() and
> > > > there
> > > > we
> > > > have:
> > > >
> > > > ...
> > > >
> > > > devices_init();
> > > > buses_init();
> > > > classes_init();
> > > >
> > > > ...
> > > >
> > > > So classes are initialized after devices which means we can't really do
> > > > class_register(&devlink_class) from devices_init(). Unless, of course,
> > > > we
> > > > re-
> > > > order things in driver_init() but that would be a questionable change at
> > > > the
> > > > very least.
> > > >
> > > > So, while I agree with what you've said, I'm still not sure if mixing
> > > > devlink
> > > > stuff between devices_init() and devlink_class_init() is the best thing
> > > > to
> > > > do
> > > > given that we already have the case where devlink_class_init() can fail
> > > > while
> > > > the driver model is up.
> > >
> > > So why don't you make devlink_class_init() do a BUG() on failure
> > > instead of returning an error? IMO crashing early is better than
> > > crashing later or otherwise failing in a subtle way due to a missed
> > > dependency.
> >
> > Well, I do agree with that... Maybe that's something that Herve can sneak in
> > this patch? Otherwise, I can later (after this one is applied) send a patch
> > for
> > it.
>
> Well, I don't thing that this have to be part of this current series.
> It is an other topic and should be handled out of this current series.
>
Yeah, fair enough... IMHO, then I would say that we should still have the
workqueue moved to devlink_class_init() and I can then follow up with a patch to
do BUG_ON(ret) in it.
Alternatively I can also just move it in the follow up patch but I don't think
it makes much sense.
- Nuno Sá
Hi Rafael,
On Wed, 6 Mar 2024 13:48:37 +0100
"Rafael J. Wysocki" <[email protected]> wrote:
> On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <[email protected]> wrote:
> >
> > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > introduces a workqueue to release the consumer and supplier devices used
> > in the devlink.
> > In the job queued, devices are release and in turn, when all the
> > references to these devices are dropped, the release function of the
> > device itself is called.
> >
> > Nothing is present to provide some synchronisation with this workqueue
> > in order to ensure that all ongoing releasing operations are done and
> > so, some other operations can be started safely.
> >
> > For instance, in the following sequence:
> > 1) of_platform_depopulate()
> > 2) of_overlay_remove()
> >
> > During the step 1, devices are released and related devlinks are removed
> > (jobs pushed in the workqueue).
> > During the step 2, OF nodes are destroyed but, without any
> > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > warnings related to missing of_node_put():
> > ERROR: memory leak, expected refcount 1 instead of 2
> >
> > Indeed, the missing of_node_put() call is going to be done, too late,
> > from the workqueue job execution.
> >
> > Introduce device_link_wait_removal() to offer a way to synchronize
> > operations waiting for the end of devlink removals (i.e. end of
> > workqueue jobs).
> > Also, as a flushing operation is done on the workqueue, the workqueue
> > used is moved from a system-wide workqueue to a local one.
> >
> > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
>
> No, it is not fixed by this patch.
Was explicitly asked by Saravana on v1 review:
https://lore.kernel.org/linux-kernel/CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com/
The commit 80dd33cf72d1 introduces the workqueue and so some asynchronous tasks
on removal.
This patch and the next one allows to re-sync execution waiting for jobs in
the workqueue when it is needed.
>
> In fact, the only possibly observable effect of this patch is the
> failure when the allocation of device_link_wq fails AFAICS.
>
> > Cc: [email protected]
>
> So why?
Cc:stable is needed as this patch is a prerequisite of patch 2 (needed
to fix the asynchronous workqueue task issue).
Best regards,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 6, 2024 at 4:24 PM Herve Codina <[email protected]> wrote:
>
> Hi Rafael,
>
> On Wed, 6 Mar 2024 13:48:37 +0100
> "Rafael J. Wysocki" <[email protected]> wrote:
>
> > On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <[email protected]> wrote:
> > >
> > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > introduces a workqueue to release the consumer and supplier devices used
> > > in the devlink.
> > > In the job queued, devices are release and in turn, when all the
> > > references to these devices are dropped, the release function of the
> > > device itself is called.
> > >
> > > Nothing is present to provide some synchronisation with this workqueue
> > > in order to ensure that all ongoing releasing operations are done and
> > > so, some other operations can be started safely.
> > >
> > > For instance, in the following sequence:
> > > 1) of_platform_depopulate()
> > > 2) of_overlay_remove()
> > >
> > > During the step 1, devices are released and related devlinks are removed
> > > (jobs pushed in the workqueue).
> > > During the step 2, OF nodes are destroyed but, without any
> > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > warnings related to missing of_node_put():
> > > ERROR: memory leak, expected refcount 1 instead of 2
> > >
> > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > from the workqueue job execution.
> > >
> > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > operations waiting for the end of devlink removals (i.e. end of
> > > workqueue jobs).
> > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > used is moved from a system-wide workqueue to a local one.
> > >
> > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> >
> > No, it is not fixed by this patch.
>
> Was explicitly asked by Saravana on v1 review:
> https://lore.kernel.org/linux-kernel/CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com/
Well, I don't think this is a valid request, sorry.
> The commit 80dd33cf72d1 introduces the workqueue and so some asynchronous tasks
> on removal.
> This patch and the next one allows to re-sync execution waiting for jobs in
> the workqueue when it is needed.
I get this, but still, this particular individual patch by itself
doesn't fix anything. Do you agree with this?
If somebody applies this patch without the next one in the series,
they will not get any change in behavior, so the tag is at least
misleading.
You can claim that the next patch on top of this one fixes things, so
adding a Fixes tag to the other patch would be fine.
There is an explicit dependency between them (the second patch is not
even applicable without the first one, or if it is, the resulting code
won't compile anyway), but you can make a note to the maintainer that
they need to go to -stable together.
> >
> > In fact, the only possibly observable effect of this patch is the
> > failure when the allocation of device_link_wq fails AFAICS.
> >
> > > Cc: [email protected]
> >
> > So why?
>
> Cc:stable is needed as this patch is a prerequisite of patch 2 (needed
> to fix the asynchronous workqueue task issue).
Dependencies like this can be expressed in "Cc: stable" tags.
Personally, I'd do it like this:
Cc: [email protected] # 5.13: Depends on the first patch in the series
On Wed, Mar 6, 2024 at 6:47 AM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-03-06 at 15:37 +0100, Rafael J. Wysocki wrote:
> > On Wed, Mar 6, 2024 at 3:08 PM Nuno Sá <[email protected]> wrote:
> > >
> > > On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <[email protected]> wrote:
> > > > >
> > > > > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > > introduces a workqueue to release the consumer and supplier
> > > > > > > > devices
> > > > > > > > used
> > > > > > > > in the devlink.
> > > > > > > > In the job queued, devices are release and in turn, when all the
> > > > > > > > references to these devices are dropped, the release function of
> > > > > > > > the
> > > > > > > > device itself is called.
> > > > > > > >
> > > > > > > > Nothing is present to provide some synchronisation with this
> > > > > > > > workqueue
> > > > > > > > in order to ensure that all ongoing releasing operations are done
> > > > > > > > and
> > > > > > > > so, some other operations can be started safely.
> > > > > > > >
> > > > > > > > For instance, in the following sequence:
> > > > > > > > 1) of_platform_depopulate()
> > > > > > > > 2) of_overlay_remove()
> > > > > > > >
> > > > > > > > During the step 1, devices are released and related devlinks are
> > > > > > > > removed
> > > > > > > > (jobs pushed in the workqueue).
> > > > > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > > > > raise
> > > > > > > > warnings related to missing of_node_put():
> > > > > > > > ERROR: memory leak, expected refcount 1 instead of 2
> > > > > > > >
> > > > > > > > Indeed, the missing of_node_put() call is going to be done, too
> > > > > > > > late,
> > > > > > > > from the workqueue job execution.
> > > > > > > >
> > > > > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > > > > workqueue jobs).
> > > > > > > > Also, as a flushing operation is done on the workqueue, the
> > > > > > > > workqueue
> > > > > > > > used is moved from a system-wide workqueue to a local one.
> > > > > > > >
> > > > > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > > Cc: [email protected]
> > > > > > > > Signed-off-by: Herve Codina <[email protected]>
> > > > > > > > ---
> > > > > > >
> > > > > > > With the below addressed:
> > > > > > >
> > > > > > > Reviewed-by: Nuno Sa <[email protected]>
> > > > > > >
> > > > > > > > drivers/base/core.c | 26 +++++++++++++++++++++++---
> > > > > > > > include/linux/device.h | 1 +
> > > > > > > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > > > > --- a/drivers/base/core.c
> > > > > > > > +++ b/drivers/base/core.c
> > > > > > > > @@ -44,6 +44,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 *device_link_wq;
> > > > > > > >
> > > > > > > > /**
> > > > > > > > * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > > > > @@ -532,12 +533,26 @@ 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.
> > > > > > > > + * supplier devices get deleted when it runs, so put it into
> > > > > > > > the
> > > > > > > > + * dedicated workqueue.
> > > > > > > > */
> > > > > > > > - queue_work(system_long_wq, &link->rm_work);
> > > > > > > > + queue_work(device_link_wq, &link->rm_work);
> > > > > > > > }
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * device_link_wait_removal - Wait for ongoing devlink removal
> > > > > > > > jobs
> > > > > > > > to
> > > > > > > > terminate
> > > > > > > > + */
> > > > > > > > +void device_link_wait_removal(void)
> > > > > > > > +{
> > > > > > > > + /*
> > > > > > > > + * devlink removal jobs are queued in the dedicated work
> > > > > > > > queue.
> > > > > > > > + * To be sure that all removal jobs are terminated, ensure
> > > > > > > > that
> > > > > > > > any
> > > > > > > > + * scheduled work has run to completion.
> > > > > > > > + */
> > > > > > > > + flush_workqueue(device_link_wq);
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > > > > +
> > > > > > > > static struct class devlink_class = {
> > > > > > > > .name = "devlink",
> > > > > > > > .dev_groups = devlink_groups,
> > > > > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > > > > sysfs_dev_char_kobj = kobject_create_and_add("char",
> > > > > > > > dev_kobj);
> > > > > > > > if (!sysfs_dev_char_kobj)
> > > > > > > > goto char_kobj_err;
> > > > > > > > + device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > > > > + if (!device_link_wq)
> > > > > > > > + goto wq_err;
> > > > > > > >
> > > > > > >
> > > > > > > I can't still agree with this. Why not doing it in
> > > > > > > devlink_class_init()?
> > > > > > > This is
> > > > > > > devlink specific so it makes complete sense to me.
> > > > > >
> > > > > > If you do that in devlink_class_init() and it fails, you essentially
> > > > > > cause the creation of every device link to fail. IOW, you try to live
> > > > > > without device links and pretend that it is all OK. That won't get
> > > > > > you very far, especially on systems where DT is used.
> > > > > >
> > > > > > Doing it here, if it fails, you prevent the driver model from working
> > > > > > at all (because one of its necessary components is unavailable), which
> > > > > > arguably is a better choice.
> > > > >
> > > > > That makes sense but then the only thing I still don't fully get is why
> > > > > we
> > > > > have
> > > > > a separate devlink_class_init() initcall for registering the devlink
> > > > > class
> > > > > (which can also fail)...
> > > >
> > > > Well, I haven't added it. :-)
> > > >
> > > > > What I take from the above is that we should fail the
> > > > > driver model if one of it's fundamental components fails so I would say
> > > > > we
> > > > > should merge devlink_class_init() with device_init() otherwise it's a
> > > > > bit
> > > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > > driver
> > > > > model
> > > > > to exist without the links (unless I'm missing some other reason for the
> > > > > devlink
> > > > > init function).
> > > >
> > > > +1
> > > >
> > > > Feel free to send a patch along these lines, chances are that it will
> > > > be popular. ;-)
> > >
> > > I was actually thinking about that but I think I encountered the reason why
> > > we
> > > have it like this... devices_init() is called from driver_init() and there
> > > we
> > > have:
> > >
> > > ...
> > >
> > > devices_init();
> > > buses_init();
> > > classes_init();
> > >
> > > ...
> > >
> > > So classes are initialized after devices which means we can't really do
> > > class_register(&devlink_class) from devices_init(). Unless, of course, we
> > > re-
> > > order things in driver_init() but that would be a questionable change at the
> > > very least.
> > >
> > > So, while I agree with what you've said, I'm still not sure if mixing
> > > devlink
> > > stuff between devices_init() and devlink_class_init() is the best thing to
> > > do
> > > given that we already have the case where devlink_class_init() can fail
> > > while
> > > the driver model is up.
> >
> > So why don't you make devlink_class_init() do a BUG() on failure
> > instead of returning an error? IMO crashing early is better than
> > crashing later or otherwise failing in a subtle way due to a missed
> > dependency.
>
> Well, I do agree with that... Maybe that's something that Herve can sneak in
> this patch? Otherwise, I can later (after this one is applied) send a patch for
> it.
I'll happily Ack the patch if you want to add a BUG(), but the way
it's written is still pedantically better than putting it in
devices_init(). All errors from devices_init() are ignored and not
even logged by the caller. At least any error from
devlink_class_init() would be logged if initcall_debug is enabled :-)
Oh, btw, I wrote devlink_class_init() as a separate initcall because
it's just another class like any other class that's being registered.
All that said, I think this whole discussion is a pedantic waste of time.
-Saravana
On Wed, Mar 6, 2024 at 7:56 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Wed, Mar 6, 2024 at 4:24 PM Herve Codina <herve.codina@bootlincom> wrote:
> >
> > Hi Rafael,
> >
> > On Wed, 6 Mar 2024 13:48:37 +0100
> > "Rafael J. Wysocki" <[email protected]> wrote:
> >
> > > On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <[email protected]> wrote:
> > > >
> > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > introduces a workqueue to release the consumer and supplier devices used
> > > > in the devlink.
> > > > In the job queued, devices are release and in turn, when all the
> > > > references to these devices are dropped, the release function of the
> > > > device itself is called.
> > > >
> > > > Nothing is present to provide some synchronisation with this workqueue
> > > > in order to ensure that all ongoing releasing operations are done and
> > > > so, some other operations can be started safely.
> > > >
> > > > For instance, in the following sequence:
> > > > 1) of_platform_depopulate()
> > > > 2) of_overlay_remove()
> > > >
> > > > During the step 1, devices are released and related devlinks are removed
> > > > (jobs pushed in the workqueue).
> > > > During the step 2, OF nodes are destroyed but, without any
> > > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > > warnings related to missing of_node_put():
> > > > ERROR: memory leak, expected refcount 1 instead of 2
> > > >
> > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > from the workqueue job execution.
> > > >
> > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > operations waiting for the end of devlink removals (i.e. end of
> > > > workqueue jobs).
> > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > used is moved from a system-wide workqueue to a local one.
> > > >
> > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > >
> > > No, it is not fixed by this patch.
> >
> > Was explicitly asked by Saravana on v1 review:
> > https://lore.kernel.org/linux-kernel/CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com/
>
> Well, I don't think this is a valid request, sorry.
>
> > The commit 80dd33cf72d1 introduces the workqueue and so some asynchronous tasks
> > on removal.
> > This patch and the next one allows to re-sync execution waiting for jobs in
> > the workqueue when it is needed.
>
> I get this, but still, this particular individual patch by itself
> doesn't fix anything. Do you agree with this?
>
> If somebody applies this patch without the next one in the series,
> they will not get any change in behavior, so the tag is at least
> misleading.
>
> You can claim that the next patch on top of this one fixes things, so
> adding a Fixes tag to the other patch would be fine.
>
> There is an explicit dependency between them (the second patch is not
> even applicable without the first one, or if it is, the resulting code
> won't compile anyway), but you can make a note to the maintainer that
> they need to go to -stable together.
>
> > >
> > > In fact, the only possibly observable effect of this patch is the
> > > failure when the allocation of device_link_wq fails AFAICS.
> > >
> > > > Cc: [email protected]
> > >
> > > So why?
> >
> > Cc:stable is needed as this patch is a prerequisite of patch 2 (needed
> > to fix the asynchronous workqueue task issue).
>
> Dependencies like this can be expressed in "Cc: stable" tags.
> Personally, I'd do it like this:
>
> Cc: [email protected] # 5.13: Depends on the first patch in the series
I'm okay with this too. I personally think it's better to list "Fixes:
xyz" in all the patches that are needed to fix xyz (especially when
there's no compile time dependency on earlier patches), but it's not a
hill I'll die on. And if Rafael's suggestion is the expected norm,
then I'll remember to follow that in the future.
-Saravana