linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3] PM / core: fix deferred probe breaking suspend resume order
@ 2018-03-05 18:29 Feng Kan
  2018-03-15 18:41 ` Feng Kan
  2018-04-05 16:40 ` Rafael J. Wysocki
  0 siblings, 2 replies; 6+ messages in thread
From: Feng Kan @ 2018-03-05 18:29 UTC (permalink / raw)
  To: rjw, linux-pm, lorenzo.pieralisi, linux-arm-kernel, linux-pci,
	bhelgaas, gregkh
  Cc: Feng Kan, Toan Le

When bridge and its endpoint is enumerated the devices are added to the
dpm list. Afterward, the bridge defers probe when IOMMU is not ready.
This causes the bridge to be moved to the end of the dpm list when
deferred probe kicks in. The order of the dpm list for bridge and
endpoint is reversed.

Add reordering code to move the bridge and its children and consumers to
the end of the pm list so the order for suspend and resume is not altered.
The code also move device and its children and consumers to the tail of
device_kset list if it is registered.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Toan Le <toanle@apm.com>
---
 V3:
	1. additional code comment changes
 V2:
        1. change patch title from "move device and its children..."
        2. move define based on Bjorn's comment
        3. rename function name and comment content

 drivers/base/base.h |  3 +++
 drivers/base/core.c | 20 ++++++++++++++++++++
 drivers/base/dd.c   |  4 +---
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index d800de6..a75c302 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { }
 extern void device_links_no_driver(struct device *dev);
 extern bool device_links_busy(struct device *dev);
 extern void device_links_unbind_consumers(struct device *dev);
+
+/* device pm support */
+void device_pm_move_to_tail(struct device *dev);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 110230d..3b37906 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 }
 
 /**
+ * device_pm_move_to_tail - Move set of devices to the end of device lists
+ * @dev: current device pointer
+ *
+ * This is a device_reorder_to_tail() wrapper taking the requisite locks.
+ *
+ * It moves the device along with all of its children and all of its consumers
+ * to the ends of the device_kset and dpm_list, recursively.
+ */
+void device_pm_move_to_tail(struct device *dev)
+{
+	int idx;
+
+	idx = device_links_read_lock();
+	device_pm_lock();
+	device_reorder_to_tail(dev, NULL);
+	device_pm_unlock();
+	device_links_read_unlock(idx);
+}
+
+/**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
  * @supplier: Supplier end of the link.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 2c964f5..96fab29 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work)
 		 * the list is a good order for suspend but deferred
 		 * probe makes that very unsafe.
 		 */
-		device_pm_lock();
-		device_pm_move_last(dev);
-		device_pm_unlock();
+		device_pm_move_to_tail(dev);
 
 		dev_dbg(dev, "Retrying from deferred list\n");
 		if (initcall_debug && !initcalls_done)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH V3] PM / core: fix deferred probe breaking suspend resume order
  2018-03-05 18:29 [PATCH V3] PM / core: fix deferred probe breaking suspend resume order Feng Kan
@ 2018-03-15 18:41 ` Feng Kan
  2018-03-30 22:50   ` Bjorn Helgaas
  2018-04-05 16:40 ` Rafael J. Wysocki
  1 sibling, 1 reply; 6+ messages in thread
From: Feng Kan @ 2018-03-15 18:41 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux PM, Lorenzo Pieralisi, linux-arm-kernel,
	Linux PCI, Bjorn Helgaas, Greg Kroah-Hartman
  Cc: Feng Kan, Toan Le

On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote:
> When bridge and its endpoint is enumerated the devices are added to the
> dpm list. Afterward, the bridge defers probe when IOMMU is not ready.
> This causes the bridge to be moved to the end of the dpm list when
> deferred probe kicks in. The order of the dpm list for bridge and
> endpoint is reversed.
>
> Add reordering code to move the bridge and its children and consumers to
> the end of the pm list so the order for suspend and resume is not altered.
> The code also move device and its children and consumers to the tail of
> device_kset list if it is registered.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Toan Le <toanle@apm.com>
> ---
>  V3:
>         1. additional code comment changes
>  V2:
>         1. change patch title from "move device and its children..."
>         2. move define based on Bjorn's comment
>         3. rename function name and comment content
>
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c | 20 ++++++++++++++++++++
>  drivers/base/dd.c   |  4 +---
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index d800de6..a75c302 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { }
>  extern void device_links_no_driver(struct device *dev);
>  extern bool device_links_busy(struct device *dev);
>  extern void device_links_unbind_consumers(struct device *dev);
> +
> +/* device pm support */
> +void device_pm_move_to_tail(struct device *dev);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 110230d..3b37906 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
>  }
>
>  /**
> + * device_pm_move_to_tail - Move set of devices to the end of device lists
> + * @dev: current device pointer
> + *
> + * This is a device_reorder_to_tail() wrapper taking the requisite locks.
> + *
> + * It moves the device along with all of its children and all of its consumers
> + * to the ends of the device_kset and dpm_list, recursively.
> + */
> +void device_pm_move_to_tail(struct device *dev)
> +{
> +       int idx;
> +
> +       idx = device_links_read_lock();
> +       device_pm_lock();
> +       device_reorder_to_tail(dev, NULL);
> +       device_pm_unlock();
> +       device_links_read_unlock(idx);
> +}
> +
> +/**
>   * device_link_add - Create a link between two devices.
>   * @consumer: Consumer end of the link.
>   * @supplier: Supplier end of the link.
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 2c964f5..96fab29 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work)
>                  * the list is a good order for suspend but deferred
>                  * probe makes that very unsafe.
>                  */
> -               device_pm_lock();
> -               device_pm_move_last(dev);
> -               device_pm_unlock();
> +               device_pm_move_to_tail(dev);
>
>                 dev_dbg(dev, "Retrying from deferred list\n");
>                 if (initcall_debug && !initcalls_done)
> --
> 1.8.3.1
>
Hi, just wanted to check if there is anything else I need to change on
this. Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V3] PM / core: fix deferred probe breaking suspend resume order
  2018-03-15 18:41 ` Feng Kan
@ 2018-03-30 22:50   ` Bjorn Helgaas
  2018-04-04 21:10     ` Feng Kan
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2018-03-30 22:50 UTC (permalink / raw)
  To: Feng Kan
  Cc: Lorenzo Pieralisi, Linux PM, Linux PCI, Rafael J. Wysocki,
	Greg Kroah-Hartman, Bjorn Helgaas, Toan Le, linux-arm-kernel

On Thu, Mar 15, 2018 at 11:41:33AM -0700, Feng Kan wrote:
> On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote:
> > When bridge and its endpoint is enumerated the devices are added to the
> > dpm list. Afterward, the bridge defers probe when IOMMU is not ready.
> > This causes the bridge to be moved to the end of the dpm list when
> > deferred probe kicks in. The order of the dpm list for bridge and
> > endpoint is reversed.
> >
> > Add reordering code to move the bridge and its children and consumers to
> > the end of the pm list so the order for suspend and resume is not altered.
> > The code also move device and its children and consumers to the tail of
> > device_kset list if it is registered.
> >
> > Signed-off-by: Feng Kan <fkan@apm.com>
> > Signed-off-by: Toan Le <toanle@apm.com>
> > ...
> >
> Hi, just wanted to check if there is anything else I need to change on
> this. Thanks.

I'm assuming Rafael will handle this.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V3] PM / core: fix deferred probe breaking suspend resume order
  2018-03-30 22:50   ` Bjorn Helgaas
@ 2018-04-04 21:10     ` Feng Kan
  2018-04-05 16:38       ` Rafael J. Wysocki
  0 siblings, 1 reply; 6+ messages in thread
From: Feng Kan @ 2018-04-04 21:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Lorenzo Pieralisi, Linux PM, Linux PCI, Rafael J. Wysocki,
	Greg Kroah-Hartman, Bjorn Helgaas, Toan Le, linux-arm-kernel

On Fri, Mar 30, 2018 at 3:50 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Mar 15, 2018 at 11:41:33AM -0700, Feng Kan wrote:
>> On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote:
>> > When bridge and its endpoint is enumerated the devices are added to the
>> > dpm list. Afterward, the bridge defers probe when IOMMU is not ready.
>> > This causes the bridge to be moved to the end of the dpm list when
>> > deferred probe kicks in. The order of the dpm list for bridge and
>> > endpoint is reversed.
>> >
>> > Add reordering code to move the bridge and its children and consumers to
>> > the end of the pm list so the order for suspend and resume is not altered.
>> > The code also move device and its children and consumers to the tail of
>> > device_kset list if it is registered.
>> >
>> > Signed-off-by: Feng Kan <fkan@apm.com>
>> > Signed-off-by: Toan Le <toanle@apm.com>
>> > ...
>> >
>> Hi, just wanted to check if there is anything else I need to change on
>> this. Thanks.
>
> I'm assuming Rafael will handle this.
Rafael, sorry to ping again. Do you have a position on this patch yet?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V3] PM / core: fix deferred probe breaking suspend resume order
  2018-04-04 21:10     ` Feng Kan
@ 2018-04-05 16:38       ` Rafael J. Wysocki
  0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2018-04-05 16:38 UTC (permalink / raw)
  To: Feng Kan
  Cc: Lorenzo Pieralisi, Linux PM, Linux PCI, Bjorn Helgaas,
	Greg Kroah-Hartman, Bjorn Helgaas, Toan Le, linux-arm-kernel

On Wednesday, April 4, 2018 11:10:41 PM CEST Feng Kan wrote:
> On Fri, Mar 30, 2018 at 3:50 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Mar 15, 2018 at 11:41:33AM -0700, Feng Kan wrote:
> >> On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote:
> >> > When bridge and its endpoint is enumerated the devices are added to the
> >> > dpm list. Afterward, the bridge defers probe when IOMMU is not ready.
> >> > This causes the bridge to be moved to the end of the dpm list when
> >> > deferred probe kicks in. The order of the dpm list for bridge and
> >> > endpoint is reversed.
> >> >
> >> > Add reordering code to move the bridge and its children and consumers to
> >> > the end of the pm list so the order for suspend and resume is not altered.
> >> > The code also move device and its children and consumers to the tail of
> >> > device_kset list if it is registered.
> >> >
> >> > Signed-off-by: Feng Kan <fkan@apm.com>
> >> > Signed-off-by: Toan Le <toanle@apm.com>
> >> > ...
> >> >
> >> Hi, just wanted to check if there is anything else I need to change on
> >> this. Thanks.
> >
> > I'm assuming Rafael will handle this.
> Rafael, sorry to ping again. Do you have a position on this patch yet?

Sorry, I thought that Greg might want to send a comment here.

I'm basically fine with the approach.  I only have two minor remarks to the
kernedoc comment of the new function, but let me reply directly to the patch
message.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V3] PM / core: fix deferred probe breaking suspend resume order
  2018-03-05 18:29 [PATCH V3] PM / core: fix deferred probe breaking suspend resume order Feng Kan
  2018-03-15 18:41 ` Feng Kan
@ 2018-04-05 16:40 ` Rafael J. Wysocki
  1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2018-04-05 16:40 UTC (permalink / raw)
  To: Feng Kan
  Cc: lorenzo.pieralisi, linux-pm, linux-pci, gregkh, bhelgaas,
	Toan Le, linux-arm-kernel

On Monday, March 5, 2018 7:29:39 PM CEST Feng Kan wrote:
> When bridge and its endpoint is enumerated the devices are added to the
> dpm list. Afterward, the bridge defers probe when IOMMU is not ready.
> This causes the bridge to be moved to the end of the dpm list when
> deferred probe kicks in. The order of the dpm list for bridge and
> endpoint is reversed.
> 
> Add reordering code to move the bridge and its children and consumers to
> the end of the pm list so the order for suspend and resume is not altered.
> The code also move device and its children and consumers to the tail of
> device_kset list if it is registered.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Toan Le <toanle@apm.com>
> ---
>  V3:
> 	1. additional code comment changes
>  V2:
>         1. change patch title from "move device and its children..."
>         2. move define based on Bjorn's comment
>         3. rename function name and comment content
> 
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c | 20 ++++++++++++++++++++
>  drivers/base/dd.c   |  4 +---
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index d800de6..a75c302 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { }
>  extern void device_links_no_driver(struct device *dev);
>  extern bool device_links_busy(struct device *dev);
>  extern void device_links_unbind_consumers(struct device *dev);
> +
> +/* device pm support */
> +void device_pm_move_to_tail(struct device *dev);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 110230d..3b37906 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
>  }
>  
>  /**
> + * device_pm_move_to_tail - Move set of devices to the end of device lists
> + * @dev: current device pointer

What does "current" mean here, exactly?

I guess it would be better to say "Device to move" or similar here.

> + *
> + * This is a device_reorder_to_tail() wrapper taking the requisite locks.
> + *
> + * It moves the device along with all of its children and all of its consumers

"The device" is @dev, so it's better to use @dev here, like

"It moves @dev along with ..."

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-04-05 16:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05 18:29 [PATCH V3] PM / core: fix deferred probe breaking suspend resume order Feng Kan
2018-03-15 18:41 ` Feng Kan
2018-03-30 22:50   ` Bjorn Helgaas
2018-04-04 21:10     ` Feng Kan
2018-04-05 16:38       ` Rafael J. Wysocki
2018-04-05 16:40 ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).