All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] PM: runtime: Fix rpm_idle() and relocate rpm_callback()
@ 2022-12-02 14:28 Rafael J. Wysocki
  2022-12-02 14:30 ` [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle() Rafael J. Wysocki
  2022-12-02 14:32 ` [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback() Rafael J. Wysocki
  0 siblings, 2 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-12-02 14:28 UTC (permalink / raw)
  To: Linux PM
  Cc: LKML, Tushar Nimkar, Adrian Hunter, Rafael J. Wysocki,
	Nitin Rawat, Peter Wang, Alan Stern, Ulf Hansson

Hi All,

The first patch in this series replaces the __rpm_callback() invocation in
rpm_idle() with something simpler (and more correct) and the second one
moves rpm_callback() next to __rpm_callback().

Thanks!




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

* [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-02 14:28 [PATCH v1 0/2] PM: runtime: Fix rpm_idle() and relocate rpm_callback() Rafael J. Wysocki
@ 2022-12-02 14:30 ` Rafael J. Wysocki
  2022-12-05  7:45   ` Adrian Hunter
  2022-12-05 12:07   ` Ulf Hansson
  2022-12-02 14:32 ` [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback() Rafael J. Wysocki
  1 sibling, 2 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-12-02 14:30 UTC (permalink / raw)
  To: Linux PM
  Cc: LKML, Tushar Nimkar, Adrian Hunter, Rafael J. Wysocki,
	Nitin Rawat, Peter Wang, Alan Stern, Ulf Hansson

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Calling __rpm_callback() from rpm_idle() after adding device links
support to the former is a clear mistake.

Not only it causes rpm_idle() to carry out unnecessary actions, but it
is also against the assumption regarding the stability of PM-runtime
status accross __rpm_callback() invocations, because rpm_suspend() and
rpm_resume() may run in parallel with __rpm_callback() when it is called
by rpm_idle() and the device's PM-runtime status can be updated by any
of them.

Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
 
 	dev->power.idle_notification = true;
 
-	retval = __rpm_callback(callback, dev);
+	if (dev->power.irq_safe)
+		spin_unlock(&dev->power.lock);
+	else
+		spin_unlock_irq(&dev->power.lock);
+
+	retval = callback(dev);
+
+	if (dev->power.irq_safe)
+		spin_lock(&dev->power.lock);
+	else
+		spin_lock_irq(&dev->power.lock);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);




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

* [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback()
  2022-12-02 14:28 [PATCH v1 0/2] PM: runtime: Fix rpm_idle() and relocate rpm_callback() Rafael J. Wysocki
  2022-12-02 14:30 ` [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle() Rafael J. Wysocki
@ 2022-12-02 14:32 ` Rafael J. Wysocki
  2022-12-05  8:13   ` Adrian Hunter
  1 sibling, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-12-02 14:32 UTC (permalink / raw)
  To: Linux PM
  Cc: LKML, Tushar Nimkar, Adrian Hunter, Rafael J. Wysocki,
	Nitin Rawat, Peter Wang, Alan Stern, Ulf Hansson

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because rpm_callback() is a wrapper around __rpm_callback(), and the
only caller of it after the change eliminating an invocation of it
from rpm_idle(), move the former next to the latter to make the code
a bit easier to follow.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |   64 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -422,6 +422,38 @@ fail:
 }
 
 /**
+ * rpm_callback - Run a given runtime PM callback for a given device.
+ * @cb: Runtime PM callback to run.
+ * @dev: Device to run the callback for.
+ */
+static int rpm_callback(int (*cb)(struct device *), struct device *dev)
+{
+	int retval;
+
+	if (dev->power.memalloc_noio) {
+		unsigned int noio_flag;
+
+		/*
+		 * Deadlock might be caused if memory allocation with
+		 * GFP_KERNEL happens inside runtime_suspend and
+		 * runtime_resume callbacks of one block device's
+		 * ancestor or the block device itself. Network
+		 * device might be thought as part of iSCSI block
+		 * device, so network device and its ancestor should
+		 * be marked as memalloc_noio too.
+		 */
+		noio_flag = memalloc_noio_save();
+		retval = __rpm_callback(cb, dev);
+		memalloc_noio_restore(noio_flag);
+	} else {
+		retval = __rpm_callback(cb, dev);
+	}
+
+	dev->power.runtime_error = retval;
+	return retval != -EACCES ? retval : -EIO;
+}
+
+/**
  * rpm_idle - Notify device bus type if the device can be suspended.
  * @dev: Device to notify the bus type about.
  * @rpmflags: Flag bits.
@@ -505,38 +537,6 @@ static int rpm_idle(struct device *dev,
 }
 
 /**
- * rpm_callback - Run a given runtime PM callback for a given device.
- * @cb: Runtime PM callback to run.
- * @dev: Device to run the callback for.
- */
-static int rpm_callback(int (*cb)(struct device *), struct device *dev)
-{
-	int retval;
-
-	if (dev->power.memalloc_noio) {
-		unsigned int noio_flag;
-
-		/*
-		 * Deadlock might be caused if memory allocation with
-		 * GFP_KERNEL happens inside runtime_suspend and
-		 * runtime_resume callbacks of one block device's
-		 * ancestor or the block device itself. Network
-		 * device might be thought as part of iSCSI block
-		 * device, so network device and its ancestor should
-		 * be marked as memalloc_noio too.
-		 */
-		noio_flag = memalloc_noio_save();
-		retval = __rpm_callback(cb, dev);
-		memalloc_noio_restore(noio_flag);
-	} else {
-		retval = __rpm_callback(cb, dev);
-	}
-
-	dev->power.runtime_error = retval;
-	return retval != -EACCES ? retval : -EIO;
-}
-
-/**
  * rpm_suspend - Carry out runtime suspend of given device.
  * @dev: Device to suspend.
  * @rpmflags: Flag bits.




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

* Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-02 14:30 ` [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle() Rafael J. Wysocki
@ 2022-12-05  7:45   ` Adrian Hunter
  2022-12-05 14:45     ` Rafael J. Wysocki
  2022-12-05 12:07   ` Ulf Hansson
  1 sibling, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2022-12-05  7:45 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux PM
  Cc: LKML, Tushar Nimkar, Adrian Hunter, Rafael J. Wysocki,
	Nitin Rawat, Peter Wang, Alan Stern, Ulf Hansson

On 2/12/22 16:30, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Calling __rpm_callback() from rpm_idle() after adding device links
> support to the former is a clear mistake.
> 
> Not only it causes rpm_idle() to carry out unnecessary actions, but it
> is also against the assumption regarding the stability of PM-runtime
> status accross __rpm_callback() invocations, because rpm_suspend() and

accross -> across

> rpm_resume() may run in parallel with __rpm_callback() when it is called
> by rpm_idle() and the device's PM-runtime status can be updated by any
> of them.
> 
> Fixes: 21d5c57b3726 ("PM / runtime: Use device links")

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
>  
>  	dev->power.idle_notification = true;
>  
> -	retval = __rpm_callback(callback, dev);
> +	if (dev->power.irq_safe)
> +		spin_unlock(&dev->power.lock);
> +	else
> +		spin_unlock_irq(&dev->power.lock);
> +
> +	retval = callback(dev);
> +
> +	if (dev->power.irq_safe)
> +		spin_lock(&dev->power.lock);
> +	else
> +		spin_lock_irq(&dev->power.lock);
>  
>  	dev->power.idle_notification = false;
>  	wake_up_all(&dev->power.wait_queue);
> 
> 
> 


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

* Re: [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback()
  2022-12-02 14:32 ` [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback() Rafael J. Wysocki
@ 2022-12-05  8:13   ` Adrian Hunter
  0 siblings, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-12-05  8:13 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux PM
  Cc: LKML, Tushar Nimkar, Rafael J. Wysocki, Nitin Rawat, Peter Wang,
	Alan Stern, Ulf Hansson

On 2/12/22 16:32, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because rpm_callback() is a wrapper around __rpm_callback(), and the
> only caller of it after the change eliminating an invocation of it
> from rpm_idle(), move the former next to the latter to make the code
> a bit easier to follow.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/base/power/runtime.c |   64 +++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -422,6 +422,38 @@ fail:
>  }
>  
>  /**
> + * rpm_callback - Run a given runtime PM callback for a given device.
> + * @cb: Runtime PM callback to run.
> + * @dev: Device to run the callback for.
> + */
> +static int rpm_callback(int (*cb)(struct device *), struct device *dev)
> +{
> +	int retval;
> +
> +	if (dev->power.memalloc_noio) {
> +		unsigned int noio_flag;
> +
> +		/*
> +		 * Deadlock might be caused if memory allocation with
> +		 * GFP_KERNEL happens inside runtime_suspend and
> +		 * runtime_resume callbacks of one block device's
> +		 * ancestor or the block device itself. Network
> +		 * device might be thought as part of iSCSI block
> +		 * device, so network device and its ancestor should
> +		 * be marked as memalloc_noio too.
> +		 */
> +		noio_flag = memalloc_noio_save();
> +		retval = __rpm_callback(cb, dev);
> +		memalloc_noio_restore(noio_flag);
> +	} else {
> +		retval = __rpm_callback(cb, dev);
> +	}
> +
> +	dev->power.runtime_error = retval;
> +	return retval != -EACCES ? retval : -EIO;
> +}
> +
> +/**
>   * rpm_idle - Notify device bus type if the device can be suspended.
>   * @dev: Device to notify the bus type about.
>   * @rpmflags: Flag bits.
> @@ -505,38 +537,6 @@ static int rpm_idle(struct device *dev,
>  }
>  
>  /**
> - * rpm_callback - Run a given runtime PM callback for a given device.
> - * @cb: Runtime PM callback to run.
> - * @dev: Device to run the callback for.
> - */
> -static int rpm_callback(int (*cb)(struct device *), struct device *dev)
> -{
> -	int retval;
> -
> -	if (dev->power.memalloc_noio) {
> -		unsigned int noio_flag;
> -
> -		/*
> -		 * Deadlock might be caused if memory allocation with
> -		 * GFP_KERNEL happens inside runtime_suspend and
> -		 * runtime_resume callbacks of one block device's
> -		 * ancestor or the block device itself. Network
> -		 * device might be thought as part of iSCSI block
> -		 * device, so network device and its ancestor should
> -		 * be marked as memalloc_noio too.
> -		 */
> -		noio_flag = memalloc_noio_save();
> -		retval = __rpm_callback(cb, dev);
> -		memalloc_noio_restore(noio_flag);
> -	} else {
> -		retval = __rpm_callback(cb, dev);
> -	}
> -
> -	dev->power.runtime_error = retval;
> -	return retval != -EACCES ? retval : -EIO;
> -}
> -
> -/**
>   * rpm_suspend - Carry out runtime suspend of given device.
>   * @dev: Device to suspend.
>   * @rpmflags: Flag bits.
> 
> 
> 


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

* Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-02 14:30 ` [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle() Rafael J. Wysocki
  2022-12-05  7:45   ` Adrian Hunter
@ 2022-12-05 12:07   ` Ulf Hansson
  2022-12-05 12:13     ` Rafael J. Wysocki
  1 sibling, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2022-12-05 12:07 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PM, LKML, Tushar Nimkar, Adrian Hunter, Rafael J. Wysocki,
	Nitin Rawat, Peter Wang, Alan Stern

On Fri, 2 Dec 2022 at 15:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Calling __rpm_callback() from rpm_idle() after adding device links
> support to the former is a clear mistake.
>
> Not only it causes rpm_idle() to carry out unnecessary actions, but it
> is also against the assumption regarding the stability of PM-runtime
> status accross __rpm_callback() invocations, because rpm_suspend() and
> rpm_resume() may run in parallel with __rpm_callback() when it is called
> by rpm_idle() and the device's PM-runtime status can be updated by any
> of them.

Urgh, that's a nasty bug you are fixing here. Is there perhaps some
links to some error reports that can make sense to include here?

>
> Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
>
>         dev->power.idle_notification = true;
>
> -       retval = __rpm_callback(callback, dev);

Couldn't we just extend __rpm_callback() to take another in-parameter,
rather than open-coding the below?

Note that, __rpm_callback() already uses a "bool use_links" internal
variable, that indicates whether the device links should be used or
not.

> +       if (dev->power.irq_safe)
> +               spin_unlock(&dev->power.lock);
> +       else
> +               spin_unlock_irq(&dev->power.lock);
> +
> +       retval = callback(dev);
> +
> +       if (dev->power.irq_safe)
> +               spin_lock(&dev->power.lock);
> +       else
> +               spin_lock_irq(&dev->power.lock);
>
>         dev->power.idle_notification = false;
>         wake_up_all(&dev->power.wait_queue);
>
>
>

Kind regards
Uffe

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

* Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-05 12:07   ` Ulf Hansson
@ 2022-12-05 12:13     ` Rafael J. Wysocki
  2022-12-05 12:46       ` Ulf Hansson
  0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-12-05 12:13 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rafael J. Wysocki, Linux PM, LKML, Tushar Nimkar, Adrian Hunter,
	Rafael J. Wysocki, Nitin Rawat, Peter Wang, Alan Stern

On Mon, Dec 5, 2022 at 1:08 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 2 Dec 2022 at 15:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Calling __rpm_callback() from rpm_idle() after adding device links
> > support to the former is a clear mistake.
> >
> > Not only it causes rpm_idle() to carry out unnecessary actions, but it
> > is also against the assumption regarding the stability of PM-runtime
> > status accross __rpm_callback() invocations, because rpm_suspend() and
> > rpm_resume() may run in parallel with __rpm_callback() when it is called
> > by rpm_idle() and the device's PM-runtime status can be updated by any
> > of them.
>
> Urgh, that's a nasty bug you are fixing here. Is there perhaps some
> links to some error reports that can make sense to include here?

There is a bug report, but I have no confirmation that this fix is
sufficient to address it (even though I'm quite confident that it will
be).

> >
> > Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/runtime.c |   12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
> >
> >         dev->power.idle_notification = true;
> >
> > -       retval = __rpm_callback(callback, dev);
>
> Couldn't we just extend __rpm_callback() to take another in-parameter,
> rather than open-coding the below?

I'd rather not do that.

I'd prefer rpm_callback() to be used only in rpm_suspend() and
rpm_resume() where all of the assumptions hold and rpm_idle() really
is a special case.

And there is not much open-coding here, just the locking part.

> Note that, __rpm_callback() already uses a "bool use_links" internal
> variable, that indicates whether the device links should be used or
> not.

Yes, it does, but why does that matter?

> > +       if (dev->power.irq_safe)
> > +               spin_unlock(&dev->power.lock);
> > +       else
> > +               spin_unlock_irq(&dev->power.lock);
> > +
> > +       retval = callback(dev);
> > +
> > +       if (dev->power.irq_safe)
> > +               spin_lock(&dev->power.lock);
> > +       else
> > +               spin_lock_irq(&dev->power.lock);
> >
> >         dev->power.idle_notification = false;
> >         wake_up_all(&dev->power.wait_queue);
> >
> >
> >

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

* Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-05 12:13     ` Rafael J. Wysocki
@ 2022-12-05 12:46       ` Ulf Hansson
  2022-12-05 12:51         ` Rafael J. Wysocki
  0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2022-12-05 12:46 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linux PM, LKML, Tushar Nimkar, Adrian Hunter,
	Nitin Rawat, Peter Wang, Alan Stern

On Mon, 5 Dec 2022 at 13:13, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Dec 5, 2022 at 1:08 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 2 Dec 2022 at 15:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Calling __rpm_callback() from rpm_idle() after adding device links
> > > support to the former is a clear mistake.
> > >
> > > Not only it causes rpm_idle() to carry out unnecessary actions, but it
> > > is also against the assumption regarding the stability of PM-runtime
> > > status accross __rpm_callback() invocations, because rpm_suspend() and
> > > rpm_resume() may run in parallel with __rpm_callback() when it is called
> > > by rpm_idle() and the device's PM-runtime status can be updated by any
> > > of them.
> >
> > Urgh, that's a nasty bug you are fixing here. Is there perhaps some
> > links to some error reports that can make sense to include here?
>
> There is a bug report, but I have no confirmation that this fix is
> sufficient to address it (even though I'm quite confident that it will
> be).
>
> > >
> > > Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/power/runtime.c |   12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-pm/drivers/base/power/runtime.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > +++ linux-pm/drivers/base/power/runtime.c
> > > @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
> > >
> > >         dev->power.idle_notification = true;
> > >
> > > -       retval = __rpm_callback(callback, dev);
> >
> > Couldn't we just extend __rpm_callback() to take another in-parameter,
> > rather than open-coding the below?
>
> I'd rather not do that.
>
> I'd prefer rpm_callback() to be used only in rpm_suspend() and
> rpm_resume() where all of the assumptions hold and rpm_idle() really
> is a special case.
>
> And there is not much open-coding here, just the locking part.

That and the actual call to the callback. Not much, but still.

>
> > Note that, __rpm_callback() already uses a "bool use_links" internal
> > variable, that indicates whether the device links should be used or
> > not.
>
> Yes, it does, but why does that matter?

It means that __rpm_callback() is already prepared to (almost) cover this case.

>
> > > +       if (dev->power.irq_safe)
> > > +               spin_unlock(&dev->power.lock);
> > > +       else
> > > +               spin_unlock_irq(&dev->power.lock);
> > > +
> > > +       retval = callback(dev);
> > > +
> > > +       if (dev->power.irq_safe)
> > > +               spin_lock(&dev->power.lock);
> > > +       else
> > > +               spin_lock_irq(&dev->power.lock);
> > >
> > >         dev->power.idle_notification = false;
> > >         wake_up_all(&dev->power.wait_queue);
> > >
> > >
> > >

Note, it's not a big deal to me, if you feel strongly that your
current approach is better, I am fine with that too.

Kind regards
Uffe

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

* Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-05 12:46       ` Ulf Hansson
@ 2022-12-05 12:51         ` Rafael J. Wysocki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-12-05 12:51 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM, LKML,
	Tushar Nimkar, Adrian Hunter, Nitin Rawat, Peter Wang,
	Alan Stern

On Mon, Dec 5, 2022 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 5 Dec 2022 at 13:13, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Dec 5, 2022 at 1:08 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 2 Dec 2022 at 15:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Calling __rpm_callback() from rpm_idle() after adding device links
> > > > support to the former is a clear mistake.
> > > >
> > > > Not only it causes rpm_idle() to carry out unnecessary actions, but it
> > > > is also against the assumption regarding the stability of PM-runtime
> > > > status accross __rpm_callback() invocations, because rpm_suspend() and
> > > > rpm_resume() may run in parallel with __rpm_callback() when it is called
> > > > by rpm_idle() and the device's PM-runtime status can be updated by any
> > > > of them.
> > >
> > > Urgh, that's a nasty bug you are fixing here. Is there perhaps some
> > > links to some error reports that can make sense to include here?
> >
> > There is a bug report, but I have no confirmation that this fix is
> > sufficient to address it (even though I'm quite confident that it will
> > be).
> >
> > > >
> > > > Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/base/power/runtime.c |   12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > Index: linux-pm/drivers/base/power/runtime.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > > +++ linux-pm/drivers/base/power/runtime.c
> > > > @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
> > > >
> > > >         dev->power.idle_notification = true;
> > > >
> > > > -       retval = __rpm_callback(callback, dev);
> > >
> > > Couldn't we just extend __rpm_callback() to take another in-parameter,
> > > rather than open-coding the below?
> >
> > I'd rather not do that.
> >
> > I'd prefer rpm_callback() to be used only in rpm_suspend() and
> > rpm_resume() where all of the assumptions hold and rpm_idle() really
> > is a special case.
> >
> > And there is not much open-coding here, just the locking part.
>
> That and the actual call to the callback. Not much, but still.

Note that it doesn't need to check the callback pointer, though.

Moreover, IMO this code is easier to read without having to look at
__rpm_callback() and reverse engineer all of the different use cases
covered by it.

> > > Note that, __rpm_callback() already uses a "bool use_links" internal
> > > variable, that indicates whether the device links should be used or
> > > not.
> >
> > Yes, it does, but why does that matter?
>
> It means that __rpm_callback() is already prepared to (almost) cover this case.

Well, why does it have to cover all of the cases that are even somewhat related?

> >
> > > > +       if (dev->power.irq_safe)
> > > > +               spin_unlock(&dev->power.lock);
> > > > +       else
> > > > +               spin_unlock_irq(&dev->power.lock);
> > > > +
> > > > +       retval = callback(dev);
> > > > +
> > > > +       if (dev->power.irq_safe)
> > > > +               spin_lock(&dev->power.lock);
> > > > +       else
> > > > +               spin_lock_irq(&dev->power.lock);
> > > >
> > > >         dev->power.idle_notification = false;
> > > >         wake_up_all(&dev->power.wait_queue);
> > > >
> > > >
> > > >
>
> Note, it's not a big deal to me, if you feel strongly that your
> current approach is better, I am fine with that too.

OK, thanks!

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

* Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
  2022-12-05  7:45   ` Adrian Hunter
@ 2022-12-05 14:45     ` Rafael J. Wysocki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-12-05 14:45 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J. Wysocki, Linux PM, LKML, Tushar Nimkar,
	Rafael J. Wysocki, Nitin Rawat, Peter Wang, Alan Stern,
	Ulf Hansson

On Mon, Dec 5, 2022 at 8:45 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 2/12/22 16:30, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Calling __rpm_callback() from rpm_idle() after adding device links
> > support to the former is a clear mistake.
> >
> > Not only it causes rpm_idle() to carry out unnecessary actions, but it
> > is also against the assumption regarding the stability of PM-runtime
> > status accross __rpm_callback() invocations, because rpm_suspend() and
>
> accross -> across

Fixed whey applying the patch.

> > rpm_resume() may run in parallel with __rpm_callback() when it is called
> > by rpm_idle() and the device's PM-runtime status can be updated by any
> > of them.
> >
> > Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
>
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Thank you!

> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/runtime.c |   12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
> >
> >       dev->power.idle_notification = true;
> >
> > -     retval = __rpm_callback(callback, dev);
> > +     if (dev->power.irq_safe)
> > +             spin_unlock(&dev->power.lock);
> > +     else
> > +             spin_unlock_irq(&dev->power.lock);
> > +
> > +     retval = callback(dev);
> > +
> > +     if (dev->power.irq_safe)
> > +             spin_lock(&dev->power.lock);
> > +     else
> > +             spin_lock_irq(&dev->power.lock);
> >
> >       dev->power.idle_notification = false;
> >       wake_up_all(&dev->power.wait_queue);
> >
> >
> >
>

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

end of thread, other threads:[~2022-12-05 14:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02 14:28 [PATCH v1 0/2] PM: runtime: Fix rpm_idle() and relocate rpm_callback() Rafael J. Wysocki
2022-12-02 14:30 ` [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle() Rafael J. Wysocki
2022-12-05  7:45   ` Adrian Hunter
2022-12-05 14:45     ` Rafael J. Wysocki
2022-12-05 12:07   ` Ulf Hansson
2022-12-05 12:13     ` Rafael J. Wysocki
2022-12-05 12:46       ` Ulf Hansson
2022-12-05 12:51         ` Rafael J. Wysocki
2022-12-02 14:32 ` [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback() Rafael J. Wysocki
2022-12-05  8:13   ` Adrian Hunter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.