All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
	Kevin Hilman <khilman@linaro.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Russell King <linux@arm.linux.org.uk>,
	Mark Brown <broonie@kernel.org>, Wolfram Sang <wsa@the-dreams.de>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2] driver core / PM: Add PM domain callbacks for device setup/cleanup
Date: Fri, 20 Mar 2015 13:31:46 +0100	[thread overview]
Message-ID: <178365735.3ZOqcVG98c@vostro.rjw.lan> (raw)
In-Reply-To: <CAPDyKFpvBvWOM0V0jAbz4tTQEvtzp8A6naV316p7S1sAYF43uw@mail.gmail.com>

On Friday, March 20, 2015 12:37:47 PM Ulf Hansson wrote:
> On 20 March 2015 at 08:45, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 19 March 2015 at 22:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup
> >>
> >> If PM domains are in use, it may be necessary to prepare the code
> >> handling a PM domain for driver probing.  For example, in some
> >> cases device drivers rely on the ability to power on the devices
> >> with the help of the IO runtime PM framework and the PM domain
> >> code needs to be ready for that.  Also, if that code has not been
> >> fully initialized yet, the driver probing should be deferred.
> >>
> >> Moreover, after the probing is complete, it may be necessary to
> >> put the PM domain in question into the state reflecting the current
> >> needs of the devices in it, for example, so that power is not drawn
> >> in vain.  The same should be done after removing a driver from
> >> a device, as the PM domain state may need to be changed to reflect
> >> the new situation.
> >>
> >> For these reasons, introduce new PM domain callbacks, ->activate
> >> and ->sync, called, respectively, before probing for a device
> >> driver and after the probing has been completed or the driver
> >> has been removed.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > This looks good to me!
> >
> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> I was a bit too quick acking this a minor fix is needed. See below.
> 
> >
> >> ---
> >>> ---
> >>>
> >>> This is an update without the additional bus type callbacks.  This should
> >>> be suffucient at this point for the use cases we have.
> >>>
> >>> I've decided to also call ->sync on driver removal as that is consistent
> >>> with what happens for failing probe.
> >>>
> >>> ---
> >>
> >> One more update.
> >>
> >> I've realized that in the PM domain's ->sync callback is called after
> >> clearing the device's driver field (in the failed probe or driver removal
> >> case) rather than before it, that could be used to distinguish between the
> >> two cases (successful probe vs no driver), so I reworked the patch accordingly.
> >>
> >> ---
> >>  drivers/base/dd.c  |   16 ++++++++++++++++
> >>  include/linux/pm.h |    6 ++++++
> >>  2 files changed, 22 insertions(+)
> >>
> >> Index: linux-pm/include/linux/pm.h
> >> ===================================================================
> >> --- linux-pm.orig/include/linux/pm.h
> >> +++ linux-pm/include/linux/pm.h
> >> @@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
> >>   * Power domains provide callbacks that are executed during system suspend,
> >>   * hibernation, system resume and during runtime PM transitions along with
> >>   * subsystem-level and driver-level callbacks.
> >> + *
> >> + * @detach: Called when removing a device from the domain.
> >> + * @activate: Called before executing probe routines for bus types and drivers.
> >> + * @sync: Called after driver probe and removal.
> >>   */
> >>  struct dev_pm_domain {
> >>         struct dev_pm_ops       ops;
> >>         void (*detach)(struct device *dev, bool power_off);
> >> +       int (*activate)(struct device *dev);
> >> +       void (*sync)(struct device *dev);
> >>  };
> >>
> >>  /*
> >> Index: linux-pm/drivers/base/dd.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/base/dd.c
> >> +++ linux-pm/drivers/base/dd.c
> >> @@ -279,6 +279,7 @@ static int really_probe(struct device *d
> >>  {
> >>         int ret = 0;
> >>         int local_trigger_count = atomic_read(&deferred_trigger_count);
> >> +       struct dev_pm_domain *pm_domain = dev->pm_domain;
> >>
> >>         atomic_inc(&probe_count);
> >>         pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
> >> @@ -298,6 +299,12 @@ static int really_probe(struct device *d
> >>                 goto probe_failed;
> >>         }
> >>
> 
> 
> 
> >> +       if (pm_domain && pm_domain->activate) {
> 
> You need to re-fetch the pm_domain pointer at this point, since it
> will be updated during ->probe().

Right.

> Maybe it's just safer to always do the following check:
> 
> if (dev->pm_domain && dev->pm_domain->activate|sync)

Yes, it is.

Well, overoptimization.  I seem to always forget to avoid them. :-)

Will send a v3 with that fixed and the Dmitry's comment taken into account
shortly.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

WARNING: multiple messages have this Message-ID (diff)
From: rjw@rjwysocki.net (Rafael J. Wysocki)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] driver core / PM: Add PM domain callbacks for device setup/cleanup
Date: Fri, 20 Mar 2015 13:31:46 +0100	[thread overview]
Message-ID: <178365735.3ZOqcVG98c@vostro.rjw.lan> (raw)
In-Reply-To: <CAPDyKFpvBvWOM0V0jAbz4tTQEvtzp8A6naV316p7S1sAYF43uw@mail.gmail.com>

On Friday, March 20, 2015 12:37:47 PM Ulf Hansson wrote:
> On 20 March 2015 at 08:45, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 19 March 2015 at 22:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup
> >>
> >> If PM domains are in use, it may be necessary to prepare the code
> >> handling a PM domain for driver probing.  For example, in some
> >> cases device drivers rely on the ability to power on the devices
> >> with the help of the IO runtime PM framework and the PM domain
> >> code needs to be ready for that.  Also, if that code has not been
> >> fully initialized yet, the driver probing should be deferred.
> >>
> >> Moreover, after the probing is complete, it may be necessary to
> >> put the PM domain in question into the state reflecting the current
> >> needs of the devices in it, for example, so that power is not drawn
> >> in vain.  The same should be done after removing a driver from
> >> a device, as the PM domain state may need to be changed to reflect
> >> the new situation.
> >>
> >> For these reasons, introduce new PM domain callbacks, ->activate
> >> and ->sync, called, respectively, before probing for a device
> >> driver and after the probing has been completed or the driver
> >> has been removed.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > This looks good to me!
> >
> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> I was a bit too quick acking this a minor fix is needed. See below.
> 
> >
> >> ---
> >>> ---
> >>>
> >>> This is an update without the additional bus type callbacks.  This should
> >>> be suffucient at this point for the use cases we have.
> >>>
> >>> I've decided to also call ->sync on driver removal as that is consistent
> >>> with what happens for failing probe.
> >>>
> >>> ---
> >>
> >> One more update.
> >>
> >> I've realized that in the PM domain's ->sync callback is called after
> >> clearing the device's driver field (in the failed probe or driver removal
> >> case) rather than before it, that could be used to distinguish between the
> >> two cases (successful probe vs no driver), so I reworked the patch accordingly.
> >>
> >> ---
> >>  drivers/base/dd.c  |   16 ++++++++++++++++
> >>  include/linux/pm.h |    6 ++++++
> >>  2 files changed, 22 insertions(+)
> >>
> >> Index: linux-pm/include/linux/pm.h
> >> ===================================================================
> >> --- linux-pm.orig/include/linux/pm.h
> >> +++ linux-pm/include/linux/pm.h
> >> @@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
> >>   * Power domains provide callbacks that are executed during system suspend,
> >>   * hibernation, system resume and during runtime PM transitions along with
> >>   * subsystem-level and driver-level callbacks.
> >> + *
> >> + * @detach: Called when removing a device from the domain.
> >> + * @activate: Called before executing probe routines for bus types and drivers.
> >> + * @sync: Called after driver probe and removal.
> >>   */
> >>  struct dev_pm_domain {
> >>         struct dev_pm_ops       ops;
> >>         void (*detach)(struct device *dev, bool power_off);
> >> +       int (*activate)(struct device *dev);
> >> +       void (*sync)(struct device *dev);
> >>  };
> >>
> >>  /*
> >> Index: linux-pm/drivers/base/dd.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/base/dd.c
> >> +++ linux-pm/drivers/base/dd.c
> >> @@ -279,6 +279,7 @@ static int really_probe(struct device *d
> >>  {
> >>         int ret = 0;
> >>         int local_trigger_count = atomic_read(&deferred_trigger_count);
> >> +       struct dev_pm_domain *pm_domain = dev->pm_domain;
> >>
> >>         atomic_inc(&probe_count);
> >>         pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
> >> @@ -298,6 +299,12 @@ static int really_probe(struct device *d
> >>                 goto probe_failed;
> >>         }
> >>
> 
> 
> 
> >> +       if (pm_domain && pm_domain->activate) {
> 
> You need to re-fetch the pm_domain pointer at this point, since it
> will be updated during ->probe().

Right.

> Maybe it's just safer to always do the following check:
> 
> if (dev->pm_domain && dev->pm_domain->activate|sync)

Yes, it is.

Well, overoptimization.  I seem to always forget to avoid them. :-)

Will send a v3 with that fixed and the Dmitry's comment taken into account
shortly.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

  reply	other threads:[~2015-03-20 12:07 UTC|newest]

Thread overview: 116+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-13 15:43 [PATCH 0/9] PM / Domains: Don't leave unused PM domains powered after ->probe() Ulf Hansson
2015-03-13 15:43 ` Ulf Hansson
2015-03-13 15:43 ` [PATCH 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-14  1:31   ` Rafael J. Wysocki
2015-03-14  1:31     ` Rafael J. Wysocki
2015-03-16  9:26     ` Ulf Hansson
2015-03-16  9:26       ` Ulf Hansson
2015-03-17  3:01       ` Rafael J. Wysocki
2015-03-17  3:01         ` Rafael J. Wysocki
2015-03-17  9:27         ` Ulf Hansson
2015-03-17  9:27           ` Ulf Hansson
2015-03-17 14:45           ` Rafael J. Wysocki
2015-03-17 14:45             ` Rafael J. Wysocki
2015-03-17 14:25             ` Russell King - ARM Linux
2015-03-17 14:25               ` Russell King - ARM Linux
2015-03-18  1:16               ` Rafael J. Wysocki
2015-03-18  1:16                 ` Rafael J. Wysocki
2015-03-17 14:40             ` Ulf Hansson
2015-03-17 14:40               ` Ulf Hansson
2015-03-18  1:09               ` Rafael J. Wysocki
2015-03-18  1:09                 ` Rafael J. Wysocki
2015-03-18 13:41                 ` Ulf Hansson
2015-03-18 13:41                   ` Ulf Hansson
2015-03-18 15:02                   ` [PATCH] driver core / PM: Add callbacks for PM domain initialization/cleanup Rafael J. Wysocki
2015-03-18 15:02                     ` Rafael J. Wysocki
2015-03-19  8:49                     ` Ulf Hansson
2015-03-19  8:49                       ` Ulf Hansson
2015-03-19 11:45                       ` Rafael J. Wysocki
2015-03-19 11:45                         ` Rafael J. Wysocki
2015-03-19 13:16                         ` Ulf Hansson
2015-03-19 13:16                           ` Ulf Hansson
2015-03-19 13:29                     ` Greg Kroah-Hartman
2015-03-19 13:29                       ` Greg Kroah-Hartman
2015-03-19 14:21                       ` Rafael J. Wysocki
2015-03-19 14:21                         ` Rafael J. Wysocki
2015-03-19 14:12                         ` Greg Kroah-Hartman
2015-03-19 14:12                           ` Greg Kroah-Hartman
2015-03-19 15:24                           ` Rafael J. Wysocki
2015-03-19 15:24                             ` Rafael J. Wysocki
2015-03-19 14:20                         ` Alan Stern
2015-03-19 14:20                           ` Alan Stern
2015-03-19 14:45                           ` Ulf Hansson
2015-03-19 14:45                             ` Ulf Hansson
2015-03-19 15:44                             ` Rafael J. Wysocki
2015-03-19 15:44                               ` Rafael J. Wysocki
2015-03-19 15:37                               ` Ulf Hansson
2015-03-19 15:37                                 ` Ulf Hansson
2015-03-19 16:04                                 ` Rafael J. Wysocki
2015-03-19 16:04                                   ` Rafael J. Wysocki
2015-03-19 15:48                                   ` Ulf Hansson
2015-03-19 15:48                                     ` Ulf Hansson
2015-03-19 16:18                                     ` Rafael J. Wysocki
2015-03-19 16:18                                       ` Rafael J. Wysocki
2015-03-19 16:58                                       ` [PATCH] driver core / PM: Add PM domain callbacks for device setup/cleanup Rafael J. Wysocki
2015-03-19 16:58                                         ` Rafael J. Wysocki
2015-03-19 21:51                                         ` [PATCH v2] " Rafael J. Wysocki
2015-03-19 21:51                                           ` Rafael J. Wysocki
2015-03-19 22:42                                           ` Dmitry Torokhov
2015-03-19 22:42                                             ` Dmitry Torokhov
2015-03-20  0:43                                             ` Rafael J. Wysocki
2015-03-20  0:43                                               ` Rafael J. Wysocki
2015-03-20  0:43                                               ` Dmitry Torokhov
2015-03-20  0:43                                                 ` Dmitry Torokhov
2015-03-20  7:45                                           ` Ulf Hansson
2015-03-20  7:45                                             ` Ulf Hansson
2015-03-20 11:37                                             ` Ulf Hansson
2015-03-20 11:37                                               ` Ulf Hansson
2015-03-20 12:31                                               ` Rafael J. Wysocki [this message]
2015-03-20 12:31                                                 ` Rafael J. Wysocki
2015-03-20 12:57                                           ` [PATCH v3] " Rafael J. Wysocki
2015-03-20 12:57                                             ` Rafael J. Wysocki
2015-03-20 12:59                                             ` Rafael J. Wysocki
2015-03-20 12:59                                               ` Rafael J. Wysocki
2015-03-20 13:44                                               ` Ulf Hansson
2015-03-20 13:44                                                 ` Ulf Hansson
2015-03-21  0:09                                               ` Kevin Hilman
2015-03-21  0:09                                                 ` Kevin Hilman
2015-03-21  1:00                                               ` Rafael J. Wysocki
2015-03-21  1:00                                                 ` Rafael J. Wysocki
2015-03-22 11:46                                                 ` Greg Kroah-Hartman
2015-03-22 11:46                                                   ` Greg Kroah-Hartman
2015-03-19 14:46                           ` [PATCH] driver core / PM: Add callbacks for PM domain initialization/cleanup Geert Uytterhoeven
2015-03-19 14:46                             ` Geert Uytterhoeven
2015-03-18 15:09                   ` [PATCH 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs Rafael J. Wysocki
2015-03-18 15:09                     ` Rafael J. Wysocki
2015-03-13 15:43 ` [PATCH 2/9] PM / Domains: Enable genpd to support ->get|put() callbacks Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-16  2:11   ` Chao
2015-03-16  2:11     ` [PATCH " Chao
2015-03-13 15:43 ` [PATCH 3/9] amba: Keep PM domain powered during ->probe() Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-13 16:03   ` Russell King - ARM Linux
2015-03-13 16:03     ` Russell King - ARM Linux
2015-03-16  8:37     ` Ulf Hansson
2015-03-16  8:37       ` Ulf Hansson
2015-03-13 15:43 ` [PATCH 4/9] drivercore / platform: " Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-13 15:43 ` [PATCH 5/9] i2c: core: " Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-13 15:43 ` [PATCH 6/9] spi: " Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-13 15:43 ` [PATCH 7/9] mmc: core: Attach PM domain prior probing of SDIO func driver Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-17  5:04   ` Aaron Lu
2015-03-17  5:04     ` Aaron Lu
2015-03-13 15:43 ` [PATCH 8/9] mmmc: core: Keep PM domain powered during ->probe() " Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-13 16:10   ` Russell King - ARM Linux
2015-03-13 16:10     ` Russell King - ARM Linux
2015-03-16  8:24     ` Ulf Hansson
2015-03-16  8:24       ` Ulf Hansson
2015-03-13 15:43 ` [PATCH 9/9] Revert "PM / Domains: Power on the PM domain right after attach completes" Ulf Hansson
2015-03-13 15:43   ` Ulf Hansson
2015-03-16  9:07   ` Geert Uytterhoeven
2015-03-16  9:07     ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=178365735.3ZOqcVG98c@vostro.rjw.lan \
    --to=rjw@rjwysocki.net \
    --cc=broonie@kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=geert+renesas@glider.be \
    --cc=gregkh@linuxfoundation.org \
    --cc=khilman@linaro.org \
    --cc=len.brown@intel.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=pavel@ucw.cz \
    --cc=stern@rowland.harvard.edu \
    --cc=ulf.hansson@linaro.org \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.