Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1222085 > unrolled thread

[PATCH] driver core: Ensure proper suspend/resume ordering

Back to article view | Back to linux.kernel

Contents

  [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-10 12:20 +0200
    Re: [PATCH] driver core: Ensure proper suspend/resume ordering Grygorii Strashko <grygorii.strashko@ti.com> - 2015-09-10 13:00 +0200
    Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-10 23:50 +0200
      Re: [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-11 14:10 +0200
        Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-11 21:10 +0200
          Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-12 00:10 +0200
            Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-12 19:50 +0200
              Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-15 02:50 +0200
                Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-15 16:30 +0200
                  Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-16 02:40 +0200
            Re: [PATCH] driver core: Ensure proper suspend/resume ordering Grygorii Strashko <grygorii.strashko@ti.com> - 2015-09-16 15:50 +0200
              Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-16 21:30 +0200
                Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-17 01:40 +0200
                  Re: [PATCH] driver core: Ensure proper suspend/resume ordering Grygorii Strashko <grygorii.strashko@ti.com> - 2015-09-17 17:50 +0200
                    Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-18 02:00 +0200
                      Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-18 02:10 +0200
          Re: [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-15 18:20 +0200
            Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-15 21:20 +0200
              Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-16 03:10 +0200
                Re: [PATCH] driver core: Ensure proper suspend/resume ordering Grygorii Strashko <grygorii.strashko@ti.com> - 2015-09-16 15:40 +0200
                  Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-16 19:00 +0200
                Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-16 19:10 +0200
              Re: [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-16 15:20 +0200
                Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-16 21:30 +0200
                  Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-17 02:00 +0200
                    Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-17 04:10 +0200
                      Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-17 19:10 +0200
                        Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-17 20:50 +0200
                          Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-17 23:10 +0200
                            Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-18 02:20 +0200
                          Re: [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-18 18:00 +0200
                            Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-19 01:10 +0200
                              Re: [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-21 11:00 +0200
                                Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-21 16:40 +0200
                                  Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-22 02:00 +0200
                                    Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-22 03:30 +0200
                                Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-22 02:40 +0200
                    Re: [PATCH] driver core: Ensure proper suspend/resume ordering Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2015-09-17 07:50 +0200
                      Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-17 20:20 +0200
                        Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-17 21:30 +0200
        Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-12 00:20 +0200
          Re: [PATCH] driver core: Ensure proper suspend/resume ordering Thierry Reding <thierry.reding@gmail.com> - 2015-09-15 18:00 +0200
            Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-16 03:00 +0200
              Re: [PATCH] driver core: Ensure proper suspend/resume ordering Grygorii Strashko <grygorii.strashko@ti.com> - 2015-09-16 15:40 +0200
                Re: [PATCH] driver core: Ensure proper suspend/resume ordering "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-17 02:30 +0200
      Re: [PATCH] driver core: Ensure proper suspend/resume ordering Alan Stern <stern@rowland.harvard.edu> - 2015-09-11 16:10 +0200

Page 1 of 3  [1] 2 3  Next page →

#1222085 — [PATCH] driver core: Ensure proper suspend/resume ordering

From: Thierry Reding <treding@nvidia.com>

Deferred probe can lead to strange situations where a device that is a
dependency for others will be moved to the end of the dpm_list. At the
same time the dependers may not be moved because at the time they will
be probed the dependee may already have been successfully reprobed and
they will not have to defer the probe themselves.

One example where this happens is the Jetson TK1 board (Tegra124). The
gpio-keys driver exposes the power key of the board as an input device
that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
tegra: Add gpio-ranges property") results in the gpio-tegra driver
deferring probe because one of its dependencies, the pinctrl-tegra
driver, has not successfully completed probing. Currently the deferred
probe code will move the corresponding gpio-tegra device to the end of
the dpm_list, but by the time the gpio-keys device, depending on the
gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
the gpio-keys device is not moved to the end of dpm_list itself. As a
result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
gpio-tegra. That's problematic because the gpio-keys driver requests
the power key to be a wakeup source. However, the programming of the
wakeup interrupt registers happens in the gpio-tegra driver's suspend
callback, which is now called before that of the gpio-keys driver. The
result is that the wrong values are programmed and leaves the system
unable to be resumed using the power key.

To fix this situation, always move devices to the end of the dpm_list
before probing them. Technically this should only be done for devices
that have been successfully probed, but that won't work for recursive
probing of devices (think an I2C master that instantiates children in
its ->probe()). Effectively the dpm_list will end up ordered the same
way that devices were probed, hence taking care of dependencies.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Note that this commit is kind of the PM equivalent of 52cdbdd49853
("driver core: correct device's shutdown order) and that we have two
lists that are essentially the same (dpm_list and devices_kset). I'm
wondering if it would be worth looking into getting rid of one of
them? I don't see any reason why the ordering for shutdown and
suspend/resume should be different, and having a single list would
help keep this in sync.

 drivers/base/dd.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..56291b11049b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -88,16 +88,6 @@ static void deferred_probe_work_func(struct work_struct *work)
 		 */
 		mutex_unlock(&deferred_probe_mutex);
 
-		/*
-		 * Force the device to the end of the dpm_list since
-		 * the PM code assumes that the order we add things to
-		 * 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();
-
 		dev_dbg(dev, "Retrying from deferred list\n");
 		bus_probe_device(dev);
 
@@ -312,6 +302,29 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	 */
 	devices_kset_move_last(dev);
 
+	/*
+	 * Force the device to the end of the dpm_list since the PM code
+	 * assumes that the order we add things to the list is a good order
+	 * for suspend but deferred probe makes that very unsafe.
+	 *
+	 * Deferred probe can also cause situations in which a device that is
+	 * a dependency for others gets moved further down the dpm_list as a
+	 * result of probe deferral. In that case the dependee will end up
+	 * getting suspended before any of its dependers.
+	 *
+	 * To ensure proper ordering of suspend/resume, move every device that
+	 * is being probed to the end of the dpm_list. Note that technically
+	 * only successfully probed devices need to be moved, but that breaks
+	 * for recursively added devices because they would end up in the list
+	 * in reverse of the desired order, so we simply do it unconditionally
+	 * for all devices before they are being probed. In the worst case the
+	 * list will be reordered a couple more times than necessary, which
+	 * should be an insignificant amount of work.
+	 */
+	device_pm_lock();
+	device_pm_move_last(dev);
+	device_pm_unlock();
+
 	if (dev->bus->probe) {
 		ret = dev->bus->probe(dev);
 		if (ret)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1222111

On 09/10/2015 01:19 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Deferred probe can lead to strange situations where a device that is a
> dependency for others will be moved to the end of the dpm_list. At the
> same time the dependers may not be moved because at the time they will
> be probed the dependee may already have been successfully reprobed and
> they will not have to defer the probe themselves.
> 
> One example where this happens is the Jetson TK1 board (Tegra124). The
> gpio-keys driver exposes the power key of the board as an input device
> that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> tegra: Add gpio-ranges property") results in the gpio-tegra driver
> deferring probe because one of its dependencies, the pinctrl-tegra
> driver, has not successfully completed probing. Currently the deferred
> probe code will move the corresponding gpio-tegra device to the end of
> the dpm_list, but by the time the gpio-keys device, depending on the
> gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> the gpio-keys device is not moved to the end of dpm_list itself. As a
> result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> gpio-tegra. That's problematic because the gpio-keys driver requests
> the power key to be a wakeup source. However, the programming of the
> wakeup interrupt registers happens in the gpio-tegra driver's suspend
> callback, which is now called before that of the gpio-keys driver. The
> result is that the wrong values are programmed and leaves the system
> unable to be resumed using the power key.
> 
> To fix this situation, always move devices to the end of the dpm_list
> before probing them. Technically this should only be done for devices
> that have been successfully probed, but that won't work for recursive
> probing of devices (think an I2C master that instantiates children in
> its ->probe()). Effectively the dpm_list will end up ordered the same
> way that devices were probed, hence taking care of dependencies.

Second try :), first one was here:
"[RFC 1/1] driver core: re-order dpm_list after a succussful probe"
https://lkml.org/lkml/2014/12/12/324
and it was unsuccessful exactly because dpm_list was reordered after probe()
and not before, i think.

I'll try to test it.

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Note that this commit is kind of the PM equivalent of 52cdbdd49853
> ("driver core: correct device's shutdown order) and that we have two
> lists that are essentially the same (dpm_list and devices_kset). I'm
> wondering if it would be worth looking into getting rid of one of
> them? I don't see any reason why the ordering for shutdown and
> suspend/resume should be different, and having a single list would
> help keep this in sync.

Yep. I've tried to remove one of those lists while working on shutdown issue.
I've tried to drop dmp_list, but my try was unsuccessful, because I was not
able to find simple way to handle device's  dynamic creation/removal during
suspend/resume :(.
Also note, dmp_list's presence depends on Kconfig options.

> 
>   drivers/base/dd.c | 33 +++++++++++++++++++++++----------
>   1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb4639128..56291b11049b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -88,16 +88,6 @@ static void deferred_probe_work_func(struct work_struct *work)
>   		 */
>   		mutex_unlock(&deferred_probe_mutex);
>   
> -		/*
> -		 * Force the device to the end of the dpm_list since
> -		 * the PM code assumes that the order we add things to
> -		 * 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();
> -
>   		dev_dbg(dev, "Retrying from deferred list\n");
>   		bus_probe_device(dev);
>   
> @@ -312,6 +302,29 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>   	 */
>   	devices_kset_move_last(dev);
>   
> +	/*
> +	 * Force the device to the end of the dpm_list since the PM code
> +	 * assumes that the order we add things to the list is a good order
> +	 * for suspend but deferred probe makes that very unsafe.
> +	 *
> +	 * Deferred probe can also cause situations in which a device that is
> +	 * a dependency for others gets moved further down the dpm_list as a
> +	 * result of probe deferral. In that case the dependee will end up
> +	 * getting suspended before any of its dependers.
> +	 *
> +	 * To ensure proper ordering of suspend/resume, move every device that
> +	 * is being probed to the end of the dpm_list. Note that technically
> +	 * only successfully probed devices need to be moved, but that breaks
> +	 * for recursively added devices because they would end up in the list
> +	 * in reverse of the desired order, so we simply do it unconditionally
> +	 * for all devices before they are being probed. In the worst case the
> +	 * list will be reordered a couple more times than necessary, which
> +	 * should be an insignificant amount of work.
> +	 */
> +	device_pm_lock();
> +	device_pm_move_last(dev);
> +	device_pm_unlock();
> +
>   	if (dev->bus->probe) {
>   		ret = dev->bus->probe(dev);
>   		if (ret)
> 


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222401

On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Deferred probe can lead to strange situations where a device that is a
> dependency for others will be moved to the end of the dpm_list. At the
> same time the dependers may not be moved because at the time they will
> be probed the dependee may already have been successfully reprobed and
> they will not have to defer the probe themselves.

So there's a bug in the implementation of deferred probing IMO.

> One example where this happens is the Jetson TK1 board (Tegra124). The
> gpio-keys driver exposes the power key of the board as an input device
> that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> tegra: Add gpio-ranges property") results in the gpio-tegra driver
> deferring probe because one of its dependencies, the pinctrl-tegra
> driver, has not successfully completed probing. Currently the deferred
> probe code will move the corresponding gpio-tegra device to the end of
> the dpm_list, but by the time the gpio-keys device, depending on the
> gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> the gpio-keys device is not moved to the end of dpm_list itself. As a
> result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> gpio-tegra. That's problematic because the gpio-keys driver requests
> the power key to be a wakeup source. However, the programming of the
> wakeup interrupt registers happens in the gpio-tegra driver's suspend
> callback, which is now called before that of the gpio-keys driver. The
> result is that the wrong values are programmed and leaves the system
> unable to be resumed using the power key.
> 
> To fix this situation, always move devices to the end of the dpm_list
> before probing them. Technically this should only be done for devices
> that have been successfully probed, but that won't work for recursive
> probing of devices (think an I2C master that instantiates children in
> its ->probe()). Effectively the dpm_list will end up ordered the same
> way that devices were probed, hence taking care of dependencies.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Note that this commit is kind of the PM equivalent of 52cdbdd49853
> ("driver core: correct device's shutdown order) and that we have two
> lists that are essentially the same (dpm_list and devices_kset). I'm
> wondering if it would be worth looking into getting rid of one of
> them? I don't see any reason why the ordering for shutdown and
> suspend/resume should be different, and having a single list would
> help keep this in sync.

We move away things from dpm_list during suspend and add them back to it
during resume to handle the situations in which some devices go away or
appear during suspend/resume.  That makes this idea potentially problematic.

> 
>  drivers/base/dd.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb4639128..56291b11049b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -88,16 +88,6 @@ static void deferred_probe_work_func(struct work_struct *work)
>  		 */
>  		mutex_unlock(&deferred_probe_mutex);
>  
> -		/*
> -		 * Force the device to the end of the dpm_list since
> -		 * the PM code assumes that the order we add things to
> -		 * 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();
> -
>  		dev_dbg(dev, "Retrying from deferred list\n");
>  		bus_probe_device(dev);
>  
> @@ -312,6 +302,29 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	 */
>  	devices_kset_move_last(dev);
>  
> +	/*
> +	 * Force the device to the end of the dpm_list since the PM code
> +	 * assumes that the order we add things to the list is a good order
> +	 * for suspend but deferred probe makes that very unsafe.
> +	 *
> +	 * Deferred probe can also cause situations in which a device that is
> +	 * a dependency for others gets moved further down the dpm_list as a
> +	 * result of probe deferral. In that case the dependee will end up
> +	 * getting suspended before any of its dependers.
> +	 *
> +	 * To ensure proper ordering of suspend/resume, move every device that
> +	 * is being probed to the end of the dpm_list. Note that technically
> +	 * only successfully probed devices need to be moved, but that breaks
> +	 * for recursively added devices because they would end up in the list
> +	 * in reverse of the desired order, so we simply do it unconditionally
> +	 * for all devices before they are being probed. In the worst case the
> +	 * list will be reordered a couple more times than necessary, which
> +	 * should be an insignificant amount of work.
> +	 */
> +	device_pm_lock();
> +	device_pm_move_last(dev);
> +	device_pm_unlock();

So I don't agree with doing that for every driver being probed against the
same device.  That's just wasteful IMO.

> +
>  	if (dev->bus->probe) {
>  		ret = dev->bus->probe(dev);
>  		if (ret)
> 

Alan, what do you think about this?

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222758

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 11, 2015 at 12:08:02AM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Deferred probe can lead to strange situations where a device that is a
> > dependency for others will be moved to the end of the dpm_list. At the
> > same time the dependers may not be moved because at the time they will
> > be probed the dependee may already have been successfully reprobed and
> > they will not have to defer the probe themselves.
> 
> So there's a bug in the implementation of deferred probing IMO.

Well, yeah. The root problem here is that we don't have dependency
information and deferred probing is supposed to fix that. It does so
fairly well, but it breaks in this particular case.

> > One example where this happens is the Jetson TK1 board (Tegra124). The
> > gpio-keys driver exposes the power key of the board as an input device
> > that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> > tegra: Add gpio-ranges property") results in the gpio-tegra driver
> > deferring probe because one of its dependencies, the pinctrl-tegra
> > driver, has not successfully completed probing. Currently the deferred
> > probe code will move the corresponding gpio-tegra device to the end of
> > the dpm_list, but by the time the gpio-keys device, depending on the
> > gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> > the gpio-keys device is not moved to the end of dpm_list itself. As a
> > result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> > gpio-tegra. That's problematic because the gpio-keys driver requests
> > the power key to be a wakeup source. However, the programming of the
> > wakeup interrupt registers happens in the gpio-tegra driver's suspend
> > callback, which is now called before that of the gpio-keys driver. The
> > result is that the wrong values are programmed and leaves the system
> > unable to be resumed using the power key.
> > 
> > To fix this situation, always move devices to the end of the dpm_list
> > before probing them. Technically this should only be done for devices
> > that have been successfully probed, but that won't work for recursive
> > probing of devices (think an I2C master that instantiates children in
> > its ->probe()). Effectively the dpm_list will end up ordered the same
> > way that devices were probed, hence taking care of dependencies.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Note that this commit is kind of the PM equivalent of 52cdbdd49853
> > ("driver core: correct device's shutdown order) and that we have two
> > lists that are essentially the same (dpm_list and devices_kset). I'm
> > wondering if it would be worth looking into getting rid of one of
> > them? I don't see any reason why the ordering for shutdown and
> > suspend/resume should be different, and having a single list would
> > help keep this in sync.
> 
> We move away things from dpm_list during suspend and add them back to it
> during resume to handle the situations in which some devices go away or
> appear during suspend/resume.  That makes this idea potentially problematic.

Okay, I see. If they are used for different purposes it's fine to keep
them both.

> >  drivers/base/dd.c | 33 +++++++++++++++++++++++----------
> >  1 file changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index be0eb4639128..56291b11049b 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -88,16 +88,6 @@ static void deferred_probe_work_func(struct work_struct *work)
> >  		 */
> >  		mutex_unlock(&deferred_probe_mutex);
> >  
> > -		/*
> > -		 * Force the device to the end of the dpm_list since
> > -		 * the PM code assumes that the order we add things to
> > -		 * 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();
> > -
> >  		dev_dbg(dev, "Retrying from deferred list\n");
> >  		bus_probe_device(dev);
> >  
> > @@ -312,6 +302,29 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >  	 */
> >  	devices_kset_move_last(dev);
> >  
> > +	/*
> > +	 * Force the device to the end of the dpm_list since the PM code
> > +	 * assumes that the order we add things to the list is a good order
> > +	 * for suspend but deferred probe makes that very unsafe.
> > +	 *
> > +	 * Deferred probe can also cause situations in which a device that is
> > +	 * a dependency for others gets moved further down the dpm_list as a
> > +	 * result of probe deferral. In that case the dependee will end up
> > +	 * getting suspended before any of its dependers.
> > +	 *
> > +	 * To ensure proper ordering of suspend/resume, move every device that
> > +	 * is being probed to the end of the dpm_list. Note that technically
> > +	 * only successfully probed devices need to be moved, but that breaks
> > +	 * for recursively added devices because they would end up in the list
> > +	 * in reverse of the desired order, so we simply do it unconditionally
> > +	 * for all devices before they are being probed. In the worst case the
> > +	 * list will be reordered a couple more times than necessary, which
> > +	 * should be an insignificant amount of work.
> > +	 */
> > +	device_pm_lock();
> > +	device_pm_move_last(dev);
> > +	device_pm_unlock();
> 
> So I don't agree with doing that for every driver being probed against the
> same device.  That's just wasteful IMO.

I don't understand. At this point driver matching has already taken
place, so this will only every happen for one particular driver and the
corresponding device. In the most common case this will happen exactly
once, when the device is probed. Worst case it will happen a second or
more times if a driver defers probing for a specific device. But in that
case moving the device to the end of the list is absolutely required to
keep it properly ordered for suspend/resume. Note that this is already
done by the original code in deferred_probe_work_func() that is removed
in the first hunk. The fix here is to do it for every device to ensure
that inter-device dependencies are properly dealt with.

I agree that it's a little wasteful, but that's completely in line with
deferred probe. It's a simple solution to a very difficult problem, so
naturally it comes at a cost. But it's also fairly elegant in how it
correctly solves the ordering problem with very little code. The only
other way that I can think of to avoid reordering the list for every
device probe would be to sort it in advance using dependency information
which we don't have. So we'd need to first add all that dependency
information, and using that information is likely to be more work than
simply reordering the list for each probe.

Thierry

[toc] | [prev] | [next] | [standalone]

#1223053

On Fri, 11 Sep 2015, Thierry Reding wrote:

> On Fri, Sep 11, 2015 at 12:08:02AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Deferred probe can lead to strange situations where a device that is a
> > > dependency for others will be moved to the end of the dpm_list. At the
> > > same time the dependers may not be moved because at the time they will
> > > be probed the dependee may already have been successfully reprobed and
> > > they will not have to defer the probe themselves.
> > 
> > So there's a bug in the implementation of deferred probing IMO.
> 
> Well, yeah. The root problem here is that we don't have dependency
> information and deferred probing is supposed to fix that. It does so
> fairly well, but it breaks in this particular case.
> 
> > > One example where this happens is the Jetson TK1 board (Tegra124). The
> > > gpio-keys driver exposes the power key of the board as an input device
> > > that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> > > tegra: Add gpio-ranges property") results in the gpio-tegra driver
> > > deferring probe because one of its dependencies, the pinctrl-tegra
> > > driver, has not successfully completed probing. Currently the deferred
> > > probe code will move the corresponding gpio-tegra device to the end of
> > > the dpm_list, but by the time the gpio-keys device, depending on the
> > > gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> > > the gpio-keys device is not moved to the end of dpm_list itself. As a
> > > result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> > > gpio-tegra. That's problematic because the gpio-keys driver requests
> > > the power key to be a wakeup source. However, the programming of the
> > > wakeup interrupt registers happens in the gpio-tegra driver's suspend
> > > callback, which is now called before that of the gpio-keys driver. The
> > > result is that the wrong values are programmed and leaves the system
> > > unable to be resumed using the power key.
> > > 
> > > To fix this situation, always move devices to the end of the dpm_list
> > > before probing them. Technically this should only be done for devices
> > > that have been successfully probed, but that won't work for recursive
> > > probing of devices (think an I2C master that instantiates children in
> > > its ->probe()). Effectively the dpm_list will end up ordered the same
> > > way that devices were probed, hence taking care of dependencies.

I'm not worried about the overhead involved in moving a device to the 
end of the list every time it is probed.  That ought to be relatively 
small.

There are a few things to watch out for.  Since the dpm_list gets
modified during system sleep transitions, we would have to make sure
that nothing gets probed during those times.  In principle, that's what
the "prepare" stage is meant for, but there's still a race.  As long as
no other kernel thread (such as the deferred probing mechanism) tries
to probe a device once everything has been frozen, we should be okay.
But if not, there will be trouble -- after the ->prepare callback runs, 
the device is no longer on the dpm_list and so we don't want this patch 
to put it back on that list.

There's also an issue about other types of dependencies.  For instance,
it's conceivable that device B might be discovered and depend on device
A, even before A has been bound to a driver.  (B might be discovered by 
A's subsystem rather than A's driver.)  In that case, moving A to the 
end of the list would cause B to come before A even though B depends on 
A.  Of course, deferred probing already has this problem.

An easy way to check for this sort of thing would be to verify that a 
device about to be probed doesn't have any children.  This wouldn't 
catch all the potential dependencies, but it would be a reasonable 
start.  Do we currently check that after a device has been unbound, it 
doesn't have any remaining children?  We should do that too.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223125

On Friday, September 11, 2015 03:01:14 PM Alan Stern wrote:
> On Fri, 11 Sep 2015, Thierry Reding wrote:
> 
> > On Fri, Sep 11, 2015 at 12:08:02AM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Deferred probe can lead to strange situations where a device that is a
> > > > dependency for others will be moved to the end of the dpm_list. At the
> > > > same time the dependers may not be moved because at the time they will
> > > > be probed the dependee may already have been successfully reprobed and
> > > > they will not have to defer the probe themselves.
> > > 
> > > So there's a bug in the implementation of deferred probing IMO.
> > 
> > Well, yeah. The root problem here is that we don't have dependency
> > information and deferred probing is supposed to fix that. It does so
> > fairly well, but it breaks in this particular case.
> > 
> > > > One example where this happens is the Jetson TK1 board (Tegra124). The
> > > > gpio-keys driver exposes the power key of the board as an input device
> > > > that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> > > > tegra: Add gpio-ranges property") results in the gpio-tegra driver
> > > > deferring probe because one of its dependencies, the pinctrl-tegra
> > > > driver, has not successfully completed probing. Currently the deferred
> > > > probe code will move the corresponding gpio-tegra device to the end of
> > > > the dpm_list, but by the time the gpio-keys device, depending on the
> > > > gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> > > > the gpio-keys device is not moved to the end of dpm_list itself. As a
> > > > result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> > > > gpio-tegra. That's problematic because the gpio-keys driver requests
> > > > the power key to be a wakeup source. However, the programming of the
> > > > wakeup interrupt registers happens in the gpio-tegra driver's suspend
> > > > callback, which is now called before that of the gpio-keys driver. The
> > > > result is that the wrong values are programmed and leaves the system
> > > > unable to be resumed using the power key.
> > > > 
> > > > To fix this situation, always move devices to the end of the dpm_list
> > > > before probing them. Technically this should only be done for devices
> > > > that have been successfully probed, but that won't work for recursive
> > > > probing of devices (think an I2C master that instantiates children in
> > > > its ->probe()). Effectively the dpm_list will end up ordered the same
> > > > way that devices were probed, hence taking care of dependencies.
> 
> I'm not worried about the overhead involved in moving a device to the 
> end of the list every time it is probed.  That ought to be relatively 
> small.
> 
> There are a few things to watch out for.  Since the dpm_list gets
> modified during system sleep transitions, we would have to make sure
> that nothing gets probed during those times.  In principle, that's what
> the "prepare" stage is meant for, but there's still a race.  As long as
> no other kernel thread (such as the deferred probing mechanism) tries
> to probe a device once everything has been frozen, we should be okay.
> But if not, there will be trouble -- after the ->prepare callback runs, 
> the device is no longer on the dpm_list and so we don't want this patch 
> to put it back on that list.

Right.

> There's also an issue about other types of dependencies.  For instance,
> it's conceivable that device B might be discovered and depend on device
> A, even before A has been bound to a driver.  (B might be discovered by 
> A's subsystem rather than A's driver.)  In that case, moving A to the 
> end of the list would cause B to come before A even though B depends on 
> A.  Of course, deferred probing already has this problem.

That may actually happen for PCIe ports and devices below them AFAICS.

Devices below PCIe ports are discovered by the PCI subsystem and the PCIe
ports need not be probed before those devices are probed.

> An easy way to check for this sort of thing would be to verify that a 
> device about to be probed doesn't have any children.  This wouldn't 
> catch all the potential dependencies, but it would be a reasonable 
> start.

That would address the PCIe ports issue.

> Do we currently check that after a device has been unbound, it 
> doesn't have any remaining children?  We should do that too.

We don't and if the device is something like a PCIe port, it'd be OK for it
to still have active children after unbind.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223438

On Sat, 12 Sep 2015, Rafael J. Wysocki wrote:

> On Friday, September 11, 2015 03:01:14 PM Alan Stern wrote:

> > There's also an issue about other types of dependencies.  For instance,
> > it's conceivable that device B might be discovered and depend on device
> > A, even before A has been bound to a driver.  (B might be discovered by 
> > A's subsystem rather than A's driver.)  In that case, moving A to the 
> > end of the list would cause B to come before A even though B depends on 
> > A.  Of course, deferred probing already has this problem.
> 
> That may actually happen for PCIe ports and devices below them AFAICS.
> 
> Devices below PCIe ports are discovered by the PCI subsystem and the PCIe
> ports need not be probed before those devices are probed.

Is it possible to change this?  Make it so that devices below PCIe 
ports are discovered by the port driver rather than by the PCI 
subsystem?  Or would that be far too difficult?

> > An easy way to check for this sort of thing would be to verify that a 
> > device about to be probed doesn't have any children.  This wouldn't 
> > catch all the potential dependencies, but it would be a reasonable 
> > start.
> 
> That would address the PCIe ports issue.

Well, it would _detect_ the PCIe ports issue.  It might also detect 
other things.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224561

Hi Alan,

On Sat, Sep 12, 2015 at 7:40 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 12 Sep 2015, Rafael J. Wysocki wrote:
>
>> On Friday, September 11, 2015 03:01:14 PM Alan Stern wrote:
>
>> > There's also an issue about other types of dependencies.  For instance,
>> > it's conceivable that device B might be discovered and depend on device
>> > A, even before A has been bound to a driver.  (B might be discovered by
>> > A's subsystem rather than A's driver.)  In that case, moving A to the
>> > end of the list would cause B to come before A even though B depends on
>> > A.  Of course, deferred probing already has this problem.
>>
>> That may actually happen for PCIe ports and devices below them AFAICS.
>>
>> Devices below PCIe ports are discovered by the PCI subsystem and the PCIe
>> ports need not be probed before those devices are probed.
>
> Is it possible to change this?  Make it so that devices below PCIe
> ports are discovered by the port driver rather than by the PCI
> subsystem?  Or would that be far too difficult?

I don't think it would be really useful.

PCIe ports are PCI bridges from the resource allocation and basic
functionality perspective, so they are handled accordingly.

The PCIe port driver provides additional services (such as PME/hotplug
interrupt handling and advanced error reporting) that aren't necessary
for probing devices below the ports.

I guess the ordering of PCIe ports probing might be changed to happen
at the "right" time, but care needs to be taken in that case too.

>> > An easy way to check for this sort of thing would be to verify that a
>> > device about to be probed doesn't have any children.  This wouldn't
>> > catch all the potential dependencies, but it would be a reasonable
>> > start.
>>
>> That would address the PCIe ports issue.
>
> Well, it would _detect_ the PCIe ports issue.

That's what I meant. :-)

>  It might also detect other things.

Right.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225176

On Tue, 15 Sep 2015, Rafael J. Wysocki wrote:

> Hi Alan,

Hi.

> On Sat, Sep 12, 2015 at 7:40 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, 12 Sep 2015, Rafael J. Wysocki wrote:
> >
> >> On Friday, September 11, 2015 03:01:14 PM Alan Stern wrote:
> >
> >> > There's also an issue about other types of dependencies.  For instance,
> >> > it's conceivable that device B might be discovered and depend on device
> >> > A, even before A has been bound to a driver.  (B might be discovered by
> >> > A's subsystem rather than A's driver.)  In that case, moving A to the
> >> > end of the list would cause B to come before A even though B depends on
> >> > A.  Of course, deferred probing already has this problem.
> >>
> >> That may actually happen for PCIe ports and devices below them AFAICS.
> >>
> >> Devices below PCIe ports are discovered by the PCI subsystem and the PCIe
> >> ports need not be probed before those devices are probed.
> >
> > Is it possible to change this?  Make it so that devices below PCIe
> > ports are discovered by the port driver rather than by the PCI
> > subsystem?  Or would that be far too difficult?
> 
> I don't think it would be really useful.
> 
> PCIe ports are PCI bridges from the resource allocation and basic
> functionality perspective, so they are handled accordingly.
> 
> The PCIe port driver provides additional services (such as PME/hotplug
> interrupt handling and advanced error reporting) that aren't necessary
> for probing devices below the ports.
> 
> I guess the ordering of PCIe ports probing might be changed to happen
> at the "right" time, but care needs to be taken in that case too.

Does suspending a PCIe port do anything significant?  In particular, 
does it cut off normal communication with anything attached through 
the port?

If it does then we better not move the port device to the end of the 
dpm_list after any descendant devices have been probed.

> >> > An easy way to check for this sort of thing would be to verify that a
> >> > device about to be probed doesn't have any children.  This wouldn't
> >> > catch all the potential dependencies, but it would be a reasonable
> >> > start.
> >>
> >> That would address the PCIe ports issue.
> >
> > Well, it would _detect_ the PCIe ports issue.
> 
> That's what I meant. :-)
> 
> >  It might also detect other things.
> 
> Right.

All right, I'll try writing a test patch to report these things.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225619

On Tuesday, September 15, 2015 10:23:07 AM Alan Stern wrote:
> On Tue, 15 Sep 2015, Rafael J. Wysocki wrote:
> 
> > Hi Alan,
> 
> Hi.
> 
> > On Sat, Sep 12, 2015 at 7:40 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Sat, 12 Sep 2015, Rafael J. Wysocki wrote:
> > >
> > >> On Friday, September 11, 2015 03:01:14 PM Alan Stern wrote:
> > >
> > >> > There's also an issue about other types of dependencies.  For instance,
> > >> > it's conceivable that device B might be discovered and depend on device
> > >> > A, even before A has been bound to a driver.  (B might be discovered by
> > >> > A's subsystem rather than A's driver.)  In that case, moving A to the
> > >> > end of the list would cause B to come before A even though B depends on
> > >> > A.  Of course, deferred probing already has this problem.
> > >>
> > >> That may actually happen for PCIe ports and devices below them AFAICS.
> > >>
> > >> Devices below PCIe ports are discovered by the PCI subsystem and the PCIe
> > >> ports need not be probed before those devices are probed.
> > >
> > > Is it possible to change this?  Make it so that devices below PCIe
> > > ports are discovered by the port driver rather than by the PCI
> > > subsystem?  Or would that be far too difficult?
> > 
> > I don't think it would be really useful.
> > 
> > PCIe ports are PCI bridges from the resource allocation and basic
> > functionality perspective, so they are handled accordingly.
> > 
> > The PCIe port driver provides additional services (such as PME/hotplug
> > interrupt handling and advanced error reporting) that aren't necessary
> > for probing devices below the ports.
> > 
> > I guess the ordering of PCIe ports probing might be changed to happen
> > at the "right" time, but care needs to be taken in that case too.
> 
> Does suspending a PCIe port do anything significant?  In particular, 
> does it cut off normal communication with anything attached through 
> the port?

No, it doesn't.

> If it does then we better not move the port device to the end of the 
> dpm_list after any descendant devices have been probed.
> 
> > >> > An easy way to check for this sort of thing would be to verify that a
> > >> > device about to be probed doesn't have any children.  This wouldn't
> > >> > catch all the potential dependencies, but it would be a reasonable
> > >> > start.
> > >>
> > >> That would address the PCIe ports issue.
> > >
> > > Well, it would _detect_ the PCIe ports issue.
> > 
> > That's what I meant. :-)
> > 
> > >  It might also detect other things.
> > 
> > Right.
> 
> All right, I'll try writing a test patch to report these things.

OK, thanks!

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226097

Hi Thierry, Alan, Rafael,

On 09/12/2015 01:28 AM, Rafael J. Wysocki wrote:
> On Friday, September 11, 2015 03:01:14 PM Alan Stern wrote:
>> On Fri, 11 Sep 2015, Thierry Reding wrote:
>>
>>> On Fri, Sep 11, 2015 at 12:08:02AM +0200, Rafael J. Wysocki wrote:
>>>> On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Deferred probe can lead to strange situations where a device that is a
>>>>> dependency for others will be moved to the end of the dpm_list. At the
>>>>> same time the dependers may not be moved because at the time they will
>>>>> be probed the dependee may already have been successfully reprobed and
>>>>> they will not have to defer the probe themselves.
>>>>
>>>> So there's a bug in the implementation of deferred probing IMO.
>>>
>>> Well, yeah. The root problem here is that we don't have dependency
>>> information and deferred probing is supposed to fix that. It does so
>>> fairly well, but it breaks in this particular case.

Actually this is the old problem and deferred probing just makes it more
reproducible.

Lets say, we have device P-producer and device C-consumer.
- devices are created/registered in the following order (It doesn't matter
how they are created - dt, platform,..)
-- device C created
-- device P created

- devices probing order (legacy probing order depends on Makefiles and
initcalls - no deferred probing)
-- device P probed
-- device C probed

- suspend order
-- device P suspended
-- device C suspended (ops, device C may still need device P)

To W/A such issues:
- driver's suspend code can be shifted between suspend layers (from
.suspend() to .suspend_noirq(), for example)
- device's registration order can be changed manually in DT or platform code
- can play with initcalls

May be it was enough before, but now it's not :(, simply because, now
there are much more generic/common frameworks in Kernel:
gpio, regulators, dma ++ clk, syscon, phy, extcon, component, display framework..
As result, It introduces more devices and dependencies between devices and,
therefore, suspend ordering issue can be hit more often.

>>>
>>>>> One example where this happens is the Jetson TK1 board (Tegra124). The
>>>>> gpio-keys driver exposes the power key of the board as an input device
>>>>> that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
>>>>> tegra: Add gpio-ranges property") results in the gpio-tegra driver
>>>>> deferring probe because one of its dependencies, the pinctrl-tegra
>>>>> driver, has not successfully completed probing. Currently the deferred
>>>>> probe code will move the corresponding gpio-tegra device to the end of
>>>>> the dpm_list, but by the time the gpio-keys device, depending on the
>>>>> gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
>>>>> the gpio-keys device is not moved to the end of dpm_list itself. As a
>>>>> result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
>>>>> gpio-tegra. That's problematic because the gpio-keys driver requests
>>>>> the power key to be a wakeup source. However, the programming of the
>>>>> wakeup interrupt registers happens in the gpio-tegra driver's suspend
>>>>> callback, which is now called before that of the gpio-keys driver. The
>>>>> result is that the wrong values are programmed and leaves the system
>>>>> unable to be resumed using the power key.
>>>>>
>>>>> To fix this situation, always move devices to the end of the dpm_list
>>>>> before probing them. Technically this should only be done for devices
>>>>> that have been successfully probed, but that won't work for recursive
>>>>> probing of devices (think an I2C master that instantiates children in
>>>>> its ->probe()). Effectively the dpm_list will end up ordered the same
>>>>> way that devices were probed, hence taking care of dependencies.
>>
>> I'm not worried about the overhead involved in moving a device to the
>> end of the list every time it is probed.  That ought to be relatively
>> small.
>>
>> There are a few things to watch out for.  Since the dpm_list gets
>> modified during system sleep transitions, we would have to make sure
>> that nothing gets probed during those times.  In principle, that's what
>> the "prepare" stage is meant for, but there's still a race.  As long as
>> no other kernel thread (such as the deferred probing mechanism) tries
>> to probe a device once everything has been frozen, we should be okay.
>> But if not, there will be trouble -- after the ->prepare callback runs,
>> the device is no longer on the dpm_list and so we don't want this patch
>> to put it back on that list.
> 

I think, It should prohibited to probe devices during suspend/hibernation.
And solution introduced in this patch might help to fix it -
in general, we could do :
- add sync point on suspend enter: wait_for_device_probe() and
- prohibit probing: move all devices which will request probing into
deferred_probe list
- one suspend exit: allow probing and do driver_deferred_probe_trigger

> 
>> There's also an issue about other types of dependencies.  For instance,
>> it's conceivable that device B might be discovered and depend on device
>> A, even before A has been bound to a driver.  (B might be discovered by
>> A's subsystem rather than A's driver.)  In that case, moving A to the
>> end of the list would cause B to come before A even though B depends on
>> A.  Of course, deferred probing already has this problem.
> 
> That may actually happen for PCIe ports and devices below them AFAICS.
> 
> Devices below PCIe ports are discovered by the PCI subsystem and the PCIe
> ports need not be probed before those devices are probed.

I'd like to mention here that this patch will work only
if dmp_list will be filled according device creation order ("parent<-child" dependencies)
*AND* according device's probing order ("supplier<-consumer").
So, if there is the case when Parent device can be probed AFTER its children
- it will not work, because "parent<-child" dependencies will not be tracked
any more :( Sry, I could not even imagine that such crazy case exist :'(

Are there any other subsystems with the same behavior like PCI?
If not - probably, it could be fixed in PCI subsystem using device_pm_move_after() or 
device_move() in PCIe ports probe.
if yes - ... maybe we can scan/re-check and reorder dpm_list on suspend enter and
restore ("parent<-child" dependencies).

> 
>> An easy way to check for this sort of thing would be to verify that a
>> device about to be probed doesn't have any children.  This wouldn't
>> catch all the potential dependencies, but it would be a reasonable
>> start.
> 
> That would address the PCIe ports issue.
> 

that might work also.

Truth is that smth. need to be done 100%. Personally, I was hit by this issue also,
 and it cost me 3 hours of debugging and I came up with the same patch as
Bill Huang, then spent some time trying to understand what is wrong with PCI
- finally, I've just changed the order of my devices in DT :)

Also, I think, it will be good to have this patch in -next to collect more feedbacks.
 
-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226418

On Wed, 16 Sep 2015, Grygorii Strashko wrote:

> I think, It should prohibited to probe devices during suspend/hibernation.
> And solution introduced in this patch might help to fix it -
> in general, we could do :
> - add sync point on suspend enter: wait_for_device_probe() and
> - prohibit probing: move all devices which will request probing into
> deferred_probe list
> - one suspend exit: allow probing and do driver_deferred_probe_trigger

That could work; it's a good idea.

> I'd like to mention here that this patch will work only
> if dmp_list will be filled according device creation order ("parent<-child" dependencies)
> *AND* according device's probing order ("supplier<-consumer").
> So, if there is the case when Parent device can be probed AFTER its children
> - it will not work, because "parent<-child" dependencies will not be tracked
> any more :( Sry, I could not even imagine that such crazy case exist :'(

If we avoid moving devices to the end of the dpm_list when they already 
have children, then we should be okay, right?

> Are there any other subsystems with the same behavior like PCI?

I don't know.

> If not - probably, it could be fixed in PCI subsystem using device_pm_move_after() or 
> device_move() in PCIe ports probe.
> if yes - ... maybe we can scan/re-check and reorder dpm_list on suspend enter and
> restore ("parent<-child" dependencies).

> Truth is that smth. need to be done 100%. Personally, I was hit by this issue also,
>  and it cost me 3 hours of debugging and I came up with the same patch as
> Bill Huang, then spent some time trying to understand what is wrong with PCI
> - finally, I've just changed the order of my devices in DT :)
> 
> Also, I think, it will be good to have this patch in -next to collect more feedbacks.

I like the idea of forcing all probes during a sleep transition to be 
deferred.  We could carry them out just before unfreezing the user 
threads.  That combined with the change mentioned above ought to be 
worth testing.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226575

On Wednesday, September 16, 2015 03:27:55 PM Alan Stern wrote:
> On Wed, 16 Sep 2015, Grygorii Strashko wrote:
> 
> > I think, It should prohibited to probe devices during suspend/hibernation.
> > And solution introduced in this patch might help to fix it -
> > in general, we could do :
> > - add sync point on suspend enter: wait_for_device_probe() and
> > - prohibit probing: move all devices which will request probing into
> > deferred_probe list
> > - one suspend exit: allow probing and do driver_deferred_probe_trigger
> 
> That could work; it's a good idea.
> 
> > I'd like to mention here that this patch will work only
> > if dmp_list will be filled according device creation order ("parent<-child" dependencies)
> > *AND* according device's probing order ("supplier<-consumer").
> > So, if there is the case when Parent device can be probed AFTER its children
> > - it will not work, because "parent<-child" dependencies will not be tracked
> > any more :( Sry, I could not even imagine that such crazy case exist :'(
> 
> If we avoid moving devices to the end of the dpm_list when they already 
> have children, then we should be okay, right?
> 
> > Are there any other subsystems with the same behavior like PCI?
> 
> I don't know.
> 
> > If not - probably, it could be fixed in PCI subsystem using device_pm_move_after() or 
> > device_move() in PCIe ports probe.
> > if yes - ... maybe we can scan/re-check and reorder dpm_list on suspend enter and
> > restore ("parent<-child" dependencies).
> 
> > Truth is that smth. need to be done 100%. Personally, I was hit by this issue also,
> >  and it cost me 3 hours of debugging and I came up with the same patch as
> > Bill Huang, then spent some time trying to understand what is wrong with PCI
> > - finally, I've just changed the order of my devices in DT :)
> > 
> > Also, I think, it will be good to have this patch in -next to collect more feedbacks.
> 
> I like the idea of forcing all probes during a sleep transition to be 
> deferred.  We could carry them out just before unfreezing the user 
> threads.  That combined with the change mentioned above ought to be 
> worth testing.

Agreed.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227143

Hi,
 
On 09/17/2015 03:07 AM, Rafael J. Wysocki wrote:
> On Wednesday, September 16, 2015 03:27:55 PM Alan Stern wrote:
>> On Wed, 16 Sep 2015, Grygorii Strashko wrote:
>>
>>> I think, It should prohibited to probe devices during suspend/hibernation.
>>> And solution introduced in this patch might help to fix it -
>>> in general, we could do :
>>> - add sync point on suspend enter: wait_for_device_probe() and
>>> - prohibit probing: move all devices which will request probing into
>>> deferred_probe list
>>> - one suspend exit: allow probing and do driver_deferred_probe_trigger
>>
>> That could work; it's a good idea.
>>
>>> I'd like to mention here that this patch will work only
>>> if dmp_list will be filled according device creation order ("parent<-child" dependencies)
>>> *AND* according device's probing order ("supplier<-consumer").
>>> So, if there is the case when Parent device can be probed AFTER its children
>>> - it will not work, because "parent<-child" dependencies will not be tracked
>>> any more :( Sry, I could not even imagine that such crazy case exist :'(
>>
>> If we avoid moving devices to the end of the dpm_list when they already
>> have children, then we should be okay, right?
>>
>>> Are there any other subsystems with the same behavior like PCI?
>>
>> I don't know.
>>
>>> If not - probably, it could be fixed in PCI subsystem using device_pm_move_after() or
>>> device_move() in PCIe ports probe.
>>> if yes - ... maybe we can scan/re-check and reorder dpm_list on suspend enter and
>>> restore ("parent<-child" dependencies).
>>
>>> Truth is that smth. need to be done 100%. Personally, I was hit by this issue also,
>>>   and it cost me 3 hours of debugging and I came up with the same patch as
>>> Bill Huang, then spent some time trying to understand what is wrong with PCI
>>> - finally, I've just changed the order of my devices in DT :)
>>>
>>> Also, I think, it will be good to have this patch in -next to collect more feedbacks.
>>
>> I like the idea of forcing all probes during a sleep transition to be
>> deferred.  We could carry them out just before unfreezing the user
>> threads.  That combined with the change mentioned above ought to be
>> worth testing.
> 
> Agreed.
> 

I've prepared code change which should prohibit devices probing during suspend/hibernation
(below). It also expected to fix wait_for_device_probe() to take into account the case
when the deferred probe workqueue could be still active. 

NOTE: It's only compile time tested!

I'm very sorry that I'm replying here instead of sending a proper patch -
I'm on business trip right now and I will be traveling next week also and will not
be able to work on it intensively.

If proposed approach is correct I can send RFC/RFT patch/es (or anyone else could 
pick up it if interested to move forward faster).

-- 
regards,
-grygorii

From d29e554bf1d593c6c52d2902872ba8a6c48a80a8 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Thu, 17 Sep 2015 18:33:54 +0300
Subject: [RFC/RFT PATCH] PM / sleep: prohibit devices probing during suspend/hibernation

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/base/dd.c      | 28 +++++++++++++++++++++++++++-
 include/linux/device.h |  1 +
 kernel/power/process.c |  8 ++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb46..dcadf30 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -55,6 +55,14 @@ static struct workqueue_struct *deferred_wq;
 static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
 
 /*
+ * In some cases, like suspend to RAM or hibernation, It might be reasonable
+ * to prohibit probing of devices as it could be unsafe.
+ * Once driver_force_probe_deferral is true all drivers probes will
+ * be forcibly deferred
+ */
+static bool driver_force_probe_deferral;
+
+/*
  * deferred_probe_work_func() - Retry probing devices in the active list.
  */
 static void deferred_probe_work_func(struct work_struct *work)
@@ -171,6 +179,14 @@ static void driver_deferred_probe_trigger(void)
 	queue_work(deferred_wq, &deferred_probe_work);
 }
 
+void device_force_probe_deferral(bool enable)
+{
+	driver_force_probe_deferral = enable;
+	if (!enable)
+		driver_deferred_probe_trigger();
+}
+EXPORT_SYMBOL_GPL(device_force_probe_deferral);
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
@@ -277,9 +293,15 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
 
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
-	int ret = 0;
+	int ret = -EPROBE_DEFER;
 	int local_trigger_count = atomic_read(&deferred_trigger_count);
 
+	if (driver_force_probe_deferral) {
+		dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
+		driver_deferred_probe_add(dev);
+		return ret;
+	}
+
 	atomic_inc(&probe_count);
 	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
 		 drv->bus->name, __func__, drv->name, dev_name(dev));
@@ -391,6 +413,10 @@ int driver_probe_done(void)
  */
 void wait_for_device_probe(void)
 {
+	/* wait for the deferred probe workqueue to finish */
+	if (driver_deferred_probe_enable)
+		flush_workqueue(deferred_wq);
+
 	/* wait for the known devices to complete their probing */
 	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
 	async_synchronize_full();
diff --git a/include/linux/device.h b/include/linux/device.h
index 5d7bc63..c68b8e1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1034,6 +1034,7 @@ extern int  __must_check device_attach(struct device *dev);
 extern int __must_check driver_attach(struct device_driver *drv);
 extern void device_initial_probe(struct device *dev);
 extern int __must_check device_reprobe(struct device *dev);
+extern void device_force_probe_deferral(bool enable);
 
 /*
  * Easy functions for dynamically creating devices on the fly
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 564f786..c13e78d 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -148,6 +148,13 @@ int freeze_processes(void)
 	if (!error && !oom_killer_disable())
 		error = -EBUSY;
 
+	if (!error) {
+		/** wait for the known devices to complete their probing */
+		wait_for_device_probe();
+		device_force_probe_deferral(true);
+		wait_for_device_probe();
+	}
+
 	if (error)
 		thaw_processes();
 	return error;
@@ -190,6 +197,7 @@ void thaw_processes(void)
 		atomic_dec(&system_freezing_cnt);
 	pm_freezing = false;
 	pm_nosig_freezing = false;
+	device_force_probe_deferral(false);
 
 	oom_killer_enable();
 
-- 
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227487

Hi,

On Thu, Sep 17, 2015 at 5:48 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> Hi,
>
> On 09/17/2015 03:07 AM, Rafael J. Wysocki wrote:
>> On Wednesday, September 16, 2015 03:27:55 PM Alan Stern wrote:
>>> On Wed, 16 Sep 2015, Grygorii Strashko wrote:
>>>
>>>> I think, It should prohibited to probe devices during suspend/hibernation.
>>>> And solution introduced in this patch might help to fix it -
>>>> in general, we could do :
>>>> - add sync point on suspend enter: wait_for_device_probe() and
>>>> - prohibit probing: move all devices which will request probing into
>>>> deferred_probe list
>>>> - one suspend exit: allow probing and do driver_deferred_probe_trigger
>>>
>>> That could work; it's a good idea.
>>>
>>>> I'd like to mention here that this patch will work only
>>>> if dmp_list will be filled according device creation order ("parent<-child" dependencies)
>>>> *AND* according device's probing order ("supplier<-consumer").
>>>> So, if there is the case when Parent device can be probed AFTER its children
>>>> - it will not work, because "parent<-child" dependencies will not be tracked
>>>> any more :( Sry, I could not even imagine that such crazy case exist :'(
>>>
>>> If we avoid moving devices to the end of the dpm_list when they already
>>> have children, then we should be okay, right?
>>>
>>>> Are there any other subsystems with the same behavior like PCI?
>>>
>>> I don't know.
>>>
>>>> If not - probably, it could be fixed in PCI subsystem using device_pm_move_after() or
>>>> device_move() in PCIe ports probe.
>>>> if yes - ... maybe we can scan/re-check and reorder dpm_list on suspend enter and
>>>> restore ("parent<-child" dependencies).
>>>
>>>> Truth is that smth. need to be done 100%. Personally, I was hit by this issue also,
>>>>   and it cost me 3 hours of debugging and I came up with the same patch as
>>>> Bill Huang, then spent some time trying to understand what is wrong with PCI
>>>> - finally, I've just changed the order of my devices in DT :)
>>>>
>>>> Also, I think, it will be good to have this patch in -next to collect more feedbacks.
>>>
>>> I like the idea of forcing all probes during a sleep transition to be
>>> deferred.  We could carry them out just before unfreezing the user
>>> threads.  That combined with the change mentioned above ought to be
>>> worth testing.
>>
>> Agreed.
>>
>
> I've prepared code change which should prohibit devices probing during suspend/hibernation
> (below). It also expected to fix wait_for_device_probe() to take into account the case
> when the deferred probe workqueue could be still active.
>
> NOTE: It's only compile time tested!
>
> I'm very sorry that I'm replying here instead of sending a proper patch -
> I'm on business trip right now and I will be traveling next week also and will not
> be able to work on it intensively.
>
> If proposed approach is correct I can send RFC/RFT patch/es (or anyone else could
> pick up it if interested to move forward faster).
>
> --
> regards,
> -grygorii
>
> From d29e554bf1d593c6c52d2902872ba8a6c48a80a8 Mon Sep 17 00:00:00 2001
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> Date: Thu, 17 Sep 2015 18:33:54 +0300
> Subject: [RFC/RFT PATCH] PM / sleep: prohibit devices probing during suspend/hibernation
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/base/dd.c      | 28 +++++++++++++++++++++++++++-
>  include/linux/device.h |  1 +
>  kernel/power/process.c |  8 ++++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb46..dcadf30 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -55,6 +55,14 @@ static struct workqueue_struct *deferred_wq;
>  static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
>
>  /*
> + * In some cases, like suspend to RAM or hibernation, It might be reasonable
> + * to prohibit probing of devices as it could be unsafe.
> + * Once driver_force_probe_deferral is true all drivers probes will
> + * be forcibly deferred
> + */
> +static bool driver_force_probe_deferral;

What about defer_all_probes ?

> +
> +/*
>   * deferred_probe_work_func() - Retry probing devices in the active list.
>   */
>  static void deferred_probe_work_func(struct work_struct *work)
> @@ -171,6 +179,14 @@ static void driver_deferred_probe_trigger(void)
>         queue_work(deferred_wq, &deferred_probe_work);
>  }
>
> +void device_force_probe_deferral(bool enable)

device_defer_all_probes ?

> +{
> +       driver_force_probe_deferral = enable;
> +       if (!enable)
> +               driver_deferred_probe_trigger();
> +}
> +EXPORT_SYMBOL_GPL(device_force_probe_deferral);

That doesn't need to be exported, it is only called by statically linked code.

> +
>  /**
>   * deferred_probe_initcall() - Enable probing of deferred devices
>   *
> @@ -277,9 +293,15 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
> -       int ret = 0;
> +       int ret = -EPROBE_DEFER;
>         int local_trigger_count = atomic_read(&deferred_trigger_count);
>
> +       if (driver_force_probe_deferral) {

What if the above is evaluated before the suspend sequence starts ->

> +               dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
> +               driver_deferred_probe_add(dev);
> +               return ret;
> +       }
> +

-> and the code below runs after it has started?

Isn't that racy?

>         atomic_inc(&probe_count);
>         pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
>                  drv->bus->name, __func__, drv->name, dev_name(dev));
> @@ -391,6 +413,10 @@ int driver_probe_done(void)
>   */
>  void wait_for_device_probe(void)
>  {
> +       /* wait for the deferred probe workqueue to finish */
> +       if (driver_deferred_probe_enable)
> +               flush_workqueue(deferred_wq);
> +
>         /* wait for the known devices to complete their probing */
>         wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
>         async_synchronize_full();
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5d7bc63..c68b8e1 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1034,6 +1034,7 @@ extern int  __must_check device_attach(struct device *dev);
>  extern int __must_check driver_attach(struct device_driver *drv);
>  extern void device_initial_probe(struct device *dev);
>  extern int __must_check device_reprobe(struct device *dev);
> +extern void device_force_probe_deferral(bool enable);
>
>  /*
>   * Easy functions for dynamically creating devices on the fly
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 564f786..c13e78d 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -148,6 +148,13 @@ int freeze_processes(void)
>         if (!error && !oom_killer_disable())
>                 error = -EBUSY;
>
> +       if (!error) {
> +               /** wait for the known devices to complete their probing */
> +               wait_for_device_probe();
> +               device_force_probe_deferral(true);
> +               wait_for_device_probe();

Ah, OK.  So the second wait_for_device_probe() avoids the race.

What is the first one for?

In any case, maybe call that from dpm_suspend_start() after
dpm_prepare() has run successfully?  This is the point we need to
start to block probing after all.

> +       }
> +
>         if (error)
>                 thaw_processes();
>         return error;
> @@ -190,6 +197,7 @@ void thaw_processes(void)
>                 atomic_dec(&system_freezing_cnt);
>         pm_freezing = false;
>         pm_nosig_freezing = false;
> +       device_force_probe_deferral(false);

And why don't you call that from dpm_resume_end()?

>
>         oom_killer_enable();
>
> --

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227495

On Fri, Sep 18, 2015 at 1:59 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> Hi,
>
> On Thu, Sep 17, 2015 at 5:48 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:

[cut]

>
> In any case, maybe call that from dpm_suspend_start() after
> dpm_prepare() has run successfully?  This is the point we need to
> start to block probing after all.

Actually, even before dpm_prepare().

>
>> +       }
>> +
>>         if (error)
>>                 thaw_processes();
>>         return error;
>> @@ -190,6 +197,7 @@ void thaw_processes(void)
>>                 atomic_dec(&system_freezing_cnt);
>>         pm_freezing = false;
>>         pm_nosig_freezing = false;
>> +       device_force_probe_deferral(false);
>
> And why don't you call that from dpm_resume_end()?
>
>>
>>         oom_killer_enable();
>>
>> --

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225386

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 11, 2015 at 03:01:14PM -0400, Alan Stern wrote:
> On Fri, 11 Sep 2015, Thierry Reding wrote:
> 
> > On Fri, Sep 11, 2015 at 12:08:02AM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Deferred probe can lead to strange situations where a device that is a
> > > > dependency for others will be moved to the end of the dpm_list. At the
> > > > same time the dependers may not be moved because at the time they will
> > > > be probed the dependee may already have been successfully reprobed and
> > > > they will not have to defer the probe themselves.
> > > 
> > > So there's a bug in the implementation of deferred probing IMO.
> > 
> > Well, yeah. The root problem here is that we don't have dependency
> > information and deferred probing is supposed to fix that. It does so
> > fairly well, but it breaks in this particular case.
> > 
> > > > One example where this happens is the Jetson TK1 board (Tegra124). The
> > > > gpio-keys driver exposes the power key of the board as an input device
> > > > that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> > > > tegra: Add gpio-ranges property") results in the gpio-tegra driver
> > > > deferring probe because one of its dependencies, the pinctrl-tegra
> > > > driver, has not successfully completed probing. Currently the deferred
> > > > probe code will move the corresponding gpio-tegra device to the end of
> > > > the dpm_list, but by the time the gpio-keys device, depending on the
> > > > gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> > > > the gpio-keys device is not moved to the end of dpm_list itself. As a
> > > > result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> > > > gpio-tegra. That's problematic because the gpio-keys driver requests
> > > > the power key to be a wakeup source. However, the programming of the
> > > > wakeup interrupt registers happens in the gpio-tegra driver's suspend
> > > > callback, which is now called before that of the gpio-keys driver. The
> > > > result is that the wrong values are programmed and leaves the system
> > > > unable to be resumed using the power key.
> > > > 
> > > > To fix this situation, always move devices to the end of the dpm_list
> > > > before probing them. Technically this should only be done for devices
> > > > that have been successfully probed, but that won't work for recursive
> > > > probing of devices (think an I2C master that instantiates children in
> > > > its ->probe()). Effectively the dpm_list will end up ordered the same
> > > > way that devices were probed, hence taking care of dependencies.
> 
> I'm not worried about the overhead involved in moving a device to the 
> end of the list every time it is probed.  That ought to be relatively 
> small.
> 
> There are a few things to watch out for.  Since the dpm_list gets
> modified during system sleep transitions, we would have to make sure
> that nothing gets probed during those times.  In principle, that's what
> the "prepare" stage is meant for, but there's still a race.  As long as
> no other kernel thread (such as the deferred probing mechanism) tries
> to probe a device once everything has been frozen, we should be okay.
> But if not, there will be trouble -- after the ->prepare callback runs, 
> the device is no longer on the dpm_list and so we don't want this patch 
> to put it back on that list.

Perhaps moving to the end of the list needs to be a little smarter. That
is it could check whether the device has been prepared for suspension or
not and only move when it hasn't?

Then again, shouldn't the core even prohibit new probes once the suspend
has been triggered? Sounds like asking for a lot of trouble if it didn't
...

> There's also an issue about other types of dependencies.  For instance,
> it's conceivable that device B might be discovered and depend on device
> A, even before A has been bound to a driver.  (B might be discovered by 
> A's subsystem rather than A's driver.)  In that case, moving A to the 
> end of the list would cause B to come before A even though B depends on 
> A.  Of course, deferred probing already has this problem.

But that's exactly the problem that I'm seeing. B isn't discovered by
A's subsystem, but the type of dependency is the same. A in this case
would be the GPIO controller and B the gpio-keys device. B clearly
depends on A, but deferred probe currently moves A to the end of the
list but not A, hence why the problem occurs.

That's also a problem that I think this patch solves. By moving every
device to the end of the list before it is probed we ensure that the
dpm_list is ordered in the same way as the probe order. For this to work
the precondition of course is that drivers know about the dependencies
and will defer probe if necessary.

> An easy way to check for this sort of thing would be to verify that a 
> device about to be probed doesn't have any children.  This wouldn't 
> catch all the potential dependencies, but it would be a reasonable 
> start.  Do we currently check that after a device has been unbound, it 
> doesn't have any remaining children?  We should do that too.

I think that would cover the other half of the cases where deferred
probe isn't implemented because drivers are written with the assumption
that children will be instantiated after their parent has been probed.

But it won't catch all the cases where there is no parent-child
relationship between the depender and the dependee.

Thierry

[toc] | [prev] | [next] | [standalone]

#1225518

On Tue, 15 Sep 2015, Thierry Reding wrote:

> > There are a few things to watch out for.  Since the dpm_list gets
> > modified during system sleep transitions, we would have to make sure
> > that nothing gets probed during those times.  In principle, that's what
> > the "prepare" stage is meant for, but there's still a race.  As long as
> > no other kernel thread (such as the deferred probing mechanism) tries
> > to probe a device once everything has been frozen, we should be okay.
> > But if not, there will be trouble -- after the ->prepare callback runs, 
> > the device is no longer on the dpm_list and so we don't want this patch 
> > to put it back on that list.
> 
> Perhaps moving to the end of the list needs to be a little smarter. That
> is it could check whether the device has been prepared for suspension or
> not and only move when it hasn't?

Maybe.  But doesn't that mean it won't solve your problem completely?

> Then again, shouldn't the core even prohibit new probes once the suspend
> has been triggered? Sounds like asking for a lot of trouble if it didn't
> ...

The core prohibits new devices from being registered.  It does not 
prohibit probes of existing devices, because they currently do not 
affect the dpm_list.

In general, we rely on subsystems not to do any probing once a device 
is suspended.  It's probably reasonable to ask them not to do any 
probing once a device has gone through the "prepare" stage.

> > There's also an issue about other types of dependencies.  For instance,
> > it's conceivable that device B might be discovered and depend on device
> > A, even before A has been bound to a driver.  (B might be discovered by 
> > A's subsystem rather than A's driver.)  In that case, moving A to the 
> > end of the list would cause B to come before A even though B depends on 
> > A.  Of course, deferred probing already has this problem.
> 
> But that's exactly the problem that I'm seeing.

Not quite.

>  B isn't discovered by
> A's subsystem, but the type of dependency is the same. A in this case
> would be the GPIO controller and B the gpio-keys device. B clearly
> depends on A, but deferred probe currently moves A to the end of the
> list but not A, hence why the problem occurs.

The difference is that in my example, B can be probed before A.  In
your case it can't.  Therefore the patch works for your case but not
for mine.

> That's also a problem that I think this patch solves. By moving every
> device to the end of the list before it is probed we ensure that the
> dpm_list is ordered in the same way as the probe order. For this to work
> the precondition of course is that drivers know about the dependencies
> and will defer probe if necessary.

Do I understand correctly?  You're saying a driver must defer a probe
if the device it's probing depends on another device which hasn't
been bound yet.  That does not sound like a reasonable sort of
requirement -- we might know about the dependency but we shouldn't have
to check whether the prerequisite device has been bound.

> > An easy way to check for this sort of thing would be to verify that a 
> > device about to be probed doesn't have any children.  This wouldn't 
> > catch all the potential dependencies, but it would be a reasonable 
> > start.  Do we currently check that after a device has been unbound, it 
> > doesn't have any remaining children?  We should do that too.
> 
> I think that would cover the other half of the cases where deferred
> probe isn't implemented because drivers are written with the assumption
> that children will be instantiated after their parent has been probed.
> 
> But it won't catch all the cases where there is no parent-child
> relationship between the depender and the dependee.

No, it won't catch all cases.  But the cases it does catch are ones 
where your patch is likely to cause trouble, so it would be good to 
know about them.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225650

On Tuesday, September 15, 2015 03:18:19 PM Alan Stern wrote:
> On Tue, 15 Sep 2015, Thierry Reding wrote:
> 
> > > There are a few things to watch out for.  Since the dpm_list gets
> > > modified during system sleep transitions, we would have to make sure
> > > that nothing gets probed during those times.  In principle, that's what
> > > the "prepare" stage is meant for, but there's still a race.  As long as
> > > no other kernel thread (such as the deferred probing mechanism) tries
> > > to probe a device once everything has been frozen, we should be okay.
> > > But if not, there will be trouble -- after the ->prepare callback runs, 
> > > the device is no longer on the dpm_list and so we don't want this patch 
> > > to put it back on that list.
> > 
> > Perhaps moving to the end of the list needs to be a little smarter. That
> > is it could check whether the device has been prepared for suspension or
> > not and only move when it hasn't?
> 
> Maybe.  But doesn't that mean it won't solve your problem completely?
> 
> > Then again, shouldn't the core even prohibit new probes once the suspend
> > has been triggered? Sounds like asking for a lot of trouble if it didn't
> > ...
> 
> The core prohibits new devices from being registered.  It does not 
> prohibit probes of existing devices, because they currently do not 
> affect the dpm_list.

Which may be a mistake, because it does affect callbacks executed during
suspend/resume (after successful probe the device potentially has a different
set of PM callbacks than before).

> In general, we rely on subsystems not to do any probing once a device 
> is suspended.  It's probably reasonable to ask them not to do any 
> probing once a device has gone through the "prepare" stage.

Right.

Question is when it should be allowed to probe again.  I guess at the same
time we allow registrations to to take place again?

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226085

On 09/16/2015 04:28 AM, Rafael J. Wysocki wrote:
> On Tuesday, September 15, 2015 03:18:19 PM Alan Stern wrote:
>> On Tue, 15 Sep 2015, Thierry Reding wrote:
>>
>>>> There are a few things to watch out for.  Since the dpm_list gets
>>>> modified during system sleep transitions, we would have to make sure
>>>> that nothing gets probed during those times.  In principle, that's what
>>>> the "prepare" stage is meant for, but there's still a race.  As long as
>>>> no other kernel thread (such as the deferred probing mechanism) tries
>>>> to probe a device once everything has been frozen, we should be okay.
>>>> But if not, there will be trouble -- after the ->prepare callback runs,
>>>> the device is no longer on the dpm_list and so we don't want this patch
>>>> to put it back on that list.
>>>
>>> Perhaps moving to the end of the list needs to be a little smarter. That
>>> is it could check whether the device has been prepared for suspension or
>>> not and only move when it hasn't?
>>
>> Maybe.  But doesn't that mean it won't solve your problem completely?
>>
>>> Then again, shouldn't the core even prohibit new probes once the suspend
>>> has been triggered? Sounds like asking for a lot of trouble if it didn't
>>> ...
>>
>> The core prohibits new devices from being registered.  It does not
>> prohibit probes of existing devices, because they currently do not
>> affect the dpm_list.

Seems I missed smth, but I can't find the place in Kernel that prohibits
creation of new devices during suspend.

Could someone point me on, please?




-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web