Somehow one of the instance of freeing resources failed to use kfree_rcu() and used kfree() instead. This might cause problems as the node might be referenced by readers under rcu locks and we must wait for the rcu grace period as well.
While we are at it, also update comment over 'struct device_opp' to mention why we are waiting for both rcu and srcu grace periods.
Fixes: 129eec55df6a ("PM / OPP Introduce APIs to remove OPPs") Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Hi Rafael,
Few more updates for the opp layer. First one is a potential bug fix and rest are cleanups.
drivers/base/power/opp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 1bbef8e838e7..e1807268cbf2 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -84,7 +84,11 @@ struct dev_pm_opp { * * This is an internal data structure maintaining the link to opps attached to * a device. This structure is not meant to be shared to users as it is - * meant for book keeping and private to OPP library + * meant for book keeping and private to OPP library. + * + * Because the opp structures can be used from both rcu and srcu readers, we + * need to wait for the grace period of both of them before freeing any + * resources. And so we have used kfree_rcu() from within call_srcu() handlers. */ struct device_opp { struct list_head node; @@ -511,7 +515,7 @@ static void kfree_device_rcu(struct rcu_head *head) { struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
- kfree(device_opp); + kfree_rcu(device_opp, rcu_head); }
void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp)
Its a local routine and must not be accessible outside of opp.c.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index e1807268cbf2..fa065d6e1731 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -518,7 +518,8 @@ static void kfree_device_rcu(struct rcu_head *head) kfree_rcu(device_opp, rcu_head); }
-void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +static void __dev_pm_opp_remove(struct device_opp *dev_opp, + struct dev_pm_opp *opp) { /* * Notify the changes in the availability of the operable
Reuse find_device_opp() in opp_set_availability() instead of duplicating code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index fa065d6e1731..525ffb202d77 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -597,7 +597,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove); static int opp_set_availability(struct device *dev, unsigned long freq, bool availability_req) { - struct device_opp *tmp_dev_opp, *dev_opp = ERR_PTR(-ENODEV); + struct device_opp *dev_opp; struct dev_pm_opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV); int r = 0;
@@ -611,12 +611,7 @@ static int opp_set_availability(struct device *dev, unsigned long freq, mutex_lock(&dev_opp_list_lock);
/* Find the device_opp */ - list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) { - if (dev == tmp_dev_opp->dev) { - dev_opp = tmp_dev_opp; - break; - } - } + dev_opp = find_device_opp(dev); if (IS_ERR(dev_opp)) { r = PTR_ERR(dev_opp); dev_warn(dev, "%s: Device OPP not found (%d)\n", __func__, r);
Get the 'device_opp' allocation code into a separate routine to keep only the necessary part in dev_pm_opp_add_dynamic().
Also do s/sizeof(struct device_opp)/sizeof(*dev_opp) and remove the print message on kzalloc() failure as checkpatch warns for that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 525ffb202d77..1150b9d2e012 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -386,6 +386,27 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, } EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
+static struct device_opp *add_device_opp(struct device *dev) +{ + struct device_opp *dev_opp; + + /* + * Allocate a new device OPP table. In the infrequent case where a new + * device is needed to be added, we pay this penalty. + */ + dev_opp = kzalloc(sizeof(*dev_opp), GFP_KERNEL); + if (!dev_opp) + return NULL; + + dev_opp->dev = dev; + srcu_init_notifier_head(&dev_opp->srcu_head); + INIT_LIST_HEAD(&dev_opp->opp_list); + + /* Secure the device list modification */ + list_add_rcu(&dev_opp->node, &dev_opp_list); + return dev_opp; +} + static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, unsigned long u_volt, bool dynamic) { @@ -412,27 +433,13 @@ static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, /* Check for existing list for 'dev' */ dev_opp = find_device_opp(dev); if (IS_ERR(dev_opp)) { - /* - * Allocate a new device OPP table. In the infrequent case - * where a new device is needed to be added, we pay this - * penalty. - */ - dev_opp = kzalloc(sizeof(struct device_opp), GFP_KERNEL); + dev_opp = add_device_opp(dev); if (!dev_opp) { mutex_unlock(&dev_opp_list_lock); kfree(new_opp); - dev_warn(dev, - "%s: Unable to create device OPP structure\n", - __func__); return -ENOMEM; }
- dev_opp->dev = dev; - srcu_init_notifier_head(&dev_opp->srcu_head); - INIT_LIST_HEAD(&dev_opp->opp_list); - - /* Secure the device list modification */ - list_add_rcu(&dev_opp->node, &dev_opp_list); head = &dev_opp->opp_list; goto list_add; }
On Wednesday, December 10, 2014 09:45:34 AM Viresh Kumar wrote:
Get the 'device_opp' allocation code into a separate routine to keep only the necessary part in dev_pm_opp_add_dynamic().
Also do s/sizeof(struct device_opp)/sizeof(*dev_opp) and remove the print message on kzalloc() failure as checkpatch warns for that.
I've lost patches [1-2/5] somehow, but never mind. I can get them from Patchwork.
In this one I'm wondering about the printk removal mentioned above. Did checkpatch complain about the printk?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/base/power/opp.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 525ffb202d77..1150b9d2e012 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -386,6 +386,27 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, } EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor); +static struct device_opp *add_device_opp(struct device *dev) +{
- struct device_opp *dev_opp;
- /*
* Allocate a new device OPP table. In the infrequent case where a new* device is needed to be added, we pay this penalty.*/- dev_opp = kzalloc(sizeof(*dev_opp), GFP_KERNEL);
- if (!dev_opp)
return NULL;- dev_opp->dev = dev;
- srcu_init_notifier_head(&dev_opp->srcu_head);
- INIT_LIST_HEAD(&dev_opp->opp_list);
- /* Secure the device list modification */
- list_add_rcu(&dev_opp->node, &dev_opp_list);
- return dev_opp;
+}
static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, unsigned long u_volt, bool dynamic) { @@ -412,27 +433,13 @@ static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, /* Check for existing list for 'dev' */ dev_opp = find_device_opp(dev); if (IS_ERR(dev_opp)) {
/** Allocate a new device OPP table. In the infrequent case* where a new device is needed to be added, we pay this* penalty.*/dev_opp = kzalloc(sizeof(struct device_opp), GFP_KERNEL);
dev_opp = add_device_opp(dev);
dev_warn(dev,"%s: Unable to create device OPP structure\n",__func__); return -ENOMEM;
dev_opp->dev = dev;srcu_init_notifier_head(&dev_opp->srcu_head);INIT_LIST_HEAD(&dev_opp->opp_list);/* Secure the device list modification */list_add_rcu(&dev_opp->node, &dev_opp_list);
On 11 December 2014 at 03:31, Rafael J. Wysocki rjw@rjwysocki.net wrote:
I've lost patches [1-2/5] somehow, but never mind. I can get them from Patchwork.
Strange, you were always in --to for them ..
In this one I'm wondering about the printk removal mentioned above. Did checkpatch complain about the printk?
commit ebfdc40969f24fc0cdd1349835d36e8ebae05374 Author: Joe Perches joe@perches.com Date: Wed Aug 6 16:10:27 2014 -0700
checkpatch: attempt to find unnecessary 'out of memory' messages
Logging messages that show some type of "out of memory" error are generally unnecessary as there is a generic message and a stack dump done by the memory subsystem.
These messages generally increase kernel size without much added value.
Emit a warning on these types of messages.
This test looks for any inserted message function, then looks at the previous line for an "if (!foo)" or "if (foo == NULL)" test and then looks at the preceding statement for an allocation function like "foo = kmalloc()"
ie: this code matches:
foo = kmalloc(); if (foo == NULL) { printk("Out of memory\n"); return -ENOMEM; }
This test is very crude and incomplete.
This test can miss quite a lot of of OOM messages that do not have this specific form.
ie: this code does not match:
foo = kmalloc(); if (!foo) { rtn = -ENOMEM; printk("Out of memory!\n"); goto out; }
This test could also be a false positive when the logging message itself does not specify anything about memory, but I did not find any false positives in my limited testing.
spatch could be a better solution but correctness seems non-trivial for that tool too.
On Thursday, December 11, 2014 08:45:22 AM Viresh Kumar wrote:
On 11 December 2014 at 03:31, Rafael J. Wysocki rjw@rjwysocki.net wrote:
I've lost patches [1-2/5] somehow, but never mind. I can get them from Patchwork.
Strange, you were always in --to for them ..
In this one I'm wondering about the printk removal mentioned above. Did checkpatch complain about the printk?
commit ebfdc40969f24fc0cdd1349835d36e8ebae05374 Author: Joe Perches joe@perches.com Date: Wed Aug 6 16:10:27 2014 -0700
checkpatch: attempt to find unnecessary 'out of memory' messages
OK
So I've applied this, but here are the rules for the future regarding checkpatch.pl:
(1) checkpatch.pl is meant for checking changes made by a patch and *not* for checking the existing code.
(2) Always use common sense on top of checkpatch.pl complaints.
On 12 December 2014 at 02:22, Rafael J. Wysocki rjw@rjwysocki.net wrote:
OK
So I've applied this, but here are the rules for the future regarding checkpatch.pl:
(1) checkpatch.pl is meant for checking changes made by a patch and *not* for checking the existing code.
(2) Always use common sense on top of checkpatch.pl complaints.
Correct, normally I do ignore the warnings for cases which I don't care about. I did react to this one because there have been patches going around removing print messages on allocation failures. And I didn't wanted to get another one for this and so removed it in my patch only..
-- viresh
This makes it less error prone and moves common resource deallocation at a single place.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 1150b9d2e012..d24dd614a0bd 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -413,6 +413,7 @@ static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, struct device_opp *dev_opp = NULL; struct dev_pm_opp *opp, *new_opp; struct list_head *head; + int ret;
/* allocate new OPP node */ new_opp = kzalloc(sizeof(*new_opp), GFP_KERNEL); @@ -435,9 +436,8 @@ static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, if (IS_ERR(dev_opp)) { dev_opp = add_device_opp(dev); if (!dev_opp) { - mutex_unlock(&dev_opp_list_lock); - kfree(new_opp); - return -ENOMEM; + ret = -ENOMEM; + goto free_opp; }
head = &dev_opp->opp_list; @@ -458,15 +458,13 @@ static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq,
/* Duplicate OPPs ? */ if (new_opp->rate == opp->rate) { - int ret = opp->available && new_opp->u_volt == opp->u_volt ? + ret = opp->available && new_opp->u_volt == opp->u_volt ? 0 : -EEXIST;
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", __func__, opp->rate, opp->u_volt, opp->available, new_opp->rate, new_opp->u_volt, new_opp->available); - mutex_unlock(&dev_opp_list_lock); - kfree(new_opp); - return ret; + goto free_opp; }
list_add: @@ -480,6 +478,11 @@ static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, */ srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); return 0; + +free_opp: + mutex_unlock(&dev_opp_list_lock); + kfree(new_opp); + return ret; }
/**
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Viresh Kumar