Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions
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
> ---
> 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 *t
Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions
On 21 May 2014 13:36, Inderpal Singh 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 :( -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions
On Wed, May 21, 2014 at 1:25 PM, Viresh Kumar wrote:
> On 21 May 2014 11:21, Inderpal Singh 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
>> ---
>> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions
On 21 May 2014 11:21, Inderpal Singh 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
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions
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
---
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)
+{
+
