* [PATCH v1 1/3] driver core: Don't do deferred probe in parallel with kernel_init thread
2020-07-01 19:42 [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Saravana Kannan
@ 2020-07-01 19:42 ` Saravana Kannan
2020-07-07 15:59 ` Rafael J. Wysocki
2020-07-01 19:42 ` [PATCH v1 2/3] driver core: Rename dev_links_info.defer_sync to defer_hook Saravana Kannan
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Saravana Kannan @ 2020-07-01 19:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: linux-kernel, Geert Uytterhoeven, kernel-team
The current deferred probe implementation can mess up suspend/resume
ordering if deferred probe thread is kicked off in parallel with the
main initcall thread (kernel_init thread) [1].
For example:
Say device-B is a consumer of device-A.
Initcall thread Deferred probe thread
=============== =====================
1. device-A is added.
2. device-B is added.
3. dpm_list is now [device-A, device-B].
4. driver-A defers probe of device-A.
5. device-A is moved to
end of dpm_list
6. dpm_list is now
[device-B, device-A]
7. driver-B is registereed and probes device-B.
8. dpm_list stays as [device-B, device-A].
The reverse order of dpm_list is used for suspend. So in this case
device-A would incorrectly get suspended before device-B.
Commit 716a7a259690 ("driver core: fw_devlink: Add support for batching
fwnode parsing") kicked off the deferred probe thread early during boot
to run in parallel with the initcall thread and caused suspend/resume
regressions. This patch removes the parallel run of the deferred probe
thread to avoid the suspend/resume regressions.
[1] - https://lore.kernel.org/lkml/CAGETcx8W96KAw-d_siTX4qHB_-7ddk0miYRDQeHE6E0_8qx-6Q@mail.gmail.com/
Fixes: 716a7a259690 ("driver core: fw_devlink: Add support for batching fwnode parsing")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/base.h | 1 -
drivers/base/core.c | 1 -
drivers/base/dd.c | 5 -----
3 files changed, 7 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..40fb069a8a7e 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -153,7 +153,6 @@ extern char *make_class_name(const char *name, struct kobject *kobj);
extern int devres_release_all(struct device *dev);
extern void device_block_probing(void);
extern void device_unblock_probing(void);
-extern void driver_deferred_probe_force_trigger(void);
/* /sys/devices directory */
extern struct kset *devices_kset;
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..35cc9896eb9e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1323,7 +1323,6 @@ void fw_devlink_resume(void)
goto out;
device_link_add_missing_supplier_links();
- driver_deferred_probe_force_trigger();
out:
mutex_unlock(&defer_fw_devlink_lock);
}
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..48ca81cb8ebc 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -164,11 +164,6 @@ static void driver_deferred_probe_trigger(void)
if (!driver_deferred_probe_enable)
return;
- driver_deferred_probe_force_trigger();
-}
-
-void driver_deferred_probe_force_trigger(void)
-{
/*
* A successful probe means that all the devices in the pending list
* should be triggered to be reprobed. Move all the deferred devices
--
2.27.0.212.ge8ba1cc988-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/3] driver core: Don't do deferred probe in parallel with kernel_init thread
2020-07-01 19:42 ` [PATCH v1 1/3] driver core: Don't do deferred probe in parallel with kernel_init thread Saravana Kannan
@ 2020-07-07 15:59 ` Rafael J. Wysocki
2020-07-07 21:54 ` Saravana Kannan
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2020-07-07 15:59 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List,
Geert Uytterhoeven, Cc: Android Kernel
On Wed, Jul 1, 2020 at 9:43 PM Saravana Kannan <saravanak@google.com> wrote:
>
> The current deferred probe implementation can mess up suspend/resume
> ordering if deferred probe thread is kicked off in parallel with the
> main initcall thread (kernel_init thread) [1].
>
> For example:
>
> Say device-B is a consumer of device-A.
>
> Initcall thread Deferred probe thread
> =============== =====================
> 1. device-A is added.
> 2. device-B is added.
> 3. dpm_list is now [device-A, device-B].
> 4. driver-A defers probe of device-A.
> 5. device-A is moved to
> end of dpm_list
> 6. dpm_list is now
> [device-B, device-A]
> 7. driver-B is registereed and probes device-B.
> 8. dpm_list stays as [device-B, device-A].
>
> The reverse order of dpm_list is used for suspend. So in this case
> device-A would incorrectly get suspended before device-B.
>
> Commit 716a7a259690 ("driver core: fw_devlink: Add support for batching
> fwnode parsing") kicked off the deferred probe thread early during boot
> to run in parallel with the initcall thread and caused suspend/resume
> regressions. This patch removes the parallel run of the deferred probe
> thread to avoid the suspend/resume regressions.
>
> [1] - https://lore.kernel.org/lkml/CAGETcx8W96KAw-d_siTX4qHB_-7ddk0miYRDQeHE6E0_8qx-6Q@mail.gmail.com/
>
> Fixes: 716a7a259690 ("driver core: fw_devlink: Add support for batching fwnode parsing")
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/base/base.h | 1 -
> drivers/base/core.c | 1 -
> drivers/base/dd.c | 5 -----
> 3 files changed, 7 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 95c22c0f9036..40fb069a8a7e 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -153,7 +153,6 @@ extern char *make_class_name(const char *name, struct kobject *kobj);
> extern int devres_release_all(struct device *dev);
> extern void device_block_probing(void);
> extern void device_unblock_probing(void);
> -extern void driver_deferred_probe_force_trigger(void);
>
> /* /sys/devices directory */
> extern struct kset *devices_kset;
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 67d39a90b45c..35cc9896eb9e 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1323,7 +1323,6 @@ void fw_devlink_resume(void)
> goto out;
>
> device_link_add_missing_supplier_links();
> - driver_deferred_probe_force_trigger();
So it looks like this was not needed after all.
What was the role of it then?
> out:
> mutex_unlock(&defer_fw_devlink_lock);
> }
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940342ac..48ca81cb8ebc 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -164,11 +164,6 @@ static void driver_deferred_probe_trigger(void)
> if (!driver_deferred_probe_enable)
> return;
>
> - driver_deferred_probe_force_trigger();
> -}
> -
> -void driver_deferred_probe_force_trigger(void)
> -{
> /*
> * A successful probe means that all the devices in the pending list
> * should be triggered to be reprobed. Move all the deferred devices
> --
> 2.27.0.212.ge8ba1cc988-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/3] driver core: Don't do deferred probe in parallel with kernel_init thread
2020-07-07 15:59 ` Rafael J. Wysocki
@ 2020-07-07 21:54 ` Saravana Kannan
0 siblings, 0 replies; 10+ messages in thread
From: Saravana Kannan @ 2020-07-07 21:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Greg Kroah-Hartman, Linux Kernel Mailing List,
Geert Uytterhoeven, Cc: Android Kernel
On Tue, Jul 7, 2020 at 8:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 1, 2020 at 9:43 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > The current deferred probe implementation can mess up suspend/resume
> > ordering if deferred probe thread is kicked off in parallel with the
> > main initcall thread (kernel_init thread) [1].
> >
> > For example:
> >
> > Say device-B is a consumer of device-A.
> >
> > Initcall thread Deferred probe thread
> > =============== =====================
> > 1. device-A is added.
> > 2. device-B is added.
> > 3. dpm_list is now [device-A, device-B].
> > 4. driver-A defers probe of device-A.
> > 5. device-A is moved to
> > end of dpm_list
> > 6. dpm_list is now
> > [device-B, device-A]
> > 7. driver-B is registereed and probes device-B.
> > 8. dpm_list stays as [device-B, device-A].
> >
> > The reverse order of dpm_list is used for suspend. So in this case
> > device-A would incorrectly get suspended before device-B.
> >
> > Commit 716a7a259690 ("driver core: fw_devlink: Add support for batching
> > fwnode parsing") kicked off the deferred probe thread early during boot
> > to run in parallel with the initcall thread and caused suspend/resume
> > regressions. This patch removes the parallel run of the deferred probe
> > thread to avoid the suspend/resume regressions.
> >
> > [1] - https://lore.kernel.org/lkml/CAGETcx8W96KAw-d_siTX4qHB_-7ddk0miYRDQeHE6E0_8qx-6Q@mail.gmail.com/
> >
> > Fixes: 716a7a259690 ("driver core: fw_devlink: Add support for batching fwnode parsing")
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > drivers/base/base.h | 1 -
> > drivers/base/core.c | 1 -
> > drivers/base/dd.c | 5 -----
> > 3 files changed, 7 deletions(-)
> >
> > diff --git a/drivers/base/base.h b/drivers/base/base.h
> > index 95c22c0f9036..40fb069a8a7e 100644
> > --- a/drivers/base/base.h
> > +++ b/drivers/base/base.h
> > @@ -153,7 +153,6 @@ extern char *make_class_name(const char *name, struct kobject *kobj);
> > extern int devres_release_all(struct device *dev);
> > extern void device_block_probing(void);
> > extern void device_unblock_probing(void);
> > -extern void driver_deferred_probe_force_trigger(void);
> >
> > /* /sys/devices directory */
> > extern struct kset *devices_kset;
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 67d39a90b45c..35cc9896eb9e 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1323,7 +1323,6 @@ void fw_devlink_resume(void)
> > goto out;
> >
> > device_link_add_missing_supplier_links();
> > - driver_deferred_probe_force_trigger();
>
> So it looks like this was not needed after all.
>
> What was the role of it then?
It's needed to avoid delaying all probe till late_initcall. Patch 3/3
has more details.
-Saravana
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 2/3] driver core: Rename dev_links_info.defer_sync to defer_hook
2020-07-01 19:42 [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Saravana Kannan
2020-07-01 19:42 ` [PATCH v1 1/3] driver core: Don't do deferred probe in parallel with kernel_init thread Saravana Kannan
@ 2020-07-01 19:42 ` Saravana Kannan
2020-07-07 16:02 ` Rafael J. Wysocki
2020-07-01 19:42 ` [PATCH v1 3/3] driver core: Avoid deferred probe due to fw_devlink_pause/resume() Saravana Kannan
2020-07-07 15:56 ` [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Rafael J. Wysocki
3 siblings, 1 reply; 10+ messages in thread
From: Saravana Kannan @ 2020-07-01 19:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Saravana Kannan, linux-kernel, Geert Uytterhoeven, kernel-team
The defer_sync field is used as a hook to add the device to the
deferred_sync list. Rename it so that it's more meaningful for the next
patch that'll also use this field as a hook to a deferred_fw_devlink
list.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 22 +++++++++++-----------
include/linux/device.h | 4 ++--
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 35cc9896eb9e..d1d2cdc3a8d8 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -754,11 +754,11 @@ static void __device_links_queue_sync_state(struct device *dev,
*/
dev->state_synced = true;
- if (WARN_ON(!list_empty(&dev->links.defer_sync)))
+ if (WARN_ON(!list_empty(&dev->links.defer_hook)))
return;
get_device(dev);
- list_add_tail(&dev->links.defer_sync, list);
+ list_add_tail(&dev->links.defer_hook, list);
}
/**
@@ -776,8 +776,8 @@ static void device_links_flush_sync_list(struct list_head *list,
{
struct device *dev, *tmp;
- list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
- list_del_init(&dev->links.defer_sync);
+ list_for_each_entry_safe(dev, tmp, list, links.defer_hook) {
+ list_del_init(&dev->links.defer_hook);
if (dev != dont_lock_dev)
device_lock(dev);
@@ -815,12 +815,12 @@ void device_links_supplier_sync_state_resume(void)
if (defer_sync_state_count)
goto out;
- list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
+ list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_hook) {
/*
* Delete from deferred_sync list before queuing it to
- * sync_list because defer_sync is used for both lists.
+ * sync_list because defer_hook is used for both lists.
*/
- list_del_init(&dev->links.defer_sync);
+ list_del_init(&dev->links.defer_hook);
__device_links_queue_sync_state(dev, &sync_list);
}
out:
@@ -838,8 +838,8 @@ late_initcall(sync_state_resume_initcall);
static void __device_links_supplier_defer_sync(struct device *sup)
{
- if (list_empty(&sup->links.defer_sync) && dev_has_sync_state(sup))
- list_add_tail(&sup->links.defer_sync, &deferred_sync);
+ if (list_empty(&sup->links.defer_hook) && dev_has_sync_state(sup))
+ list_add_tail(&sup->links.defer_hook, &deferred_sync);
}
static void device_link_drop_managed(struct device_link *link)
@@ -1052,7 +1052,7 @@ void device_links_driver_cleanup(struct device *dev)
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
- list_del_init(&dev->links.defer_sync);
+ list_del_init(&dev->links.defer_hook);
__device_links_no_driver(dev);
device_links_write_unlock();
@@ -2171,7 +2171,7 @@ void device_initialize(struct device *dev)
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
INIT_LIST_HEAD(&dev->links.needs_suppliers);
- INIT_LIST_HEAD(&dev->links.defer_sync);
+ INIT_LIST_HEAD(&dev->links.defer_hook);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..9bb2bc7bb8e3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -433,7 +433,7 @@ enum dl_dev_state {
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
* @needs_suppliers: Hook to global list of devices waiting for suppliers.
- * @defer_sync: Hook to global list of devices that have deferred sync_state.
+ * @defer_hook: Hook to global list of devices that have deferred sync_state.
* @need_for_probe: If needs_suppliers is on a list, this indicates if the
* suppliers are needed for probe or not.
* @status: Driver status information.
@@ -442,7 +442,7 @@ struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
struct list_head needs_suppliers;
- struct list_head defer_sync;
+ struct list_head defer_hook;
bool need_for_probe;
enum dl_dev_state status;
};
--
2.27.0.212.ge8ba1cc988-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/3] driver core: Rename dev_links_info.defer_sync to defer_hook
2020-07-01 19:42 ` [PATCH v1 2/3] driver core: Rename dev_links_info.defer_sync to defer_hook Saravana Kannan
@ 2020-07-07 16:02 ` Rafael J. Wysocki
0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2020-07-07 16:02 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List,
Geert Uytterhoeven, Cc: Android Kernel
On Wed, Jul 1, 2020 at 9:43 PM Saravana Kannan <saravanak@google.com> wrote:
>
> The defer_sync field is used as a hook to add the device to the
> deferred_sync list. Rename it so that it's more meaningful for the next
> patch that'll also use this field as a hook to a deferred_fw_devlink
> list.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
This is a trivial rename if I'm not mistaken, so
Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
> ---
> drivers/base/core.c | 22 +++++++++++-----------
> include/linux/device.h | 4 ++--
> 2 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 35cc9896eb9e..d1d2cdc3a8d8 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -754,11 +754,11 @@ static void __device_links_queue_sync_state(struct device *dev,
> */
> dev->state_synced = true;
>
> - if (WARN_ON(!list_empty(&dev->links.defer_sync)))
> + if (WARN_ON(!list_empty(&dev->links.defer_hook)))
> return;
>
> get_device(dev);
> - list_add_tail(&dev->links.defer_sync, list);
> + list_add_tail(&dev->links.defer_hook, list);
> }
>
> /**
> @@ -776,8 +776,8 @@ static void device_links_flush_sync_list(struct list_head *list,
> {
> struct device *dev, *tmp;
>
> - list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
> - list_del_init(&dev->links.defer_sync);
> + list_for_each_entry_safe(dev, tmp, list, links.defer_hook) {
> + list_del_init(&dev->links.defer_hook);
>
> if (dev != dont_lock_dev)
> device_lock(dev);
> @@ -815,12 +815,12 @@ void device_links_supplier_sync_state_resume(void)
> if (defer_sync_state_count)
> goto out;
>
> - list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
> + list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_hook) {
> /*
> * Delete from deferred_sync list before queuing it to
> - * sync_list because defer_sync is used for both lists.
> + * sync_list because defer_hook is used for both lists.
> */
> - list_del_init(&dev->links.defer_sync);
> + list_del_init(&dev->links.defer_hook);
> __device_links_queue_sync_state(dev, &sync_list);
> }
> out:
> @@ -838,8 +838,8 @@ late_initcall(sync_state_resume_initcall);
>
> static void __device_links_supplier_defer_sync(struct device *sup)
> {
> - if (list_empty(&sup->links.defer_sync) && dev_has_sync_state(sup))
> - list_add_tail(&sup->links.defer_sync, &deferred_sync);
> + if (list_empty(&sup->links.defer_hook) && dev_has_sync_state(sup))
> + list_add_tail(&sup->links.defer_hook, &deferred_sync);
> }
>
> static void device_link_drop_managed(struct device_link *link)
> @@ -1052,7 +1052,7 @@ void device_links_driver_cleanup(struct device *dev)
> WRITE_ONCE(link->status, DL_STATE_DORMANT);
> }
>
> - list_del_init(&dev->links.defer_sync);
> + list_del_init(&dev->links.defer_hook);
> __device_links_no_driver(dev);
>
> device_links_write_unlock();
> @@ -2171,7 +2171,7 @@ void device_initialize(struct device *dev)
> INIT_LIST_HEAD(&dev->links.consumers);
> INIT_LIST_HEAD(&dev->links.suppliers);
> INIT_LIST_HEAD(&dev->links.needs_suppliers);
> - INIT_LIST_HEAD(&dev->links.defer_sync);
> + INIT_LIST_HEAD(&dev->links.defer_hook);
> dev->links.status = DL_DEV_NO_DRIVER;
> }
> EXPORT_SYMBOL_GPL(device_initialize);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 15460a5ac024..9bb2bc7bb8e3 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -433,7 +433,7 @@ enum dl_dev_state {
> * @suppliers: List of links to supplier devices.
> * @consumers: List of links to consumer devices.
> * @needs_suppliers: Hook to global list of devices waiting for suppliers.
> - * @defer_sync: Hook to global list of devices that have deferred sync_state.
> + * @defer_hook: Hook to global list of devices that have deferred sync_state.
> * @need_for_probe: If needs_suppliers is on a list, this indicates if the
> * suppliers are needed for probe or not.
> * @status: Driver status information.
> @@ -442,7 +442,7 @@ struct dev_links_info {
> struct list_head suppliers;
> struct list_head consumers;
> struct list_head needs_suppliers;
> - struct list_head defer_sync;
> + struct list_head defer_hook;
> bool need_for_probe;
> enum dl_dev_state status;
> };
> --
> 2.27.0.212.ge8ba1cc988-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 3/3] driver core: Avoid deferred probe due to fw_devlink_pause/resume()
2020-07-01 19:42 [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Saravana Kannan
2020-07-01 19:42 ` [PATCH v1 1/3] driver core: Don't do deferred probe in parallel with kernel_init thread Saravana Kannan
2020-07-01 19:42 ` [PATCH v1 2/3] driver core: Rename dev_links_info.defer_sync to defer_hook Saravana Kannan
@ 2020-07-01 19:42 ` Saravana Kannan
2020-07-07 15:56 ` [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Rafael J. Wysocki
3 siblings, 0 replies; 10+ messages in thread
From: Saravana Kannan @ 2020-07-01 19:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: linux-kernel, Geert Uytterhoeven, kernel-team
With the earlier patch in this series, all devices that deferred probe
due to fw_devlink_pause() will have their probes delayed till the
deferred probe thread is kicked off during late_initcall. This will also
affect all their consumers.
This delayed probing in unnecessary. So this patch just keeps track of
the devices that had their probe deferred due to fw_devlink_pause() and
attempts to probe them once during fw_devlink_resume().
Fixes: 716a7a259690 ("driver core: fw_devlink: Add support for batching fwnode parsing")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 21 +++++++++++++++++++++
include/linux/device.h | 3 ++-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1d2cdc3a8d8..05d414e9e8a4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -50,6 +50,7 @@ static DEFINE_MUTEX(wfs_lock);
static LIST_HEAD(deferred_sync);
static unsigned int defer_sync_state_count = 1;
static unsigned int defer_fw_devlink_count;
+static LIST_HEAD(deferred_fw_devlink);
static DEFINE_MUTEX(defer_fw_devlink_lock);
static bool fw_devlink_is_permissive(void);
@@ -1244,6 +1245,12 @@ static void fw_devlink_link_device(struct device *dev)
fw_ret = -EAGAIN;
} else {
fw_ret = -ENODEV;
+ /*
+ * defer_hook is not used to add device to deferred_sync list
+ * until device is bound. Since deferred fw devlink also blocks
+ * probing, same list hook can be used for deferred_fw_devlink.
+ */
+ list_add_tail(&dev->links.defer_hook, &deferred_fw_devlink);
}
if (fw_ret == -ENODEV)
@@ -1312,6 +1319,9 @@ void fw_devlink_pause(void)
*/
void fw_devlink_resume(void)
{
+ struct device *dev, *tmp;
+ LIST_HEAD(probe_list);
+
mutex_lock(&defer_fw_devlink_lock);
if (!defer_fw_devlink_count) {
WARN(true, "Unmatched fw_devlink pause/resume!");
@@ -1323,8 +1333,19 @@ void fw_devlink_resume(void)
goto out;
device_link_add_missing_supplier_links();
+ list_splice_tail_init(&deferred_fw_devlink, &probe_list);
out:
mutex_unlock(&defer_fw_devlink_lock);
+
+ /*
+ * bus_probe_device() can cause new devices to get added and they'll
+ * try to grab defer_fw_devlink_lock. So, this needs to be done outside
+ * the defer_fw_devlink_lock.
+ */
+ list_for_each_entry_safe(dev, tmp, &probe_list, links.defer_hook) {
+ list_del_init(&dev->links.defer_hook);
+ bus_probe_device(dev);
+ }
}
/* Device links support end. */
diff --git a/include/linux/device.h b/include/linux/device.h
index 9bb2bc7bb8e3..5efed864b387 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -433,7 +433,8 @@ enum dl_dev_state {
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
* @needs_suppliers: Hook to global list of devices waiting for suppliers.
- * @defer_hook: Hook to global list of devices that have deferred sync_state.
+ * @defer_hook: Hook to global list of devices that have deferred sync_state or
+ * deferred fw_devlink.
* @need_for_probe: If needs_suppliers is on a list, this indicates if the
* suppliers are needed for probe or not.
* @status: Driver status information.
--
2.27.0.212.ge8ba1cc988-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization
2020-07-01 19:42 [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Saravana Kannan
` (2 preceding siblings ...)
2020-07-01 19:42 ` [PATCH v1 3/3] driver core: Avoid deferred probe due to fw_devlink_pause/resume() Saravana Kannan
@ 2020-07-07 15:56 ` Rafael J. Wysocki
2020-07-08 8:16 ` Geert Uytterhoeven
3 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2020-07-07 15:56 UTC (permalink / raw)
To: Saravana Kannan, Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List,
Cc: Android Kernel
On Wed, Jul 1, 2020 at 9:43 PM Saravana Kannan <saravanak@google.com> wrote:
>
> When commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when
> adding all top level devices") enabled batch processing of fw_devlink to
> optimize the parsing time, it caused a suspend/resume regression due to
> the use of deferred probing early on at boot.
>
> This series fixes the regression by avoiding the use of deferred probing
> when optimizing fw_devlink parsing.
>
> Saravana Kannan (3):
> driver core: Don't do deferred probe in parallel with kernel_init
> thread
> driver core: Rename dev_links_info.defer_sync to defer_hook
> driver core: Avoid deferred probe due to fw_devlink_pause/resume()
>
> drivers/base/base.h | 1 -
> drivers/base/core.c | 44 ++++++++++++++++++++++++++++++------------
> drivers/base/dd.c | 5 -----
> include/linux/device.h | 5 +++--
> 4 files changed, 35 insertions(+), 20 deletions(-)
>
> --
Geert, any chance to test this series on top of 5.8-rc? It is
expected to fix the suspend/resume regression reported by you.
Cheers!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization
2020-07-07 15:56 ` [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization Rafael J. Wysocki
@ 2020-07-08 8:16 ` Geert Uytterhoeven
2020-07-08 17:36 ` Saravana Kannan
0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-07-08 8:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Saravana Kannan, Greg Kroah-Hartman, Linux Kernel Mailing List,
Cc: Android Kernel, Linux-Renesas
Hi Rafael,
On Tue, Jul 7, 2020 at 5:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jul 1, 2020 at 9:43 PM Saravana Kannan <saravanak@google.com> wrote:
> > When commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when
> > adding all top level devices") enabled batch processing of fw_devlink to
> > optimize the parsing time, it caused a suspend/resume regression due to
> > the use of deferred probing early on at boot.
> >
> > This series fixes the regression by avoiding the use of deferred probing
> > when optimizing fw_devlink parsing.
> >
> > Saravana Kannan (3):
> > driver core: Don't do deferred probe in parallel with kernel_init
> > thread
> > driver core: Rename dev_links_info.defer_sync to defer_hook
> > driver core: Avoid deferred probe due to fw_devlink_pause/resume()
> >
> > drivers/base/base.h | 1 -
> > drivers/base/core.c | 44 ++++++++++++++++++++++++++++++------------
> > drivers/base/dd.c | 5 -----
> > include/linux/device.h | 5 +++--
> > 4 files changed, 35 insertions(+), 20 deletions(-)
> >
> > --
>
> Geert, any chance to test this series on top of 5.8-rc? It is
> expected to fix the suspend/resume regression reported by you.
Sorry, I had completely forgotten I hadn't tested this iteration of
the fix(es) yet. Thanks for the reminder!
Works fine when applied on top of v5.8-rc4.
Tested on r8a7740/armadillo, sh73a0/kzm9g, r8a7791/koelsch,
and r8a77951/salvator-xs.
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/3] Fix dpm_list ordering issue due to fw_devlink optimization
2020-07-08 8:16 ` Geert Uytterhoeven
@ 2020-07-08 17:36 ` Saravana Kannan
0 siblings, 0 replies; 10+ messages in thread
From: Saravana Kannan @ 2020-07-08 17:36 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Linux Kernel Mailing List,
Cc: Android Kernel, Linux-Renesas
On Wed, Jul 8, 2020 at 1:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rafael,
>
> On Tue, Jul 7, 2020 at 5:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jul 1, 2020 at 9:43 PM Saravana Kannan <saravanak@google.com> wrote:
> > > When commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when
> > > adding all top level devices") enabled batch processing of fw_devlink to
> > > optimize the parsing time, it caused a suspend/resume regression due to
> > > the use of deferred probing early on at boot.
> > >
> > > This series fixes the regression by avoiding the use of deferred probing
> > > when optimizing fw_devlink parsing.
> > >
> > > Saravana Kannan (3):
> > > driver core: Don't do deferred probe in parallel with kernel_init
> > > thread
> > > driver core: Rename dev_links_info.defer_sync to defer_hook
> > > driver core: Avoid deferred probe due to fw_devlink_pause/resume()
> > >
> > > drivers/base/base.h | 1 -
> > > drivers/base/core.c | 44 ++++++++++++++++++++++++++++++------------
> > > drivers/base/dd.c | 5 -----
> > > include/linux/device.h | 5 +++--
> > > 4 files changed, 35 insertions(+), 20 deletions(-)
> > >
> > > --
> >
> > Geert, any chance to test this series on top of 5.8-rc? It is
> > expected to fix the suspend/resume regression reported by you.
>
> Sorry, I had completely forgotten I hadn't tested this iteration of
> the fix(es) yet. Thanks for the reminder!
>
> Works fine when applied on top of v5.8-rc4.
> Tested on r8a7740/armadillo, sh73a0/kzm9g, r8a7791/koelsch,
> and r8a77951/salvator-xs.
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Great! Thanks!
-Saravana
^ permalink raw reply [flat|nested] 10+ messages in thread