| Message ID | 7hsihx1bvo.fsf@deeprootsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday, November 05, 2014 03:11:23 PM Kevin Hilman wrote: > "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: > > > On Wednesday, November 05, 2014 02:43:31 PM Kevin Hilman wrote: > >> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: > >> > >> > On Thu, Oct 30, 2014 at 01:38:30PM -0700, Kevin Hilman wrote: > >> >> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: > >> >> > >> >> > On Thursday, October 30, 2014 01:02:49 PM Ulf Hansson wrote: > >> >> >> Convert the prototype to return and int. This is just an initial step, > >> >> >> needed to support error handling. > >> >> >> > >> >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >> >> > >> >> Acked-by: Kevin Hilman <khilman@linaro.org> > >> >> > >> >> >> > >> >> >> This patch is intended as fix for 3.18 rc[n]. Why? > >> >> >> > >> >> >> There are other SOC specific patches around that adds genpd support and which > >> >> >> implements the ->attach_dev() callback. To prevent having an "atomic" patch > >> >> >> during the next release cycle, let's change the prototype now instead. > >> >> >> > >> >> >> Further patches will add the actual error handling in genpd and these can then > >> >> >> be reviewed and tested thoroughly. > >> >> > > >> >> > So we have no users of ->attach_dev at the moment, right? > >> >> > >> >> Not in mainline, but there are a couple getting ready to hit -next, so > >> >> we wanted to fix this before they arrive so that adding the error > >> >> handling will be easier. > >> > > >> > BTW, while we are at it, can we also pass the domain itself to > >> > attach_dev() and detach_dev()? If anything it helps with debugging (you > >> > can print domain name from the callbacks). > >> > >> Agreed, and it makes it match the other callbacks (power_off, power_on) > >> which currently take struct generic_pm_domain *domain. > >> > >> Updated version of $SUBJECT patch below. > > > > The subject and changelog need to be updated too IMO. > > > > Right. Here you go. I've replaced the Ulf's original with this one, thanks! > ----- >8 ------ > From c18a6bf3121979c4102ba4f7edb3ac41e3921fc6 Mon Sep 17 00:00:00 2001 > From: Ulf Hansson <ulf.hansson@linaro.org> > Date: Thu, 30 Oct 2014 13:02:49 +0100 > Subject: [PATCH] PM / Domains: Change prototype for the attach and detach > callbacks > > Convert the prototypes to return an int in order to support error > handling in these callbacks. > > Also, as suggested by Dmitry Torokhov, pass the domain pointer for use > inside the callbacks, and so that they match the existing > power_on/power_off callbacks which currently take the domain pointer. > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > [khilman: added domain as parameter to callbacks, as suggested by Dmitry] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Kevin Hilman <khilman@linaro.org> > --- > drivers/base/power/domain.c | 4 ++-- > include/linux/pm_domain.h | 6 ++++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 40bc2f4072cc..b520687046d4 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1437,7 +1437,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > spin_unlock_irq(&dev->power.lock); > > if (genpd->attach_dev) > - genpd->attach_dev(dev); > + genpd->attach_dev(genpd, dev); > > mutex_lock(&gpd_data->lock); > gpd_data->base.dev = dev; > @@ -1499,7 +1499,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd, > genpd->max_off_time_changed = true; > > if (genpd->detach_dev) > - genpd->detach_dev(dev); > + genpd->detach_dev(genpd, dev); > > spin_lock_irq(&dev->power.lock); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 73e938b7e937..b3ed7766a291 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -72,8 +72,10 @@ struct generic_pm_domain { > bool max_off_time_changed; > bool cached_power_down_ok; > struct gpd_cpuidle_data *cpuidle_data; > - void (*attach_dev)(struct device *dev); > - void (*detach_dev)(struct device *dev); > + int (*attach_dev)(struct generic_pm_domain *domain, > + struct device *dev); > + void (*detach_dev)(struct generic_pm_domain *domain, > + struct device *dev); > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) >
On Thu, Nov 6, 2014 at 12:11 AM, Kevin Hilman <khilman@kernel.org> wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > Date: Thu, 30 Oct 2014 13:02:49 +0100 > Subject: [PATCH] PM / Domains: Change prototype for the attach and detach > callbacks > > Convert the prototypes to return an int in order to support error > handling in these callbacks. > > Also, as suggested by Dmitry Torokhov, pass the domain pointer for use > inside the callbacks, and so that they match the existing > power_on/power_off callbacks which currently take the domain pointer. > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > [khilman: added domain as parameter to callbacks, as suggested by Dmitry] OK, so back to the signature of the original version... > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Kevin Hilman <khilman@linaro.org> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 6 November 2014 00:11, Kevin Hilman <khilman@kernel.org> wrote: > "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: > >> On Wednesday, November 05, 2014 02:43:31 PM Kevin Hilman wrote: >>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: >>> >>> > On Thu, Oct 30, 2014 at 01:38:30PM -0700, Kevin Hilman wrote: >>> >> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: >>> >> >>> >> > On Thursday, October 30, 2014 01:02:49 PM Ulf Hansson wrote: >>> >> >> Convert the prototype to return and int. This is just an initial step, >>> >> >> needed to support error handling. >>> >> >> >>> >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> >> >>> >> Acked-by: Kevin Hilman <khilman@linaro.org> >>> >> >>> >> >> >>> >> >> This patch is intended as fix for 3.18 rc[n]. Why? >>> >> >> >>> >> >> There are other SOC specific patches around that adds genpd support and which >>> >> >> implements the ->attach_dev() callback. To prevent having an "atomic" patch >>> >> >> during the next release cycle, let's change the prototype now instead. >>> >> >> >>> >> >> Further patches will add the actual error handling in genpd and these can then >>> >> >> be reviewed and tested thoroughly. >>> >> > >>> >> > So we have no users of ->attach_dev at the moment, right? >>> >> >>> >> Not in mainline, but there are a couple getting ready to hit -next, so >>> >> we wanted to fix this before they arrive so that adding the error >>> >> handling will be easier. >>> > >>> > BTW, while we are at it, can we also pass the domain itself to >>> > attach_dev() and detach_dev()? If anything it helps with debugging (you >>> > can print domain name from the callbacks). >>> >>> Agreed, and it makes it match the other callbacks (power_off, power_on) >>> which currently take struct generic_pm_domain *domain. >>> >>> Updated version of $SUBJECT patch below. >> >> The subject and changelog need to be updated too IMO. >> > > Right. Here you go. > > Kevin Thanks for helping out Kevin! Kind regards Uffe > > > ----- >8 ------ > From c18a6bf3121979c4102ba4f7edb3ac41e3921fc6 Mon Sep 17 00:00:00 2001 > From: Ulf Hansson <ulf.hansson@linaro.org> > Date: Thu, 30 Oct 2014 13:02:49 +0100 > Subject: [PATCH] PM / Domains: Change prototype for the attach and detach > callbacks > > Convert the prototypes to return an int in order to support error > handling in these callbacks. > > Also, as suggested by Dmitry Torokhov, pass the domain pointer for use > inside the callbacks, and so that they match the existing > power_on/power_off callbacks which currently take the domain pointer. > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > [khilman: added domain as parameter to callbacks, as suggested by Dmitry] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Kevin Hilman <khilman@linaro.org> > --- > drivers/base/power/domain.c | 4 ++-- > include/linux/pm_domain.h | 6 ++++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 40bc2f4072cc..b520687046d4 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1437,7 +1437,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > spin_unlock_irq(&dev->power.lock); > > if (genpd->attach_dev) > - genpd->attach_dev(dev); > + genpd->attach_dev(genpd, dev); > > mutex_lock(&gpd_data->lock); > gpd_data->base.dev = dev; > @@ -1499,7 +1499,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd, > genpd->max_off_time_changed = true; > > if (genpd->detach_dev) > - genpd->detach_dev(dev); > + genpd->detach_dev(genpd, dev); > > spin_lock_irq(&dev->power.lock); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 73e938b7e937..b3ed7766a291 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -72,8 +72,10 @@ struct generic_pm_domain { > bool max_off_time_changed; > bool cached_power_down_ok; > struct gpd_cpuidle_data *cpuidle_data; > - void (*attach_dev)(struct device *dev); > - void (*detach_dev)(struct device *dev); > + int (*attach_dev)(struct generic_pm_domain *domain, > + struct device *dev); > + void (*detach_dev)(struct generic_pm_domain *domain, > + struct device *dev); > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) > -- > 2.1.0 >
Hi Rafael, On Thu, Nov 6, 2014 at 1:54 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, November 05, 2014 03:11:23 PM Kevin Hilman wrote: >> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: >> > On Wednesday, November 05, 2014 02:43:31 PM Kevin Hilman wrote: >> >> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: >> >> > On Thu, Oct 30, 2014 at 01:38:30PM -0700, Kevin Hilman wrote: >> >> >> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: >> >> >> > On Thursday, October 30, 2014 01:02:49 PM Ulf Hansson wrote: >> >> >> >> Convert the prototype to return and int. This is just an initial step, >> >> >> >> needed to support error handling. >> >> >> >> >> >> >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> >> >> >> >> >> Acked-by: Kevin Hilman <khilman@linaro.org> >> >> >> >> >> >> >> This patch is intended as fix for 3.18 rc[n]. Why? >> >> >> >> >> >> >> >> There are other SOC specific patches around that adds genpd support and which >> >> >> >> implements the ->attach_dev() callback. To prevent having an "atomic" patch >> >> >> >> during the next release cycle, let's change the prototype now instead. >> >> >> >> >> >> >> >> Further patches will add the actual error handling in genpd and these can then >> >> >> >> be reviewed and tested thoroughly. >> >> >> > >> >> >> > So we have no users of ->attach_dev at the moment, right? >> >> >> >> >> >> Not in mainline, but there are a couple getting ready to hit -next, so >> >> >> we wanted to fix this before they arrive so that adding the error >> >> >> handling will be easier. >> >> > >> >> > BTW, while we are at it, can we also pass the domain itself to >> >> > attach_dev() and detach_dev()? If anything it helps with debugging (you >> >> > can print domain name from the callbacks). >> >> >> >> Agreed, and it makes it match the other callbacks (power_off, power_on) >> >> which currently take struct generic_pm_domain *domain. >> >> >> >> Updated version of $SUBJECT patch below. >> > >> > The subject and changelog need to be updated too IMO. >> > >> >> Right. Here you go. > > I've replaced the Ulf's original with this one, thanks! When do you plan to send a pull request for this to Linus? There are PM domain drivers blocked on entering -next due to this dependency. As they have to pass through arm-soc first, and the arm-soc merge window will close soon, time is running out. Alternatively, you could provide an immutable branch containing this fix to unblock dependents. Thanks for your understanding. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wednesday, November 05, 2014 03:11:23 PM Kevin Hilman wrote: > "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: > > > On Wednesday, November 05, 2014 02:43:31 PM Kevin Hilman wrote: > >> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: > >> > >> > On Thu, Oct 30, 2014 at 01:38:30PM -0700, Kevin Hilman wrote: > >> >> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes: > >> >> > >> >> > On Thursday, October 30, 2014 01:02:49 PM Ulf Hansson wrote: > >> >> >> Convert the prototype to return and int. This is just an initial step, > >> >> >> needed to support error handling. > >> >> >> > >> >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >> >> > >> >> Acked-by: Kevin Hilman <khilman@linaro.org> > >> >> > >> >> >> > >> >> >> This patch is intended as fix for 3.18 rc[n]. Why? > >> >> >> > >> >> >> There are other SOC specific patches around that adds genpd support and which > >> >> >> implements the ->attach_dev() callback. To prevent having an "atomic" patch > >> >> >> during the next release cycle, let's change the prototype now instead. > >> >> >> > >> >> >> Further patches will add the actual error handling in genpd and these can then > >> >> >> be reviewed and tested thoroughly. > >> >> > > >> >> > So we have no users of ->attach_dev at the moment, right? > >> >> > >> >> Not in mainline, but there are a couple getting ready to hit -next, so > >> >> we wanted to fix this before they arrive so that adding the error > >> >> handling will be easier. > >> > > >> > BTW, while we are at it, can we also pass the domain itself to > >> > attach_dev() and detach_dev()? If anything it helps with debugging (you > >> > can print domain name from the callbacks). > >> > >> Agreed, and it makes it match the other callbacks (power_off, power_on) > >> which currently take struct generic_pm_domain *domain. > >> > >> Updated version of $SUBJECT patch below. > > > > The subject and changelog need to be updated too IMO. > > > > Right. Here you go. > > Kevin > > > ----- >8 ------ > From c18a6bf3121979c4102ba4f7edb3ac41e3921fc6 Mon Sep 17 00:00:00 2001 > From: Ulf Hansson <ulf.hansson@linaro.org> > Date: Thu, 30 Oct 2014 13:02:49 +0100 > Subject: [PATCH] PM / Domains: Change prototype for the attach and detach > callbacks > > Convert the prototypes to return an int in order to support error > handling in these callbacks. > > Also, as suggested by Dmitry Torokhov, pass the domain pointer for use > inside the callbacks, and so that they match the existing > power_on/power_off callbacks which currently take the domain pointer. > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > [khilman: added domain as parameter to callbacks, as suggested by Dmitry] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Kevin Hilman <khilman@linaro.org> That's in the Linus' tree now. > --- > drivers/base/power/domain.c | 4 ++-- > include/linux/pm_domain.h | 6 ++++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 40bc2f4072cc..b520687046d4 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1437,7 +1437,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > spin_unlock_irq(&dev->power.lock); > > if (genpd->attach_dev) > - genpd->attach_dev(dev); > + genpd->attach_dev(genpd, dev); > > mutex_lock(&gpd_data->lock); > gpd_data->base.dev = dev; > @@ -1499,7 +1499,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd, > genpd->max_off_time_changed = true; > > if (genpd->detach_dev) > - genpd->detach_dev(dev); > + genpd->detach_dev(genpd, dev); > > spin_lock_irq(&dev->power.lock); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 73e938b7e937..b3ed7766a291 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -72,8 +72,10 @@ struct generic_pm_domain { > bool max_off_time_changed; > bool cached_power_down_ok; > struct gpd_cpuidle_data *cpuidle_data; > - void (*attach_dev)(struct device *dev); > - void (*detach_dev)(struct device *dev); > + int (*attach_dev)(struct generic_pm_domain *domain, > + struct device *dev); > + void (*detach_dev)(struct generic_pm_domain *domain, > + struct device *dev); > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) >
Hi Rafael, On Sat, Nov 15, 2014 at 12:48 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> Subject: [PATCH] PM / Domains: Change prototype for the attach and detach >> callbacks >> >> Convert the prototypes to return an int in order to support error >> handling in these callbacks. >> >> Also, as suggested by Dmitry Torokhov, pass the domain pointer for use >> inside the callbacks, and so that they match the existing >> power_on/power_off callbacks which currently take the domain pointer. >> >> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> [khilman: added domain as parameter to callbacks, as suggested by Dmitry] >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> Signed-off-by: Kevin Hilman <khilman@linaro.org> > > That's in the Linus' tree now. Thank you very much. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 40bc2f4072cc..b520687046d4 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1437,7 +1437,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, spin_unlock_irq(&dev->power.lock); if (genpd->attach_dev) - genpd->attach_dev(dev); + genpd->attach_dev(genpd, dev); mutex_lock(&gpd_data->lock); gpd_data->base.dev = dev; @@ -1499,7 +1499,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd, genpd->max_off_time_changed = true; if (genpd->detach_dev) - genpd->detach_dev(dev); + genpd->detach_dev(genpd, dev); spin_lock_irq(&dev->power.lock); diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 73e938b7e937..b3ed7766a291 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -72,8 +72,10 @@ struct generic_pm_domain { bool max_off_time_changed; bool cached_power_down_ok; struct gpd_cpuidle_data *cpuidle_data; - void (*attach_dev)(struct device *dev); - void (*detach_dev)(struct device *dev); + int (*attach_dev)(struct generic_pm_domain *domain, + struct device *dev); + void (*detach_dev)(struct generic_pm_domain *domain, + struct device *dev); }; static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)