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


Groups > linux.kernel > #1531870 > unrolled thread

Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev list

Started byStephen Boyd <sboyd@codeaurora.org>
First post2016-11-29 03:50 +0100
Last post2016-11-30 06:40 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev  list Stephen Boyd <sboyd@codeaurora.org> - 2016-11-29 03:50 +0100
    Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev  list Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-29 05:00 +0100
    Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev  list Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-29 06:20 +0100
      Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev  list Stephen Boyd <sboyd@codeaurora.org> - 2016-11-29 22:00 +0100
        Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev list Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-30 02:40 +0100
          Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev  list Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-30 06:40 +0100

#1531870 — Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev list

FromStephen Boyd <sboyd@codeaurora.org>
Date2016-11-29 03:50 +0100
SubjectRe: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev list
Message-ID<sIGEx-2fz-9@gated-at.bofh.it>
On 11/25, Viresh Kumar wrote:
> Joonyoung Shim reported an interesting problem on his ARM octa-core
> Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
> was failing for a struct device for which dev_pm_opp_set_regulator() is
> called earlier.
> 
> This happened because an earlier call to
> dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
> removed all the entries from opp_table->dev_list apart from the last CPU
> device in the cpumask of CPUs sharing the OPP.
> 
> But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
> routines get CPU device for the first CPU in the cpumask. And so the OPP
> core failed to find the OPP table for the struct device.
> 
> This patch attempts to fix this problem by adding another field in the
> struct opp_device: inactive.
> 
> Instead of removing the entries from the list during
> dev_pm_opp_of_cpumask_remove_table() function call, we mark them as
> inactive. Such inactive devices will not be used by the core in most of
> the cases, like before, but will be used only at special places which
> need to take inactive devices into account.
> 
> All the devices are removed from the list together now and that happens
> only when the opp_table gets destroyed.
> 
> This patch is tested on Dual A15, Exynos5250 platform by compiling the
> cpufreq-dt driver as a module. The module is inserted/removed multiple
> times with combinations of CPU offline/online steps.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp/core.c    | 156 ++++++++++++++++++++++++++-------------
>  drivers/base/power/opp/cpu.c     |   4 +-
>  drivers/base/power/opp/debugfs.c |   4 +-
>  drivers/base/power/opp/of.c      |   2 +-
>  drivers/base/power/opp/opp.h     |   6 +-
>  5 files changed, 116 insertions(+), 56 deletions(-)

That's a lot of lines for something that we want to backport to
stable kernels!

The whole dev_list design seems fairly broken to me. Another
solution would be to iterate the cpumask in reverse, but there
doesn't seem to be a construct for that and adding one is
probably not worth the effort.

Adding yet another member to the structure and doing accounting
in different places seems to be papering over the problem as
well. Now we want to have "inactive" devices in the list? That
seems like a problem for cpufreq to solve. It can decide to not
call OPP APIs when the cpu device isn't actually physically
removed if it wants to.

It also exposes the OPP API's strong reliance on struct device
for everything. Really we shouldn't be storing device pointers in
the OPP core at all because we're not treating them like the
reference counted objects they are. The dev_list should go
probably go away and be replaced with some sort of counter. It
would also be nice if struct device had a pointer to the OPP
table(s) for a device so the lookup is direct.

BTW, _dev_pm_opp_remove_table() calls _find_opp_dev() twice, once
to find the opp_table for a device and then to find the
opp_device inside the table that was used to match up the table
in the first place. Madness!

Anyway, rant over, how about handing out the opp table pointer to
the caller so they can pass it back in when they call the put
side? That should fix the same problem if I understand correctly.

We should think about changing the API further so that callers
have to "get" the OPP table cookie for their device and then pass
that pointer to the dev_pm_*_set() APIs instead of passing a
struct device pointer. That would save lots of cycles searching
for something we already had.

 drivers/base/power/opp/core.c | 23 +++++++----------------
 drivers/cpufreq/cpufreq-dt.c  | 12 ++++++++----
 include/linux/pm_opp.h        | 12 +++++++-----
 3 files changed, 22 insertions(+), 25 deletions(-)

And the diff is 1/5 and negative.

----8<----
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 4c7c6da7a989..4a1ebec88ddd 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1316,7 +1316,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
 {
 	struct opp_table *opp_table;
 	struct regulator *reg;
@@ -1354,20 +1354,21 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 	opp_table->regulator = reg;
 
 	mutex_unlock(&opp_table_lock);
-	return 0;
+	return opp_table;
 
 err:
 	_remove_opp_table(opp_table);
+	opp_table = ERR_PTR(ret);
 unlock:
 	mutex_unlock(&opp_table_lock);
 
-	return ret;
+	return opp_table;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
 
 /**
  * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
- * @dev: Device for which regulator was set.
+ * @dev: opp_table returned from dev_pm_opp_set_regulator
  *
  * Locking: The internal opp_table and opp structures are RCU protected.
  * Hence this function internally uses RCU updater strategy with mutex locks
@@ -1375,22 +1376,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-void dev_pm_opp_put_regulator(struct device *dev)
+void dev_pm_opp_put_regulator(struct opp_table *opp_table)
 {
-	struct opp_table *opp_table;
-
 	mutex_lock(&opp_table_lock);
 
-	/* Check for existing table for 'dev' first */
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "Failed to find opp_table: %ld\n",
-			PTR_ERR(opp_table));
-		goto unlock;
-	}
-
 	if (IS_ERR(opp_table->regulator)) {
-		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		pr_err("%s: Doesn't have regulator set\n", __func__);
 		goto unlock;
 	}
 
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 5c07ae05d69a..4d3ec92cbabf 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -28,6 +28,7 @@
 #include "cpufreq-dt.h"
 
 struct private_data {
+	struct opp_table *opp_table;
 	struct device *cpu_dev;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
@@ -143,6 +144,7 @@ static int resources_available(void)
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_frequency_table *freq_table;
+	struct opp_table *opp_table = NULL;
 	struct private_data *priv;
 	struct device *cpu_dev;
 	struct clk *cpu_clk;
@@ -186,8 +188,9 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 */
 	name = find_supply_name(cpu_dev);
 	if (name) {
-		ret = dev_pm_opp_set_regulator(cpu_dev, name);
-		if (ret) {
+		opp_table = dev_pm_opp_set_regulator(cpu_dev, name);
+		if (IS_ERR(opp_table)) {
+			ret = PTR_ERR(opp_table);
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
 			goto out_put_clk;
@@ -237,6 +240,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->reg_name = name;
+	priv->opp_table = opp_table;
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
@@ -285,7 +289,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
-		dev_pm_opp_put_regulator(cpu_dev);
+		dev_pm_opp_put_regulator(opp_table);
 out_put_clk:
 	clk_put(cpu_clk);
 
@@ -300,7 +304,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	if (priv->reg_name)
-		dev_pm_opp_put_regulator(priv->cpu_dev);
+		dev_pm_opp_put_regulator(priv->opp_table);
 
 	clk_put(policy->clk);
 	kfree(priv);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index bca26157f5b6..a2066abb2a35 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -19,6 +19,7 @@
 
 struct dev_pm_opp;
 struct device;
+struct opp_table;
 
 enum dev_pm_opp_event {
 	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
@@ -62,8 +63,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 void dev_pm_opp_put_supported_hw(struct device *dev);
 int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
-int dev_pm_opp_set_regulator(struct device *dev, const char *name);
-void dev_pm_opp_put_regulator(struct device *dev);
+struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name);
+void dev_pm_opp_put_regulator(struct opp_table *opp_table);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -170,12 +171,13 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
 
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+static inline struct opp_table *
+dev_pm_opp_set_regulator(struct device *dev, const char *name)
 {
-	return -ENOTSUPP;
+	return ERR_PTR(-ENOTSUPP);
 }
 
-static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {}
 
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]


#1531886

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-29 05:00 +0100
Message-ID<sIHKh-2Xo-1@gated-at.bofh.it>
In reply to#1531870
On 28-11-16, 18:46, Stephen Boyd wrote:
> That's a lot of lines for something that we want to backport to
> stable kernels!

Hmm, I agree.

> The whole dev_list design seems fairly broken to me. Another
> solution would be to iterate the cpumask in reverse, but there
> doesn't seem to be a construct for that and adding one is
> probably not worth the effort.
> 
> Adding yet another member to the structure and doing accounting
> in different places seems to be papering over the problem as
> well. Now we want to have "inactive" devices in the list? That
> seems like a problem for cpufreq to solve. It can decide to not
> call OPP APIs when the cpu device isn't actually physically
> removed if it wants to.
> 
> It also exposes the OPP API's strong reliance on struct device
> for everything. Really we shouldn't be storing device pointers in
> the OPP core at all because we're not treating them like the
> reference counted objects they are. The dev_list should go
> probably go away and be replaced with some sort of counter. It
> would also be nice if struct device had a pointer to the OPP
> table(s) for a device so the lookup is direct.

If the struct device gets a pointer to the opp-table, then yes we can kill the
dev-list completely. I will work on cleaning up OPP core a bit later on.

> BTW, _dev_pm_opp_remove_table() calls _find_opp_dev() twice, once
> to find the opp_table for a device and then to find the
> opp_device inside the table that was used to match up the table
> in the first place. Madness!
> 
> Anyway, rant over, how about handing out the opp table pointer to
> the caller so they can pass it back in when they call the put
> side? That should fix the same problem if I understand correctly.

Yes, that can be a solution for the time being.

> We should think about changing the API further so that callers
> have to "get" the OPP table cookie for their device and then pass
> that pointer to the dev_pm_*_set() APIs instead of passing a
> struct device pointer. That would save lots of cycles searching
> for something we already had.

Hmm, we need to do some cleanup soon I believe. Also note that we want to kill
the RCU thing :)

> -static inline void dev_pm_opp_put_regulator(struct device *dev) {}
> +static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {}

We need to modify few more things as well. I will send a patch for that soon.

-- 
viresh

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


#1531902

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-29 06:20 +0100
Message-ID<sIIZH-3Ua-1@gated-at.bofh.it>
In reply to#1531870
On 28-11-16, 18:46, Stephen Boyd wrote:
> Anyway, rant over, how about handing out the opp table pointer to
> the caller so they can pass it back in when they call the put
> side? That should fix the same problem if I understand correctly.

Hmm, so the problem is that all below routines (and their callers) need to get
updated:

int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
void dev_pm_opp_put_supported_hw(struct device *dev);
int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct device *dev);
struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name);
void dev_pm_opp_put_regulator(struct opp_table *opp_table);

And that will make it difficult to get it back to stable kernels, specially
because they were all added in different kernel releases after 4.4.

And we also need to fix them properly, with something like a cookie instead of a
plain opp_table pointer.

I suggest this patch for the time being, with a big FIXME in it, so that we can
get it to stable kernels.

-------------------------8<-------------------------

diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c
index 8c3434bdb26d..2e96efdb10b2 100644
--- a/drivers/base/power/opp/cpu.c
+++ b/drivers/base/power/opp/cpu.c
@@ -118,26 +118,45 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
 #endif /* CONFIG_CPU_FREQ */
 
+void _cpu_remove_table(unsigned int cpu, bool of)
+{
+       struct device *cpu_dev = get_cpu_device(cpu);
+
+       if (!cpu_dev) {
+               pr_err("%s: failed to get cpu%d device\n", __func__, cpu);
+               return;
+       }
+
+       if (of)
+               dev_pm_opp_of_remove_table(cpu_dev);
+       else
+               dev_pm_opp_remove_table(cpu_dev);
+}
+
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of)
 {
        struct device *cpu_dev;
-       int cpu;
+       int cpu, first_cpu;
 
        WARN_ON(cpumask_empty(cpumask));
 
-       for_each_cpu(cpu, cpumask) {
-               cpu_dev = get_cpu_device(cpu);
-               if (!cpu_dev) {
-                       pr_err("%s: failed to get cpu%d device\n", __func__,
-                              cpu);
-                       continue;
-               }
-
-               if (of)
-                       dev_pm_opp_of_remove_table(cpu_dev);
-               else
-                       dev_pm_opp_remove_table(cpu_dev);
-       }
+       /*
+        * The first cpu in the cpumask is important as that is used to create
+        * the opp-table initially and routines like dev_pm_opp_put_regulator()
+        * will expect the list-dev for the first CPU to be present while such
+        * routines are called, otherwise we will fail to find the opp-table for
+        * such devices.
+        *
+        * FIXME: Cleanup this mess and implement cookie based solutions instead
+        * of working on the device pointer.
+        */
+       first_cpu = cpumask_first(cpumask);
+       cpumask_clear_cpu(first_cpu, cpumask);
+
+       for_each_cpu(cpu, cpumask)
+               _cpu_remove_table(cpu, of);
+
+       _cpu_remove_table(first_cpu, of);
 }
 
 /**

-- 
viresh

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


#1532763

FromStephen Boyd <sboyd@codeaurora.org>
Date2016-11-29 22:00 +0100
Message-ID<sIXFo-4YE-25@gated-at.bofh.it>
In reply to#1531902
On 11/29, Viresh Kumar wrote:
> On 28-11-16, 18:46, Stephen Boyd wrote:
> > Anyway, rant over, how about handing out the opp table pointer to
> > the caller so they can pass it back in when they call the put
> > side? That should fix the same problem if I understand correctly.
> 
> Hmm, so the problem is that all below routines (and their callers) need to get
> updated:
> 
> int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
> void dev_pm_opp_put_supported_hw(struct device *dev);
> int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
> void dev_pm_opp_put_prop_name(struct device *dev);
> struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name);
> void dev_pm_opp_put_regulator(struct opp_table *opp_table);
> 
> And that will make it difficult to get it back to stable kernels, specially
> because they were all added in different kernel releases after 4.4.

Why do we care? The put variants of the prop and supported_hw
functions are never called, so we're not going to hit this
problem there. Sure the code is broken, but nobody is using the
code in mainline so there isn't anything to backport to stable
urgently.

> 
> And we also need to fix them properly, with something like a cookie instead of a
> plain opp_table pointer.

Perhaps this means my approach in using opp_table is undesirable
for some reason? Care to elaborate why?

> 
> I suggest this patch for the time being, with a big FIXME in it, so that we can
> get it to stable kernels.
> 
> -------------------------8<-------------------------
> 
> diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c
> index 8c3434bdb26d..2e96efdb10b2 100644
> --- a/drivers/base/power/opp/cpu.c
> +++ b/drivers/base/power/opp/cpu.c
> @@ -118,26 +118,45 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev,
>  EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
>  #endif /* CONFIG_CPU_FREQ */
>  
> +void _cpu_remove_table(unsigned int cpu, bool of)

static?

> +{
> +       struct device *cpu_dev = get_cpu_device(cpu);
> +
> +       if (!cpu_dev) {
> +               pr_err("%s: failed to get cpu%d device\n", __func__, cpu);
> +               return;
> +       }
> +
> +       if (of)
> +               dev_pm_opp_of_remove_table(cpu_dev);
> +       else
> +               dev_pm_opp_remove_table(cpu_dev);
> +}
> +
>  void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of)
>  {
>         struct device *cpu_dev;
> -       int cpu;
> +       int cpu, first_cpu;
>  
>         WARN_ON(cpumask_empty(cpumask));
>  
> -       for_each_cpu(cpu, cpumask) {
> -               cpu_dev = get_cpu_device(cpu);
> -               if (!cpu_dev) {
> -                       pr_err("%s: failed to get cpu%d device\n", __func__,
> -                              cpu);
> -                       continue;
> -               }
> -
> -               if (of)
> -                       dev_pm_opp_of_remove_table(cpu_dev);
> -               else
> -                       dev_pm_opp_remove_table(cpu_dev);
> -       }
> +       /*
> +        * The first cpu in the cpumask is important as that is used to create
> +        * the opp-table initially and routines like dev_pm_opp_put_regulator()
> +        * will expect the list-dev for the first CPU to be present while such
> +        * routines are called, otherwise we will fail to find the opp-table for
> +        * such devices.

This seems a lot like the patch from Joonyoung. It would be good
to add a note that the patch is based on that one and also a
reported-by tag.

Also, this approach is brittle as it requires that the first
device in the mask be used for the set/put APIs, when that could
be any of the devices. I'd prefer we used my patch because it
isn't as easy to break and more directly fixes the problem at
hand.

> +        *
> +        * FIXME: Cleanup this mess and implement cookie based solutions instead
> +        * of working on the device pointer.
> +        */
> +       first_cpu = cpumask_first(cpumask);
> +       cpumask_clear_cpu(first_cpu, cpumask);
> +
> +       for_each_cpu(cpu, cpumask)
> +               _cpu_remove_table(cpu, of);
> +
> +       _cpu_remove_table(first_cpu, of);
>  }

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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


#1532898 — Re: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev list

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-30 02:40 +0100
SubjectRe: [PATCH] PM / OPP: Allow inactive opp_device to be present in dev list
Message-ID<sJ22m-7Nz-7@gated-at.bofh.it>
In reply to#1532763
On 30 November 2016 at 02:26, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/29, Viresh Kumar wrote:
>> On 28-11-16, 18:46, Stephen Boyd wrote:
>> > Anyway, rant over, how about handing out the opp table pointer to
>> > the caller so they can pass it back in when they call the put
>> > side? That should fix the same problem if I understand correctly.
>>
>> Hmm, so the problem is that all below routines (and their callers) need to get
>> updated:
>>
>> int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
>> void dev_pm_opp_put_supported_hw(struct device *dev);
>> int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
>> void dev_pm_opp_put_prop_name(struct device *dev);
>> struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name);
>> void dev_pm_opp_put_regulator(struct opp_table *opp_table);
>>
>> And that will make it difficult to get it back to stable kernels, specially
>> because they were all added in different kernel releases after 4.4.
>
> Why do we care? The put variants of the prop and supported_hw
> functions are never called, so we're not going to hit this
> problem there. Sure the code is broken, but nobody is using the
> code in mainline so there isn't anything to backport to stable
> urgently.

Hmm, only the set variants are used by the sti driver.

>>
>> And we also need to fix them properly, with something like a cookie instead of a
>> plain opp_table pointer.
>
> Perhaps this means my approach in using opp_table is undesirable
> for some reason? Care to elaborate why?

You only suggested the cookie method as well, isn't it ? I am fine with your
patch as well, the only problem is that we will have different prototype for
a single set of APIs..

> I'd prefer we used my patch because it
> isn't as easy to break and more directly fixes the problem at
> hand.

Okay, can you please send it formally and I can Ack it then ?

--
viresh

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


#1532964

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-30 06:40 +0100
Message-ID<sJ5MB-1Us-7@gated-at.bofh.it>
In reply to#1532898
On 30-11-16, 07:06, Viresh Kumar wrote:
> Okay, can you please send it formally and I can Ack it then ?

I have sent it now to move things faster. Thanks.

-- 
viresh

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web