[PATCH] PM / OPP: Implement free_opp_table function

[PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

At the driver unloading time the associated opp table may need
to be deleted. Otherwise it amounts to memory leak. The existing
OPP library does not have provison to do so.

Hence this patch implements the function to free the opp table.

Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
---
 drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h   |  6 ++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index d9e376a..d45ffd5 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
+
+/**
+ * dev_pm_opp_free_opp_table() - free the opp table
+ * @dev:	device for which we do this operation
+ *
+ * Free up the allocated opp table
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking or synchronize_rcu() blocking calls cannot be used.
+ */
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+	struct device_opp *dev_opp = NULL;
+	struct dev_pm_opp *opp;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	/* Check for existing list for 'dev' */
+	dev_opp = find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		mutex_unlock(&dev_opp_list_lock);
+		return;
+	}
+
+	while (!list_empty(&dev_opp->opp_list)) {
+		opp = list_entry_rcu(dev_opp->opp_list.next,
+					struct dev_pm_opp, node);
+		list_del_rcu(&opp->node);
+		kfree_rcu(opp, head);
+	}
+
+	list_del_rcu(&dev_opp->node);
+	mutex_unlock(&dev_opp_list_lock);
+	synchronize_rcu();
+	kfree(dev_opp);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
 #endif
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0330217..3c29620 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+
+void dev_pm_opp_free_opp_table(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -105,6 +107,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 {
 	return ERR_PTR(-EINVAL);
 }
+
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+}
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
1.8.3.2

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Viresh Kumar]

On 16 May 2014 14:39, Inderpal Singh <inderpal.s@samsung.com> wrote:
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> +void dev_pm_opp_free_opp_table(struct device *dev)
> +{
> +       struct device_opp *dev_opp = NULL;
> +       struct dev_pm_opp *opp;
> +
> +       /* Hold our list modification lock here */
> +       mutex_lock(&dev_opp_list_lock);
> +
> +       /* Check for existing list for 'dev' */
> +       dev_opp = find_device_opp(dev);
> +       if (IS_ERR(dev_opp)) {
> +               mutex_unlock(&dev_opp_list_lock);
> +               return;
> +       }
> +
> +       while (!list_empty(&dev_opp->opp_list)) {
> +               opp = list_entry_rcu(dev_opp->opp_list.next,
> +                                       struct dev_pm_opp, node);

list_for_each_entry_rcu ?

> +               list_del_rcu(&opp->node);
> +               kfree_rcu(opp, head);
> +       }
> +
> +       list_del_rcu(&dev_opp->node);
> +       mutex_unlock(&dev_opp_list_lock);
> +       synchronize_rcu();
> +       kfree(dev_opp);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
>  #endif
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0330217..3c29620 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>
>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
> +
> +void dev_pm_opp_free_opp_table(struct device *dev);
>  #else
>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>  {
> @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
>  {
>         return ERR_PTR(-EINVAL);
>  }
> +
> +void dev_pm_opp_free_opp_table(struct device *dev)
> +{
> +}
>  #endif         /* CONFIG_PM_OPP */
>
>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)

Otherwise looks fine.

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

Hi Viresh,

Thanks for the review.

On Fri, May 16, 2014 at 3:36 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16 May 2014 14:39, Inderpal Singh <inderpal.s@samsung.com> wrote:
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +       struct device_opp *dev_opp = NULL;
>> +       struct dev_pm_opp *opp;
>> +
>> +       /* Hold our list modification lock here */
>> +       mutex_lock(&dev_opp_list_lock);
>> +
>> +       /* Check for existing list for 'dev' */
>> +       dev_opp = find_device_opp(dev);
>> +       if (IS_ERR(dev_opp)) {
>> +               mutex_unlock(&dev_opp_list_lock);
>> +               return;
>> +       }
>> +
>> +       while (!list_empty(&dev_opp->opp_list)) {
>> +               opp = list_entry_rcu(dev_opp->opp_list.next,
>> +                                       struct dev_pm_opp, node);
>
> list_for_each_entry_rcu ?
>

list_for_each_entry_rcu can not be used as opp is being deleted in the loop.

Thanks,
Inder

>> +               list_del_rcu(&opp->node);
>> +               kfree_rcu(opp, head);
>> +       }
>> +
>> +       list_del_rcu(&dev_opp->node);
>> +       mutex_unlock(&dev_opp_list_lock);
>> +       synchronize_rcu();
>> +       kfree(dev_opp);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
>>  #endif
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index 0330217..3c29620 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
>>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>>
>>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev);
>>  #else
>>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>>  {
>> @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
>>  {
>>         return ERR_PTR(-EINVAL);
>>  }
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +}
>>  #endif         /* CONFIG_PM_OPP */
>>
>>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
>
> Otherwise looks fine.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Viresh Kumar]

On 16 May 2014 22:38, Inderpal Singh <inderpal.s@samsung.com> wrote:
>>> +       while (!list_empty(&dev_opp->opp_list)) {
>>> +               opp = list_entry_rcu(dev_opp->opp_list.next,
>>> +                                       struct dev_pm_opp, node);
>>
>> list_for_each_entry_rcu ?
>>
>
> list_for_each_entry_rcu can not be used as opp is being deleted in the loop.

So what? The above code can be replaced easily I think.
This is how it is implemented:

#define list_for_each_entry_rcu(pos, head, member) \
        for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
                &pos->member != (head); \
                pos = list_entry_rcu(pos->member.next, typeof(*pos), member))

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Fri, May 16, 2014 at 10:41 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16 May 2014 22:38, Inderpal Singh <inderpal.s@samsung.com> wrote:
>>>> +       while (!list_empty(&dev_opp->opp_list)) {
>>>> +               opp = list_entry_rcu(dev_opp->opp_list.next,
>>>> +                                       struct dev_pm_opp, node);
>>>
>>> list_for_each_entry_rcu ?
>>>
>>
>> list_for_each_entry_rcu can not be used as opp is being deleted in the loop.
>
> So what? The above code can be replaced easily I think.
> This is how it is implemented:
>
> #define list_for_each_entry_rcu(pos, head, member) \
>         for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
>                 &pos->member != (head); \
>                 pos = list_entry_rcu(pos->member.next, typeof(*pos), member))

by the time "pos = list_entry_rcu(pos->member.
next, typeof(*pos), member)" is executed, the pos would have been
freed in the loop.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Viresh Kumar]

On 16 May 2014 22:54, Inderpal Singh <inderpal.s@samsung.com> wrote:
> by the time "pos = list_entry_rcu(pos->member.
> next, typeof(*pos), member)" is executed, the pos would have been
> freed in the loop.

Oops!!

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Nishanth Menon]

On 05/16/2014 04:09 AM, Inderpal Singh wrote:
> At the driver unloading time the associated opp table may need
> to be deleted. Otherwise it amounts to memory leak. The existing
> OPP library does not have provison to do so.
> 
> Hence this patch implements the function to free the opp table.
> 
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
>  drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h   |  6 ++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index d9e376a..d45ffd5 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_init_opp_table);
> +
> +/**
> + * dev_pm_opp_free_opp_table() - free the opp table
> + * @dev:	device for which we do this operation
> + *
> + * Free up the allocated opp table
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks to
> + * keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
> + */
> +void dev_pm_opp_free_opp_table(struct device *dev)
> +{
> +	struct device_opp *dev_opp = NULL;
> +	struct dev_pm_opp *opp;
> +
if (!dev)
	return;

> +	/* Hold our list modification lock here */
> +	mutex_lock(&dev_opp_list_lock);
> +
> +	/* Check for existing list for 'dev' */
> +	dev_opp = find_device_opp(dev);
> +	if (IS_ERR(dev_opp)) {
> +		mutex_unlock(&dev_opp_list_lock);
> +		return;
> +	}
> +
> +	while (!list_empty(&dev_opp->opp_list)) {
> +		opp = list_entry_rcu(dev_opp->opp_list.next,
> +					struct dev_pm_opp, node);
> +		list_del_rcu(&opp->node);
> +		kfree_rcu(opp, head);
> +	}

How about the OPP notifiers? should'nt we add a new event
OPP_EVENT_REMOVE?

To maintain non-dt behavior coherency, should'nt we rather add a
opp_remove or an opp_del function?

> +
> +	list_del_rcu(&dev_opp->node);
> +	mutex_unlock(&dev_opp_list_lock);
> +	synchronize_rcu();
> +	kfree(dev_opp);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
>  #endif
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0330217..3c29620 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>  
>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
> +
> +void dev_pm_opp_free_opp_table(struct device *dev);
>  #else
>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>  {
> @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
>  {
>  	return ERR_PTR(-EINVAL);
>  }
> +
> +void dev_pm_opp_free_opp_table(struct device *dev)
> +{
> +}
>  #endif		/* CONFIG_PM_OPP */
>  
>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

Hi Nishanth,

Thanks for the review comments.

On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon <nm@ti.com> wrote:
> On 05/16/2014 04:09 AM, Inderpal Singh wrote:
>> At the driver unloading time the associated opp table may need
>> to be deleted. Otherwise it amounts to memory leak. The existing
>> OPP library does not have provison to do so.
>>
>> Hence this patch implements the function to free the opp table.
>>
>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>> ---
>>  drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_opp.h   |  6 ++++++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index d9e376a..d45ffd5 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>>       return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>> +
>> +/**
>> + * dev_pm_opp_free_opp_table() - free the opp table
>> + * @dev:     device for which we do this operation
>> + *
>> + * Free up the allocated opp table
>> + *
>> + * Locking: The internal device_opp and opp structures are RCU protected.
>> + * Hence this function internally uses RCU updater strategy with mutex locks to
>> + * keep the integrity of the internal data structures. Callers should ensure
>> + * that this function is *NOT* called under RCU protection or in contexts where
>> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
>> + */
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +     struct device_opp *dev_opp = NULL;
>> +     struct dev_pm_opp *opp;
>> +
> if (!dev)
>         return;
>

missed it. Will take care in the next version.

>> +     /* Hold our list modification lock here */
>> +     mutex_lock(&dev_opp_list_lock);
>> +
>> +     /* Check for existing list for 'dev' */
>> +     dev_opp = find_device_opp(dev);
>> +     if (IS_ERR(dev_opp)) {
>> +             mutex_unlock(&dev_opp_list_lock);
>> +             return;
>> +     }
>> +
>> +     while (!list_empty(&dev_opp->opp_list)) {
>> +             opp = list_entry_rcu(dev_opp->opp_list.next,
>> +                                     struct dev_pm_opp, node);
>> +             list_del_rcu(&opp->node);
>> +             kfree_rcu(opp, head);
>> +     }
>
> How about the OPP notifiers? should'nt we add a new event
> OPP_EVENT_REMOVE?
>

As this function is to free the whole opp table. Hence, I think,
notifier may not be needed. It may be required for per opp removal as
is the case with opp addition and enable/disable. But at present there
are no users of these notifiers at all. Let me know your view.

> To maintain non-dt behavior coherency, should'nt we rather add a
> opp_remove or an opp_del function?

Yes we should have opp_remove as well, but what's the use case ?
Should we go ahead and implement it Or, wait for the use-case?

Thanks,
Inder

>
>> +
>> +     list_del_rcu(&dev_opp->node);
>> +     mutex_unlock(&dev_opp_list_lock);
>> +     synchronize_rcu();
>> +     kfree(dev_opp);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
>>  #endif
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index 0330217..3c29620 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
>>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>>
>>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev);
>>  #else
>>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>>  {
>> @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
>>  {
>>       return ERR_PTR(-EINVAL);
>>  }
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +}
>>  #endif               /* CONFIG_PM_OPP */
>>
>>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
>>
>
>
> --
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Nishanth Menon]

On Mon, May 19, 2014 at 1:08 PM, Inderpal Singh <inderpal.s@samsung.com> wrote:
> Hi Nishanth,
>
> Thanks for the review comments.
>
> On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon <nm@ti.com> wrote:
>> On 05/16/2014 04:09 AM, Inderpal Singh wrote:
>>> At the driver unloading time the associated opp table may need
>>> to be deleted. Otherwise it amounts to memory leak. The existing
>>> OPP library does not have provison to do so.
>>>
>>> Hence this patch implements the function to free the opp table.
>>>
>>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>>> ---
>>>  drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/pm_opp.h   |  6 ++++++
>>>  2 files changed, 47 insertions(+)
>>>
>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>> index d9e376a..d45ffd5 100644
>>> --- a/drivers/base/power/opp.c
>>> +++ b/drivers/base/power/opp.c
>>> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>>>       return 0;
>>>  }
>>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>> +
>>> +/**
>>> + * dev_pm_opp_free_opp_table() - free the opp table
>>> + * @dev:     device for which we do this operation
>>> + *
>>> + * Free up the allocated opp table
>>> + *
>>> + * Locking: The internal device_opp and opp structures are RCU protected.
>>> + * Hence this function internally uses RCU updater strategy with mutex locks to
>>> + * keep the integrity of the internal data structures. Callers should ensure
>>> + * that this function is *NOT* called under RCU protection or in contexts where
>>> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
>>> + */
>>> +void dev_pm_opp_free_opp_table(struct device *dev)
>>> +{
>>> +     struct device_opp *dev_opp = NULL;
>>> +     struct dev_pm_opp *opp;
>>> +
>> if (!dev)
>>         return;
>>
>
> missed it. Will take care in the next version.
>
>>> +     /* Hold our list modification lock here */
>>> +     mutex_lock(&dev_opp_list_lock);
>>> +
>>> +     /* Check for existing list for 'dev' */
>>> +     dev_opp = find_device_opp(dev);
>>> +     if (IS_ERR(dev_opp)) {
>>> +             mutex_unlock(&dev_opp_list_lock);
>>> +             return;
>>> +     }
>>> +
>>> +     while (!list_empty(&dev_opp->opp_list)) {
>>> +             opp = list_entry_rcu(dev_opp->opp_list.next,
>>> +                                     struct dev_pm_opp, node);
>>> +             list_del_rcu(&opp->node);
>>> +             kfree_rcu(opp, head);
>>> +     }
>>
>> How about the OPP notifiers? should'nt we add a new event
>> OPP_EVENT_REMOVE?
>>
>
> As this function is to free the whole opp table. Hence, I think,
> notifier may not be needed. It may be required for per opp removal as
> is the case with opp addition and enable/disable. But at present there
> are no users of these notifiers at all. Let me know your view.

umm.. we do have devfreq which depends on OPPs :).

>> To maintain non-dt behavior coherency, should'nt we rather add a
>> opp_remove or an opp_del function?
>
> Yes we should have opp_remove as well, but what's the use case ?
> Should we go ahead and implement it Or, wait for the use-case?

IMHO, if we are doing it properly, we should add the requisite
function as well. we dont want to have differing behavior device tree
Vs non-DT.

Regards,
Nishanth Menon

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Tue, May 20, 2014 at 12:04 AM, Nishanth Menon <nm@ti.com> wrote:
> On Mon, May 19, 2014 at 1:08 PM, Inderpal Singh <inderpal.s@samsung.com> wrote:
>> Hi Nishanth,
>>
>> Thanks for the review comments.
>>
>> On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon <nm@ti.com> wrote:
>>> On 05/16/2014 04:09 AM, Inderpal Singh wrote:
>>>> At the driver unloading time the associated opp table may need
>>>> to be deleted. Otherwise it amounts to memory leak. The existing
>>>> OPP library does not have provison to do so.
>>>>
>>>> Hence this patch implements the function to free the opp table.
>>>>
>>>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>>>> ---
>>>>  drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/pm_opp.h   |  6 ++++++
>>>>  2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>>> index d9e376a..d45ffd5 100644
>>>> --- a/drivers/base/power/opp.c
>>>> +++ b/drivers/base/power/opp.c
>>>> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>>>>       return 0;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>>> +
>>>> +/**
>>>> + * dev_pm_opp_free_opp_table() - free the opp table
>>>> + * @dev:     device for which we do this operation
>>>> + *
>>>> + * Free up the allocated opp table
>>>> + *
>>>> + * Locking: The internal device_opp and opp structures are RCU protected.
>>>> + * Hence this function internally uses RCU updater strategy with mutex locks to
>>>> + * keep the integrity of the internal data structures. Callers should ensure
>>>> + * that this function is *NOT* called under RCU protection or in contexts where
>>>> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
>>>> + */
>>>> +void dev_pm_opp_free_opp_table(struct device *dev)
>>>> +{
>>>> +     struct device_opp *dev_opp = NULL;
>>>> +     struct dev_pm_opp *opp;
>>>> +
>>> if (!dev)
>>>         return;
>>>
>>
>> missed it. Will take care in the next version.
>>
>>>> +     /* Hold our list modification lock here */
>>>> +     mutex_lock(&dev_opp_list_lock);
>>>> +
>>>> +     /* Check for existing list for 'dev' */
>>>> +     dev_opp = find_device_opp(dev);
>>>> +     if (IS_ERR(dev_opp)) {
>>>> +             mutex_unlock(&dev_opp_list_lock);
>>>> +             return;
>>>> +     }
>>>> +
>>>> +     while (!list_empty(&dev_opp->opp_list)) {
>>>> +             opp = list_entry_rcu(dev_opp->opp_list.next,
>>>> +                                     struct dev_pm_opp, node);
>>>> +             list_del_rcu(&opp->node);
>>>> +             kfree_rcu(opp, head);
>>>> +     }
>>>
>>> How about the OPP notifiers? should'nt we add a new event
>>> OPP_EVENT_REMOVE?
>>>
>>
>> As this function is to free the whole opp table. Hence, I think,
>> notifier may not be needed. It may be required for per opp removal as
>> is the case with opp addition and enable/disable. But at present there
>> are no users of these notifiers at all. Let me know your view.
>
> umm.. we do have devfreq which depends on OPPs :).

Yes, devfreq does depend on OPPs, but no devfreq driver is registering
its notifier_block to handle OPP notifications.

>
>>> To maintain non-dt behavior coherency, should'nt we rather add a
>>> opp_remove or an opp_del function?
>>
>> Yes we should have opp_remove as well, but what's the use case ?
>> Should we go ahead and implement it Or, wait for the use-case?
>
> IMHO, if we are doing it properly, we should add the requisite
> function as well. we dont want to have differing behavior device tree
> Vs non-DT.

So we will have 2 functions then. One to remove the whole opp table
and the the other to remove the individual OPPs.
I will cover this in v2. Will also take care of the OPP_EVENT_REMOVE
notification part.


Regards,
Inder

>
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Nishanth Menon]

On Tue, May 20, 2014 at 12:36 AM, Inderpal Singh <inderpal.s@samsung.com> wrote:
> On Tue, May 20, 2014 at 12:04 AM, Nishanth Menon <nm@ti.com> wrote:
>> On Mon, May 19, 2014 at 1:08 PM, Inderpal Singh <inderpal.s@samsung.com> wrote:
>>> Hi Nishanth,
>>>
>>> Thanks for the review comments.
>>>
>>> On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon <nm@ti.com> wrote:
>>>> On 05/16/2014 04:09 AM, Inderpal Singh wrote:

[..]
>>>>> +     /* Hold our list modification lock here */
>>>>> +     mutex_lock(&dev_opp_list_lock);
>>>>> +
>>>>> +     /* Check for existing list for 'dev' */
>>>>> +     dev_opp = find_device_opp(dev);
>>>>> +     if (IS_ERR(dev_opp)) {
>>>>> +             mutex_unlock(&dev_opp_list_lock);
>>>>> +             return;
>>>>> +     }
>>>>> +
>>>>> +     while (!list_empty(&dev_opp->opp_list)) {
>>>>> +             opp = list_entry_rcu(dev_opp->opp_list.next,
>>>>> +                                     struct dev_pm_opp, node);
>>>>> +             list_del_rcu(&opp->node);
>>>>> +             kfree_rcu(opp, head);
>>>>> +     }
>>>>
>>>> How about the OPP notifiers? should'nt we add a new event
>>>> OPP_EVENT_REMOVE?
>>>>
>>>
>>> As this function is to free the whole opp table. Hence, I think,
>>> notifier may not be needed. It may be required for per opp removal as
>>> is the case with opp addition and enable/disable. But at present there
>>> are no users of these notifiers at all. Let me know your view.
>>
>> umm.. we do have devfreq which depends on OPPs :).
>
> Yes, devfreq does depend on OPPs, but no devfreq driver is registering
> its notifier_block to handle OPP notifications.
>

Lets not forget the power of downstream tree drivers that use the API set :)

>>
>>>> To maintain non-dt behavior coherency, should'nt we rather add a
>>>> opp_remove or an opp_del function?
>>>
>>> Yes we should have opp_remove as well, but what's the use case ?
>>> Should we go ahead and implement it Or, wait for the use-case?
>>
>> IMHO, if we are doing it properly, we should add the requisite
>> function as well. we dont want to have differing behavior device tree
>> Vs non-DT.
>
> So we will have 2 functions then. One to remove the whole opp table
> and the the other to remove the individual OPPs.
> I will cover this in v2. Will also take care of the OPP_EVENT_REMOVE
> notification part.
>

Thanks.

--
Regards,
Nishanth Menon

[PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Inderpal Singh]

At the driver unloading time the associated opps and its table may need
to be deleted. Otherwise it amounts to memory leak. The existing
OPP library does not have provision to do so.

Hence this patch implements the required functions to free the same.

Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
---
Changes in v2:
	- As suggested by Nishanth, added support to remove a single OPP
	  for non-DT users
	- Added an internal helper function to avoid the code duplication
	  between opp_remove and free_opp_table functions
 
 drivers/base/power/opp.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pm_opp.h   |  14 +++++-
 2 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index faae9cf..22adef3 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -89,6 +89,7 @@ struct device_opp {
 	struct device *dev;
 	struct srcu_notifier_head head;
 	struct list_head opp_list;
+	struct rcu_head head_rcu;
 };
 
 /*
@@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 				__func__);
 			return -ENOMEM;
 		}
-
 		dev_opp->dev = dev;
 		srcu_init_notifier_head(&dev_opp->head);
 		INIT_LIST_HEAD(&dev_opp->opp_list);
@@ -464,6 +464,82 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
+ * opp_remove_and_free()  - Internal helper to remove and free an OPP
+ * @dev_opp:	device_opp for which we do this operation
+ * @opp:	OPP to be removed
+ *
+ * This function removes an opp from the opp list and frees it. And, if the
+ * list is empty, it also removes the device_opp from the root list and
+ * frees it.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * The callers need to use RCU updater strategy with mutex locks to keep the
+ * integrity of the internal data structures.
+ */
+static void opp_remove_and_free(struct device_opp *dev_opp,
+						struct dev_pm_opp *opp)
+{
+	list_del_rcu(&opp->node);
+	/*
+	 * Notify the removal of the opp
+	 */
+	srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_REMOVE, opp);
+	kfree_rcu(opp, head);
+
+	if (list_empty(&dev_opp->opp_list)) {
+		list_del_rcu(&dev_opp->node);
+		kfree_rcu(dev_opp, head_rcu);
+	}
+}
+
+/**
+ * dev_pm_opp_remove() - remove a specific OPP
+ * @dev:	device for which we do this operation
+ * @freq:	OPP frequency to remove
+ *
+ * Removes a provided opp.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking cannot be used.
+ */
+void dev_pm_opp_remove(struct device *dev, unsigned long freq)
+{
+	struct device_opp *dev_opp = ERR_PTR(-ENODEV);
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+	if (IS_ERR_OR_NULL(dev))
+		return;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	/* Check for existing list for 'dev' */
+	dev_opp = find_device_opp(dev);
+	if (IS_ERR(dev_opp))
+		goto unlock;
+
+	/* Do we have the frequency? */
+	list_for_each_entry(temp_opp, &dev_opp->opp_list, node)
+		if (temp_opp->rate == freq) {
+			opp = temp_opp;
+			break;
+		}
+
+	if (IS_ERR(opp)) {
+		dev_warn(dev, "%s: OPP freq not found\n", __func__);
+		goto unlock;
+	}
+
+	opp_remove_and_free(dev_opp, opp);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+
+/**
  * opp_set_availability() - helper to set the availability of an opp
  * @dev:		device for which we do this operation
  * @freq:		OPP frequency to modify availability
@@ -653,3 +729,47 @@ int of_init_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
+
+/**
+ * dev_pm_opp_free_opp_table() - free the opp table
+ * @dev:	device for which we do this operation
+ *
+ * Free up the allocated opp table
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking cannot be used.
+ */
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+	struct device_opp *dev_opp = ERR_PTR(-ENODEV);
+	struct dev_pm_opp *opp;
+	int count = 0;
+
+	if (IS_ERR_OR_NULL(dev))
+		return;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	/* Check for existing list for 'dev' */
+	dev_opp = find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_warn(dev, "%s: OPP list not found\n", __func__);
+		goto unlock;
+	}
+
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node)
+		count++;
+
+	while (count--) {
+		opp = list_entry_rcu(dev_opp->opp_list.next,
+					struct dev_pm_opp, node);
+		opp_remove_and_free(dev_opp, opp);
+	}
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0330217..13e6956 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -21,7 +21,7 @@ struct dev_pm_opp;
 struct device;
 
 enum dev_pm_opp_event {
-	OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
+	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
 };
 
 #if defined(CONFIG_PM_OPP)
@@ -49,7 +49,11 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
+void dev_pm_opp_remove(struct device *dev, unsigned long freq);
+
 struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+
+void dev_pm_opp_free_opp_table(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -100,11 +104,19 @@ static inline int dev_pm_opp_disable(struct device *dev, unsigned long freq)
 	return 0;
 }
 
+static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq)
+{
+}
+
 static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 							struct device *dev)
 {
 	return ERR_PTR(-EINVAL);
 }
+
+static inline void dev_pm_opp_free_opp_table(struct device *dev)
+{
+}
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
1.8.3.2

Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Viresh Kumar]

On 21 May 2014 11:21, Inderpal Singh <inderpal.s@samsung.com> wrote:
> At the driver unloading time the associated opps and its table may need
> to be deleted. Otherwise it amounts to memory leak. The existing
> OPP library does not have provision to do so.
>
> Hence this patch implements the required functions to free the same.
>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
> Changes in v2:
>         - As suggested by Nishanth, added support to remove a single OPP
>           for non-DT users
>         - Added an internal helper function to avoid the code duplication
>           between opp_remove and free_opp_table functions
>
>  drivers/base/power/opp.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_opp.h   |  14 +++++-
>  2 files changed, 134 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index faae9cf..22adef3 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -89,6 +89,7 @@ struct device_opp {
>         struct device *dev;
>         struct srcu_notifier_head head;
>         struct list_head opp_list;
> +       struct rcu_head head_rcu;

This isn't used ...

>  };
>
>  /*
> @@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>                                 __func__);
>                         return -ENOMEM;
>                 }
> -

Unrelated change.

Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Inderpal Singh]

On Wed, May 21, 2014 at 1:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 21 May 2014 11:21, Inderpal Singh <inderpal.s@samsung.com> wrote:
>> At the driver unloading time the associated opps and its table may need
>> to be deleted. Otherwise it amounts to memory leak. The existing
>> OPP library does not have provision to do so.
>>
>> Hence this patch implements the required functions to free the same.
>>
>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>> ---
>> Changes in v2:
>>         - As suggested by Nishanth, added support to remove a single OPP
>>           for non-DT users
>>         - Added an internal helper function to avoid the code duplication
>>           between opp_remove and free_opp_table functions
>>
>>  drivers/base/power/opp.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/pm_opp.h   |  14 +++++-
>>  2 files changed, 134 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index faae9cf..22adef3 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -89,6 +89,7 @@ struct device_opp {
>>         struct device *dev;
>>         struct srcu_notifier_head head;
>>         struct list_head opp_list;
>> +       struct rcu_head head_rcu;
>
> This isn't used ...

Its being used in opp_remove_and_free function to free dev_opp with kfree_rcu.

>
>>  };
>>
>>  /*
>> @@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>>                                 __func__);
>>                         return -ENOMEM;
>>                 }
>> -
>
> Unrelated change.

Ahhh, missed it. Thanks for pointing.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Viresh Kumar]

On 21 May 2014 13:36, Inderpal Singh <inderpal.s@samsung.com> wrote:
> Its being used in opp_remove_and_free function to free dev_opp with kfree_rcu.

Wasn't aware of how kfree_rcu works :(

Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Nishanth Menon]

On 05/21/2014 12:51 AM, Inderpal Singh wrote:
Apologies on a delayed response.

> At the driver unloading time the associated opps and its table may need
s/opps/OPPs/
> to be deleted. Otherwise it amounts to memory leak. The existing
> OPP library does not have provision to do so.
> 
> Hence this patch implements the required functions to free the same.

I might state the following (based on my naive understanding of b.L
discussions so far):
Original design intent of OPP tables have been to populate OPP table
one time for the device active life time. This held true for older
SoCs where devices like CPUs would not disappear once SoC had booted
up. However, with newer technologies such as ARM b.L, this is no
longer true. .... some explanation why a one time instantiation does
not make sense anymore could be such that the device node itself might
be unregistered from the system depending on SoC activity and the
tables are no longer valid.


> 
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
> Changes in v2:
> 	- As suggested by Nishanth, added support to remove a single OPP
> 	  for non-DT users
> 	- Added an internal helper function to avoid the code duplication
> 	  between opp_remove and free_opp_table functions
>  
>  drivers/base/power/opp.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_opp.h   |  14 +++++-
>  2 files changed, 134 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index faae9cf..22adef3 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -89,6 +89,7 @@ struct device_opp {
>  	struct device *dev;
>  	struct srcu_notifier_head head;
>  	struct list_head opp_list;
> +	struct rcu_head head_rcu;

hmm.. I wonder if we should rename the "head" in struct dev_pm_opp to
head_rcu as well.. kinda better naming (but may be done as a separate
patch) and the "head" for notifier as head_notifier or so.. will make
code readable.

That said - please ensure kernel doc is updated as well.
I now get the following with this patch:
$ ./scripts/kernel-doc drivers/base/power/opp.c>/dev/null
Warning(drivers/base/power/opp.c:93): No description found for
parameter 'head_rcu'


>  };
>  
>  /*
> @@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>  				__func__);
>  			return -ENOMEM;
>  		}
> -
viresh already pointed the spurious change.

>  		dev_opp->dev = dev;
>  		srcu_init_notifier_head(&dev_opp->head);
>  		INIT_LIST_HEAD(&dev_opp->opp_list);
> @@ -464,6 +464,82 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>  EXPORT_SYMBOL_GPL(dev_pm_opp_add);
>  
>  /**
> + * opp_remove_and_free()  - Internal helper to remove and free an OPP
> + * @dev_opp:	device_opp for which we do this operation
> + * @opp:	OPP to be removed
> + *
> + * This function removes an opp from the opp list and frees it. And, if the
> + * list is empty, it also removes the device_opp from the root list and
> + * frees it.
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * The callers need to use RCU updater strategy with mutex locks to keep the
> + * integrity of the internal data structures.
> + */
> +static void opp_remove_and_free(struct device_opp *dev_opp,
> +						struct dev_pm_opp *opp)
> +{
> +	list_del_rcu(&opp->node);
> +	/*
> +	 * Notify the removal of the opp
> +	 */
> +	srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_REMOVE, opp);
Do this before list_del_rcu?

> +	kfree_rcu(opp, head);
> +
> +	if (list_empty(&dev_opp->opp_list)) {
> +		list_del_rcu(&dev_opp->node);
> +		kfree_rcu(dev_opp, head_rcu);
> +	}
> +}
> +
> +/**
> + * dev_pm_opp_remove() - remove a specific OPP
> + * @dev:	device for which we do this operation
> + * @freq:	OPP frequency to remove
> + *
> + * Removes a provided opp.
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks to
> + * keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex locking cannot be used.
> + */
> +void dev_pm_opp_remove(struct device *dev, unsigned long freq)
Debatable: Should'nt we do an int return here? if a frequency not in
the list is attempted to be deleted, bad pointer passed?
Ofcourse, what does the caller do with it? maybe find a logic bug a
print or something to the effect? It might be little hard a few years
down the line to know why we never returned an error value.. just my 2
cents..

> +{
> +	struct device_opp *dev_opp = ERR_PTR(-ENODEV);
not necessary to initialize?

> +	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
> +
> +	if (IS_ERR_OR_NULL(dev))
You don't need this? -> find_device_opp will do required check for
you. Lets leave these checks centralized.

> +		return;
> +
> +	/* Hold our list modification lock here */
> +	mutex_lock(&dev_opp_list_lock);
> +
> +	/* Check for existing list for 'dev' */
> +	dev_opp = find_device_opp(dev);
> +	if (IS_ERR(dev_opp))
> +		goto unlock;
> +
> +	/* Do we have the frequency? */
> +	list_for_each_entry(temp_opp, &dev_opp->opp_list, node)
> +		if (temp_opp->rate == freq) {
> +			opp = temp_opp;
> +			break;
> +		}
> +
> +	if (IS_ERR(opp)) {
> +		dev_warn(dev, "%s: OPP freq not found\n", __func__);
> +		goto unlock;
> +	}
> +
> +	opp_remove_and_free(dev_opp, opp);
> +unlock:
> +	mutex_unlock(&dev_opp_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
> +
> +/**
>   * opp_set_availability() - helper to set the availability of an opp
>   * @dev:		device for which we do this operation
>   * @freq:		OPP frequency to modify availability
> @@ -653,3 +729,47 @@ int of_init_opp_table(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>  #endif
> +
> +/**
> + * dev_pm_opp_free_opp_table() - free the opp table
> + * @dev:	device for which we do this operation
> + *
> + * Free up the allocated opp table
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks to
> + * keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex locking cannot be used.
> + */
> +void dev_pm_opp_free_opp_table(struct device *dev)
I understand this can be a generic function without of dependencies..
but, in general I like symmetry. of_init_opp_table -->
of_free_opp_table (the OPPs added by init is freed here) - and if
platform code such as iMX6/others do opp_add on top of of_init_opp,
they are not impacted. if the platform code did opp_add, then the
platform code has responsibility for doing opp_remove where needed.

Now, this function does more than that - it will go and remove all
OPPs for a device whether added by opp_add or added by of_opp_init.
That will bite us unexpectedly as platform code might have to deal
with that weirdness against driver/generic logic that deals with
of_opp_init. I would rather see an equivalent of_opp_deinit/free
function that handles just the OPPs that were added by the
corresponding of_opp_init and let opp_remove be used by others.

That also means that of_opp_deinit/free should be done as patch #2.

> +{
> +	struct device_opp *dev_opp = ERR_PTR(-ENODEV);
same comment.
> +	struct dev_pm_opp *opp;
> +	int count = 0;
> +
> +	if (IS_ERR_OR_NULL(dev))
> +		return;
Same comment.
> +
> +	/* Hold our list modification lock here */
> +	mutex_lock(&dev_opp_list_lock);
> +
> +	/* Check for existing list for 'dev' */
> +	dev_opp = find_device_opp(dev);
> +	if (IS_ERR(dev_opp)) {
> +		dev_warn(dev, "%s: OPP list not found\n", __func__);
> +		goto unlock;
> +	}
> +
> +	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node)
> +		count++;
> +
> +	while (count--) {
> +		opp = list_entry_rcu(dev_opp->opp_list.next,
> +					struct dev_pm_opp, node);
> +		opp_remove_and_free(dev_opp, opp);

if we use of_, then you have the option of using integrating logic of
of_init_opp_table and of_free_opp_table and just differentiating
opp_add Vs opp_remove
something like http://slexy.org/view/s21rWJzUTm (just a indicative
diff..) - we'd endup with lesser LoC and duplication of logic.

> +	}
> +unlock:
> +	mutex_unlock(&dev_opp_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0330217..13e6956 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -21,7 +21,7 @@ struct dev_pm_opp;
>  struct device;
>  
>  enum dev_pm_opp_event {
> -	OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
> +	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
>  };
>  
>  #if defined(CONFIG_PM_OPP)
> @@ -49,7 +49,11 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
>  
>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>  
> +void dev_pm_opp_remove(struct device *dev, unsigned long freq);
> +
Lets put this next to add?

>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
> +
> +void dev_pm_opp_free_opp_table(struct device *dev);

>  #else
>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>  {
> @@ -100,11 +104,19 @@ static inline int dev_pm_opp_disable(struct device *dev, unsigned long freq)
>  	return 0;
>  }
>  
> +static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq)
> +{
> +}
> +
>  static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
>  							struct device *dev)
>  {
>  	return ERR_PTR(-EINVAL);
>  }
> +
> +static inline void dev_pm_opp_free_opp_table(struct device *dev)
> +{
> +}
>  #endif		/* CONFIG_PM_OPP */
>  
>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
> 


-- 
Regards,
Nishanth Menon