[v1] PM-runtime: Check supplier_preactivated before release supplier

Commit Message

Peter Wang (王信友) June 13, 2022, 12:07 p.m. UTC

From: Peter Wang <[e-mail]>

With divice link of DL_FLAG_PM_RUNTIME, if consumer call pm_runtime_get_suppliers
to prevent supplier enter suspend, pm_runtime_release_supplier should
check supplier_preactivated before let supplier enter suspend.

If the link is drop or release, bypass check supplier_preactivated.

Signed-off-by: Peter Wang <[e-mail]>
---
 drivers/base/core.c          |  2 +-
 drivers/base/power/runtime.c | 15 ++++++++++++---
 include/linux/pm_runtime.h   |  5 +++--
 3 files changed, 16 insertions(+), 6 deletions(-)

Comments

Greg KH June 22, 2022, 6:48 a.m. UTC | #1

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Wed, Jun 22, 2022 at 02:09:16PM +0800, Peter Wang wrote:
> Hi all,
> 
> 
> gentle ping for this bug fix review.

It's only been 1 week, please give us a chance.  To help out, always
feel free to review other patch submissions.

thanks,

greg k-h

Greg KH June 27, 2022, 2:14 p.m. UTC | #2

On Mon, Jun 13, 2022 at 08:07:55PM +0800, [e-mail] wrote:
> From: Peter Wang <[e-mail]>
> 
> With divice link of DL_FLAG_PM_RUNTIME, if consumer call pm_runtime_get_suppliers
> to prevent supplier enter suspend, pm_runtime_release_supplier should
> check supplier_preactivated before let supplier enter suspend.
> 
> If the link is drop or release, bypass check supplier_preactivated.
> 
> Signed-off-by: Peter Wang <[e-mail]>
> ---
>  drivers/base/core.c          |  2 +-
>  drivers/base/power/runtime.c | 15 ++++++++++++---
>  include/linux/pm_runtime.h   |  5 +++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 7cd789c4985d..3b9cc559928f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -486,7 +486,7 @@ static void device_link_release_fn(struct work_struct *work)
>  	/* Ensure that all references to the link object have been dropped. */
>  	device_link_synchronize_removal();
>  
> -	pm_runtime_release_supplier(link, true);
> +	pm_runtime_release_supplier(link, true, true);
>  
>  	put_device(link->consumer);
>  	put_device(link->supplier);
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 676dc72d912d..3c4f425937a1 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -314,10 +314,19 @@ static int rpm_get_suppliers(struct device *dev)
>   * and if @check_idle is set, check if that device is idle (and so it can be
>   * suspended).
>   */
> -void pm_runtime_release_supplier(struct device_link *link, bool check_idle)
> +void pm_runtime_release_supplier(struct device_link *link, bool check_idle,
> +	bool drop)

This is just making this horrible api even worse.  Now there are 2
boolean flags required, 2 more than really should even be here at all.
Every time you see this function being used, you will now have to look
up the definition  to see what it really does.

Please make a new function that calls the internal function with the
flag set properly, so that it is obvious what is happening when the call
is made.

and really, the same thing should be done for the check_idle flag,
that's not good either.

thanks,

greg k-h

Rafael J. Wysocki June 27, 2022, 2:27 p.m. UTC | #3

On Mon, Jun 27, 2022 at 4:14 PM Greg KH <[e-mail]> wrote:
>
> On Mon, Jun 13, 2022 at 08:07:55PM +0800, [e-mail] wrote:
> > From: Peter Wang <[e-mail]>
> >
> > With divice link of DL_FLAG_PM_RUNTIME, if consumer call pm_runtime_get_suppliers
> > to prevent supplier enter suspend, pm_runtime_release_supplier should
> > check supplier_preactivated before let supplier enter suspend.
> >
> > If the link is drop or release, bypass check supplier_preactivated.
> >
> > Signed-off-by: Peter Wang <[e-mail]>
> > ---
> >  drivers/base/core.c          |  2 +-
> >  drivers/base/power/runtime.c | 15 ++++++++++++---
> >  include/linux/pm_runtime.h   |  5 +++--
> >  3 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 7cd789c4985d..3b9cc559928f 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -486,7 +486,7 @@ static void device_link_release_fn(struct work_struct *work)
> >       /* Ensure that all references to the link object have been dropped. */
> >       device_link_synchronize_removal();
> >
> > -     pm_runtime_release_supplier(link, true);
> > +     pm_runtime_release_supplier(link, true, true);
> >
> >       put_device(link->consumer);
> >       put_device(link->supplier);
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 676dc72d912d..3c4f425937a1 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -314,10 +314,19 @@ static int rpm_get_suppliers(struct device *dev)
> >   * and if @check_idle is set, check if that device is idle (and so it can be
> >   * suspended).
> >   */
> > -void pm_runtime_release_supplier(struct device_link *link, bool check_idle)
> > +void pm_runtime_release_supplier(struct device_link *link, bool check_idle,
> > +     bool drop)
>
> This is just making this horrible api even worse.  Now there are 2
> boolean flags required, 2 more than really should even be here at all.
> Every time you see this function being used, you will now have to look
> up the definition  to see what it really does.
>
> Please make a new function that calls the internal function with the
> flag set properly, so that it is obvious what is happening when the call
> is made.
>
> and really, the same thing should be done for the check_idle flag,
> that's not good either.

Agreed, and let me take care of this.

Rafael J. Wysocki June 27, 2022, 7 p.m. UTC | #4

On Mon, Jun 13, 2022 at 2:08 PM <[e-mail]> wrote:
>
> From: Peter Wang <[e-mail]>
>
> With divice link of DL_FLAG_PM_RUNTIME, if consumer call pm_runtime_get_suppliers
> to prevent supplier enter suspend, pm_runtime_release_supplier should
> check supplier_preactivated before let supplier enter suspend.

Why?

> If the link is drop or release, bypass check supplier_preactivated.
>
> Signed-off-by: Peter Wang <[e-mail]>
> ---
>  drivers/base/core.c          |  2 +-
>  drivers/base/power/runtime.c | 15 ++++++++++++---
>  include/linux/pm_runtime.h   |  5 +++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 7cd789c4985d..3b9cc559928f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -486,7 +486,7 @@ static void device_link_release_fn(struct work_struct *work)
>         /* Ensure that all references to the link object have been dropped. */
>         device_link_synchronize_removal();
>
> -       pm_runtime_release_supplier(link, true);
> +       pm_runtime_release_supplier(link, true, true);
>
>         put_device(link->consumer);
>         put_device(link->supplier);
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 676dc72d912d..3c4f425937a1 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -314,10 +314,19 @@ static int rpm_get_suppliers(struct device *dev)
>   * and if @check_idle is set, check if that device is idle (and so it can be
>   * suspended).
>   */
> -void pm_runtime_release_supplier(struct device_link *link, bool check_idle)
> +void pm_runtime_release_supplier(struct device_link *link, bool check_idle,
> +       bool drop)
>  {
>         struct device *supplier = link->supplier;
>
> +       /*
> +        * When consumer hold supplier, supplier cannot enter suspend.
> +        * Driect release supplier and let supplier enter suspend is not allow.
> +        * Unless the link is drop, direct relsease supplier should be okay.
> +        */
> +       if (link->supplier_preactivated && !drop)
> +               return;
> +
>         /*
>          * The additional power.usage_count check is a safety net in case
>          * the rpm_active refcount becomes saturated, in which case
> @@ -338,7 +347,7 @@ static void __rpm_put_suppliers(struct device *dev, bool try_to_suspend)
>
>         list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
>                                 device_links_read_lock_held())
> -               pm_runtime_release_supplier(link, try_to_suspend);
> +               pm_runtime_release_supplier(link, try_to_suspend, false);
>  }
>
>  static void rpm_put_suppliers(struct device *dev)
> @@ -1838,7 +1847,7 @@ void pm_runtime_drop_link(struct device_link *link)
>                 return;
>
>         pm_runtime_drop_link_count(link->consumer);
> -       pm_runtime_release_supplier(link, true);
> +       pm_runtime_release_supplier(link, true, true);
>  }
>
>  static bool pm_runtime_need_not_resume(struct device *dev)
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 9e4d056967c6..354ffb1eaec0 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -88,7 +88,8 @@ extern void pm_runtime_get_suppliers(struct device *dev);
>  extern void pm_runtime_put_suppliers(struct device *dev);
>  extern void pm_runtime_new_link(struct device *dev);
>  extern void pm_runtime_drop_link(struct device_link *link);
> -extern void pm_runtime_release_supplier(struct device_link *link, bool check_idle);
> +extern void pm_runtime_release_supplier(struct device_link *link,
> +       bool check_idle, bool drop);
>
>  extern int devm_pm_runtime_enable(struct device *dev);
>
> @@ -315,7 +316,7 @@ static inline void pm_runtime_put_suppliers(struct device *dev) {}
>  static inline void pm_runtime_new_link(struct device *dev) {}
>  static inline void pm_runtime_drop_link(struct device_link *link) {}
>  static inline void pm_runtime_release_supplier(struct device_link *link,
> -                                              bool check_idle) {}
> +                                              bool check_idle, bool drop) {}
>
>  #endif /* !CONFIG_PM */
>
> --
> 2.18.0
>

Rafael J. Wysocki June 28, 2022, 3:54 p.m. UTC | #5

On Tue, Jun 28, 2022 at 3:53 AM Peter Wang <[e-mail]> wrote:
>
>
> On 6/28/22 3:00 AM, Rafael J. Wysocki wrote:
> > On Mon, Jun 13, 2022 at 2:08 PM <[e-mail]> wrote:
> >> From: Peter Wang <[e-mail]>
> >>
> >> With divice link of DL_FLAG_PM_RUNTIME, if consumer call pm_runtime_get_suppliers
> >> to prevent supplier enter suspend, pm_runtime_release_supplier should
> >> check supplier_preactivated before let supplier enter suspend.
> > Why?
>
> because supplier_preactivated is true means supplier cannot enter
> suspend, right?

No, it doesn't mean that.

Patch

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7cd789c4985d..3b9cc559928f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -486,7 +486,7 @@  static void device_link_release_fn(struct work_struct *work)
 	/* Ensure that all references to the link object have been dropped. */
 	device_link_synchronize_removal();
 
-	pm_runtime_release_supplier(link, true);
+	pm_runtime_release_supplier(link, true, true);
 
 	put_device(link->consumer);
 	put_device(link->supplier);
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 676dc72d912d..3c4f425937a1 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -314,10 +314,19 @@  static int rpm_get_suppliers(struct device *dev)
  * and if @check_idle is set, check if that device is idle (and so it can be
  * suspended).
  */
-void pm_runtime_release_supplier(struct device_link *link, bool check_idle)
+void pm_runtime_release_supplier(struct device_link *link, bool check_idle,
+	bool drop)
 {
 	struct device *supplier = link->supplier;
 
+	/*
+	 * When consumer hold supplier, supplier cannot enter suspend.
+	 * Driect release supplier and let supplier enter suspend is not allow.
+	 * Unless the link is drop, direct relsease supplier should be okay.
+	 */
+	if (link->supplier_preactivated && !drop)
+		return;
+
 	/*
 	 * The additional power.usage_count check is a safety net in case
 	 * the rpm_active refcount becomes saturated, in which case
@@ -338,7 +347,7 @@  static void __rpm_put_suppliers(struct device *dev, bool try_to_suspend)
 
 	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
 				device_links_read_lock_held())
-		pm_runtime_release_supplier(link, try_to_suspend);
+		pm_runtime_release_supplier(link, try_to_suspend, false);
 }
 
 static void rpm_put_suppliers(struct device *dev)
@@ -1838,7 +1847,7 @@  void pm_runtime_drop_link(struct device_link *link)
 		return;
 
 	pm_runtime_drop_link_count(link->consumer);
-	pm_runtime_release_supplier(link, true);
+	pm_runtime_release_supplier(link, true, true);
 }
 
 static bool pm_runtime_need_not_resume(struct device *dev)
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 9e4d056967c6..354ffb1eaec0 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -88,7 +88,8 @@  extern void pm_runtime_get_suppliers(struct device *dev);
 extern void pm_runtime_put_suppliers(struct device *dev);
 extern void pm_runtime_new_link(struct device *dev);
 extern void pm_runtime_drop_link(struct device_link *link);
-extern void pm_runtime_release_supplier(struct device_link *link, bool check_idle);
+extern void pm_runtime_release_supplier(struct device_link *link,
+	bool check_idle, bool drop);
 
 extern int devm_pm_runtime_enable(struct device *dev);
 
@@ -315,7 +316,7 @@  static inline void pm_runtime_put_suppliers(struct device *dev) {}
 static inline void pm_runtime_new_link(struct device *dev) {}
 static inline void pm_runtime_drop_link(struct device_link *link) {}
 static inline void pm_runtime_release_supplier(struct device_link *link,
-					       bool check_idle) {}
+					       bool check_idle, bool drop) {}
 
 #endif /* !CONFIG_PM */