* [PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list
2019-11-13 23:21 [PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
@ 2019-11-13 23:21 ` Leonard Crestez
2019-12-02 1:07 ` Chanwoo Choi
2019-11-13 23:21 ` [PATCH 2/5] PM / devfreq: Split device_register usage Leonard Crestez
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Leonard Crestez @ 2019-11-13 23:21 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
Right now devfreq_dev_release will print a warning and abort the rest of
the cleanup if the devfreq instance is not part of the global
devfreq_list. But this is a valid scenario, for example it can happen if
the governor can't be found or on any other init error that happens
after device_register.
Initialize devfreq->node to an empty list head in devfreq_add_device so
that list_del becomes a safe noop inside devfreq_dev_release and we can
continue the rest of the cleanup.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
drivers/devfreq/devfreq.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 94fb8e821e12..27af1b95fd23 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -635,15 +635,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
static void devfreq_dev_release(struct device *dev)
{
struct devfreq *devfreq = to_devfreq(dev);
mutex_lock(&devfreq_list_lock);
- if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) {
- mutex_unlock(&devfreq_list_lock);
- dev_warn(&devfreq->dev, "releasing devfreq which doesn't exist\n");
- return;
- }
list_del(&devfreq->node);
mutex_unlock(&devfreq_list_lock);
if (devfreq->profile->exit)
devfreq->profile->exit(devfreq->dev.parent);
@@ -694,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
mutex_init(&devfreq->lock);
mutex_lock(&devfreq->lock);
devfreq->dev.parent = dev;
devfreq->dev.class = devfreq_class;
devfreq->dev.release = devfreq_dev_release;
+ INIT_LIST_HEAD(&devfreq->node);
devfreq->profile = profile;
strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
devfreq->previous_freq = profile->initial_freq;
devfreq->last_status.current_frequency = profile->initial_freq;
devfreq->data = data;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list
2019-11-13 23:21 ` [PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list Leonard Crestez
@ 2019-12-02 1:07 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2019-12-02 1:07 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
On 11/14/19 8:21 AM, Leonard Crestez wrote:
> Right now devfreq_dev_release will print a warning and abort the rest of
> the cleanup if the devfreq instance is not part of the global
> devfreq_list. But this is a valid scenario, for example it can happen if
> the governor can't be found or on any other init error that happens
> after device_register.
>
> Initialize devfreq->node to an empty list head in devfreq_add_device so
> that list_del becomes a safe noop inside devfreq_dev_release and we can
> continue the rest of the cleanup.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
> drivers/devfreq/devfreq.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 94fb8e821e12..27af1b95fd23 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -635,15 +635,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
> static void devfreq_dev_release(struct device *dev)
> {
> struct devfreq *devfreq = to_devfreq(dev);
>
> mutex_lock(&devfreq_list_lock);
> - if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) {
> - mutex_unlock(&devfreq_list_lock);
> - dev_warn(&devfreq->dev, "releasing devfreq which doesn't exist\n");
> - return;
> - }
> list_del(&devfreq->node);
> mutex_unlock(&devfreq_list_lock);
>
> if (devfreq->profile->exit)
> devfreq->profile->exit(devfreq->dev.parent);
> @@ -694,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
> mutex_init(&devfreq->lock);
> mutex_lock(&devfreq->lock);
> devfreq->dev.parent = dev;
> devfreq->dev.class = devfreq_class;
> devfreq->dev.release = devfreq_dev_release;
> + INIT_LIST_HEAD(&devfreq->node);
> devfreq->profile = profile;
> strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
> devfreq->previous_freq = profile->initial_freq;
> devfreq->last_status.current_frequency = profile->initial_freq;
> devfreq->data = data;
>
Applied it.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/5] PM / devfreq: Split device_register usage
2019-11-13 23:21 [PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
2019-11-13 23:21 ` [PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list Leonard Crestez
@ 2019-11-13 23:21 ` Leonard Crestez
2019-12-02 1:08 ` Chanwoo Choi
2019-11-13 23:21 ` [PATCH 3/5] PM / devfreq: Move more initialization before registration Leonard Crestez
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Leonard Crestez @ 2019-11-13 23:21 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
Splitting device_register into device_initialize and device_add allows
devm-based allocations to be performed before device_add.
It also simplifies error paths in devfreq_add_device: just call
put_device instead of duplicating parts of devfreq_dev_release.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
drivers/devfreq/devfreq.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 27af1b95fd23..b89a82382536 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
mutex_init(&devfreq->lock);
mutex_lock(&devfreq->lock);
devfreq->dev.parent = dev;
devfreq->dev.class = devfreq_class;
devfreq->dev.release = devfreq_dev_release;
+ device_initialize(&devfreq->dev);
INIT_LIST_HEAD(&devfreq->node);
devfreq->profile = profile;
strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
devfreq->previous_freq = profile->initial_freq;
devfreq->last_status.current_frequency = profile->initial_freq;
@@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
atomic_set(&devfreq->suspend_count, 0);
dev_set_name(&devfreq->dev, "devfreq%d",
atomic_inc_return(&devfreq_no));
- err = device_register(&devfreq->dev);
+ err = device_add(&devfreq->dev);
if (err) {
mutex_unlock(&devfreq->lock);
- put_device(&devfreq->dev);
- goto err_out;
+ goto err_dev;
}
devfreq->trans_table = devm_kzalloc(&devfreq->dev,
array3_size(sizeof(unsigned int),
devfreq->profile->max_state,
@@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev,
err_init:
mutex_unlock(&devfreq_list_lock);
err_devfreq:
devfreq_remove_device(devfreq);
- devfreq = NULL;
+ return ERR_PTR(err);
err_dev:
- kfree(devfreq);
+ put_device(&devfreq->dev);
err_out:
return ERR_PTR(err);
}
EXPORT_SYMBOL(devfreq_add_device);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/5] PM / devfreq: Split device_register usage
2019-11-13 23:21 ` [PATCH 2/5] PM / devfreq: Split device_register usage Leonard Crestez
@ 2019-12-02 1:08 ` Chanwoo Choi
2019-12-02 4:45 ` Leonard Crestez
0 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2019-12-02 1:08 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
On 11/14/19 8:21 AM, Leonard Crestez wrote:
> Splitting device_register into device_initialize and device_add allows
> devm-based allocations to be performed before device_add.
>
> It also simplifies error paths in devfreq_add_device: just call
> put_device instead of duplicating parts of devfreq_dev_release.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> drivers/devfreq/devfreq.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 27af1b95fd23..b89a82382536 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
> mutex_init(&devfreq->lock);
> mutex_lock(&devfreq->lock);
> devfreq->dev.parent = dev;
> devfreq->dev.class = devfreq_class;
> devfreq->dev.release = devfreq_dev_release;
> + device_initialize(&devfreq->dev);
> INIT_LIST_HEAD(&devfreq->node);
> devfreq->profile = profile;
> strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
> devfreq->previous_freq = profile->initial_freq;
> devfreq->last_status.current_frequency = profile->initial_freq;
> @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> atomic_set(&devfreq->suspend_count, 0);
>
> dev_set_name(&devfreq->dev, "devfreq%d",
> atomic_inc_return(&devfreq_no));
> - err = device_register(&devfreq->dev);
> + err = device_add(&devfreq->dev);
> if (err) {
> mutex_unlock(&devfreq->lock);
> - put_device(&devfreq->dev);
> - goto err_out;
> + goto err_dev;
> }
>
> devfreq->trans_table = devm_kzalloc(&devfreq->dev,
> array3_size(sizeof(unsigned int),
> devfreq->profile->max_state,
> @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev,
>
> err_init:
> mutex_unlock(&devfreq_list_lock);
> err_devfreq:
> devfreq_remove_device(devfreq);
> - devfreq = NULL;
> + return ERR_PTR(err);
> err_dev:
> - kfree(devfreq);
> + put_device(&devfreq->dev);
> err_out:
> return ERR_PTR(err);
> }
> EXPORT_SYMBOL(devfreq_add_device);
>
>
As I previously commented, I don't prefer to split out of bodyf of device_register().
Instead, your first version is better without devm.
Regards,
Chanwoo Choi
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/5] PM / devfreq: Split device_register usage
2019-12-02 1:08 ` Chanwoo Choi
@ 2019-12-02 4:45 ` Leonard Crestez
2019-12-02 5:02 ` Chanwoo Choi
0 siblings, 1 reply; 10+ messages in thread
From: Leonard Crestez @ 2019-12-02 4:45 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
dl-linux-imx
On 2019-12-02 3:02 AM, Chanwoo Choi wrote:
> On 11/14/19 8:21 AM, Leonard Crestez wrote:
>> Splitting device_register into device_initialize and device_add allows
>> devm-based allocations to be performed before device_add.
>>
>> It also simplifies error paths in devfreq_add_device: just call
>> put_device instead of duplicating parts of devfreq_dev_release.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>> drivers/devfreq/devfreq.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 27af1b95fd23..b89a82382536 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> mutex_init(&devfreq->lock);
>> mutex_lock(&devfreq->lock);
>> devfreq->dev.parent = dev;
>> devfreq->dev.class = devfreq_class;
>> devfreq->dev.release = devfreq_dev_release;
>> + device_initialize(&devfreq->dev);
>> INIT_LIST_HEAD(&devfreq->node);
>> devfreq->profile = profile;
>> strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>> devfreq->previous_freq = profile->initial_freq;
>> devfreq->last_status.current_frequency = profile->initial_freq;
>> @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>> atomic_set(&devfreq->suspend_count, 0);
>>
>> dev_set_name(&devfreq->dev, "devfreq%d",
>> atomic_inc_return(&devfreq_no));
>> - err = device_register(&devfreq->dev);
>> + err = device_add(&devfreq->dev);
>> if (err) {
>> mutex_unlock(&devfreq->lock);
>> - put_device(&devfreq->dev);
>> - goto err_out;
>> + goto err_dev;
>> }
>>
>> devfreq->trans_table = devm_kzalloc(&devfreq->dev,
>> array3_size(sizeof(unsigned int),
>> devfreq->profile->max_state,
>> @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>
>> err_init:
>> mutex_unlock(&devfreq_list_lock);
>> err_devfreq:
>> devfreq_remove_device(devfreq);
>> - devfreq = NULL;
>> + return ERR_PTR(err);
>> err_dev:
>> - kfree(devfreq);
>> + put_device(&devfreq->dev);
>> err_out:
>> return ERR_PTR(err);
>> }
>> EXPORT_SYMBOL(devfreq_add_device);
>>
>>
>
> As I previously commented, I don't prefer to split out of bodyf of device_register().
> Instead, your first version is better without devm.
Very well, feel free to drop 2-5 of this series then.
Or perhaps I misunderstood and the locking cleanups would be acceptable
in the variant that removes devm from a few allocations? There's quite a
bunch of stuff flying around the merge window already so I'll refrain
from posting until v5.5-rc1 anyway.
I went a little overboard with tricky cleanups and this ended up
delaying the functionality I wanted to push.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/5] PM / devfreq: Split device_register usage
2019-12-02 4:45 ` Leonard Crestez
@ 2019-12-02 5:02 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2019-12-02 5:02 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
dl-linux-imx
On 12/2/19 1:45 PM, Leonard Crestez wrote:
> On 2019-12-02 3:02 AM, Chanwoo Choi wrote:
>> On 11/14/19 8:21 AM, Leonard Crestez wrote:
>>> Splitting device_register into device_initialize and device_add allows
>>> devm-based allocations to be performed before device_add.
>>>
>>> It also simplifies error paths in devfreq_add_device: just call
>>> put_device instead of duplicating parts of devfreq_dev_release.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>> drivers/devfreq/devfreq.c | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 27af1b95fd23..b89a82382536 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>> mutex_init(&devfreq->lock);
>>> mutex_lock(&devfreq->lock);
>>> devfreq->dev.parent = dev;
>>> devfreq->dev.class = devfreq_class;
>>> devfreq->dev.release = devfreq_dev_release;
>>> + device_initialize(&devfreq->dev);
>>> INIT_LIST_HEAD(&devfreq->node);
>>> devfreq->profile = profile;
>>> strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>>> devfreq->previous_freq = profile->initial_freq;
>>> devfreq->last_status.current_frequency = profile->initial_freq;
>>> @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>>> atomic_set(&devfreq->suspend_count, 0);
>>>
>>> dev_set_name(&devfreq->dev, "devfreq%d",
>>> atomic_inc_return(&devfreq_no));
>>> - err = device_register(&devfreq->dev);
>>> + err = device_add(&devfreq->dev);
>>> if (err) {
>>> mutex_unlock(&devfreq->lock);
>>> - put_device(&devfreq->dev);
>>> - goto err_out;
>>> + goto err_dev;
>>> }
>>>
>>> devfreq->trans_table = devm_kzalloc(&devfreq->dev,
>>> array3_size(sizeof(unsigned int),
>>> devfreq->profile->max_state,
>>> @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>
>>> err_init:
>>> mutex_unlock(&devfreq_list_lock);
>>> err_devfreq:
>>> devfreq_remove_device(devfreq);
>>> - devfreq = NULL;
>>> + return ERR_PTR(err);
>>> err_dev:
>>> - kfree(devfreq);
>>> + put_device(&devfreq->dev);
>>> err_out:
>>> return ERR_PTR(err);
>>> }
>>> EXPORT_SYMBOL(devfreq_add_device);
>>>
>>>
>>
>> As I previously commented, I don't prefer to split out of bodyf of device_register().
>> Instead, your first version is better without devm.
>
> Very well, feel free to drop 2-5 of this series then.
>
> Or perhaps I misunderstood and the locking cleanups would be acceptable
> in the variant that removes devm from a few allocations? There's quite a
> bunch of stuff flying around the merge window already so I'll refrain
> from posting until v5.5-rc1 anyway.
Don't need to wait the v5.5-rc1. You can send the patches.
But, This series have to be merged to v5.6-rc1.
>
> I went a little overboard with tricky cleanups and this ended up
> delaying the functionality I wanted to push.
>
> --
> Regards,
> Leonard
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/5] PM / devfreq: Move more initialization before registration
2019-11-13 23:21 [PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
2019-11-13 23:21 ` [PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list Leonard Crestez
2019-11-13 23:21 ` [PATCH 2/5] PM / devfreq: Split device_register usage Leonard Crestez
@ 2019-11-13 23:21 ` Leonard Crestez
2019-11-13 23:21 ` [PATCH 4/5] PM / devfreq: Don't use devm on parent device Leonard Crestez
2019-11-13 23:21 ` [PATCH 5/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
4 siblings, 0 replies; 10+ messages in thread
From: Leonard Crestez @ 2019-11-13 23:21 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
In general it is a better to initialize an object before making it
accessible externally (through device_add). This simplifies the code and
makes it possible to avoid locking the partially initialized object.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
drivers/devfreq/devfreq.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b89a82382536..b38e98853fda 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -725,43 +725,43 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->max_freq = devfreq->scaling_max_freq;
devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
atomic_set(&devfreq->suspend_count, 0);
- dev_set_name(&devfreq->dev, "devfreq%d",
- atomic_inc_return(&devfreq_no));
- err = device_add(&devfreq->dev);
- if (err) {
- mutex_unlock(&devfreq->lock);
- goto err_dev;
- }
-
devfreq->trans_table = devm_kzalloc(&devfreq->dev,
array3_size(sizeof(unsigned int),
devfreq->profile->max_state,
devfreq->profile->max_state),
GFP_KERNEL);
if (!devfreq->trans_table) {
mutex_unlock(&devfreq->lock);
err = -ENOMEM;
- goto err_devfreq;
+ goto err_dev;
}
devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
devfreq->profile->max_state,
sizeof(unsigned long),
GFP_KERNEL);
if (!devfreq->time_in_state) {
mutex_unlock(&devfreq->lock);
err = -ENOMEM;
- goto err_devfreq;
+ goto err_dev;
}
devfreq->last_stat_updated = jiffies;
srcu_init_notifier_head(&devfreq->transition_notifier_list);
+ dev_set_name(&devfreq->dev, "devfreq%d",
+ atomic_inc_return(&devfreq_no));
+ err = device_add(&devfreq->dev);
+ if (err) {
+ mutex_unlock(&devfreq->lock);
+ goto err_dev;
+ }
+
mutex_unlock(&devfreq->lock);
mutex_lock(&devfreq_list_lock);
governor = try_then_request_governor(devfreq->governor_name);
@@ -787,11 +787,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
return devfreq;
err_init:
mutex_unlock(&devfreq_list_lock);
-err_devfreq:
devfreq_remove_device(devfreq);
return ERR_PTR(err);
err_dev:
put_device(&devfreq->dev);
err_out:
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/5] PM / devfreq: Don't use devm on parent device
2019-11-13 23:21 [PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
` (2 preceding siblings ...)
2019-11-13 23:21 ` [PATCH 3/5] PM / devfreq: Move more initialization before registration Leonard Crestez
@ 2019-11-13 23:21 ` Leonard Crestez
2019-11-13 23:21 ` [PATCH 5/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
4 siblings, 0 replies; 10+ messages in thread
From: Leonard Crestez @ 2019-11-13 23:21 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
In theory a driver can call devfreq_add_device, get an error in return
and still probe succesfuly. If this happens the freq_table allocated by
set_freq_table is effectively leaked.
Now that device_initialize is called early inside devfreq_add_device we
can use devm on devfreq->dev inside set_freq_table instead. Since that's
always freed if devfreq_add_device fails there is no need to devm_kfree
on any path.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
drivers/devfreq/devfreq.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b38e98853fda..2a035374ae74 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -166,11 +166,11 @@ static int set_freq_table(struct devfreq *devfreq)
count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
if (count <= 0)
return -EINVAL;
profile->max_state = count;
- profile->freq_table = devm_kcalloc(devfreq->dev.parent,
+ profile->freq_table = devm_kcalloc(&devfreq->dev,
profile->max_state,
sizeof(*profile->freq_table),
GFP_KERNEL);
if (!profile->freq_table) {
profile->max_state = 0;
@@ -178,11 +178,10 @@ static int set_freq_table(struct devfreq *devfreq)
}
for (i = 0, freq = 0; i < profile->max_state; i++, freq++) {
opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq);
if (IS_ERR(opp)) {
- devm_kfree(devfreq->dev.parent, profile->freq_table);
profile->max_state = 0;
return PTR_ERR(opp);
}
dev_pm_opp_put(opp);
profile->freq_table[i] = freq;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/5] PM / devfreq: Don't take lock in devfreq_add_device
2019-11-13 23:21 [PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
` (3 preceding siblings ...)
2019-11-13 23:21 ` [PATCH 4/5] PM / devfreq: Don't use devm on parent device Leonard Crestez
@ 2019-11-13 23:21 ` Leonard Crestez
4 siblings, 0 replies; 10+ messages in thread
From: Leonard Crestez @ 2019-11-13 23:21 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Kyungmin Park, Rafael J. Wysocki, Matthias Kaehlcke,
Artur Świgoń,
Georgi Djakov, Viresh Kumar, linux-pm, linux-arm-kernel,
linux-imx
A device usually doesn't need to lock itself during initialization
because it is not yet reachable from other threads.
This simplifies the code and helps avoid recursive lock warnings.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
drivers/devfreq/devfreq.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2a035374ae74..e1ce57902391 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -684,11 +684,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
err = -ENOMEM;
goto err_out;
}
mutex_init(&devfreq->lock);
- mutex_lock(&devfreq->lock);
devfreq->dev.parent = dev;
devfreq->dev.class = devfreq_class;
devfreq->dev.release = devfreq_dev_release;
device_initialize(&devfreq->dev);
INIT_LIST_HEAD(&devfreq->node);
@@ -698,28 +697,24 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->last_status.current_frequency = profile->initial_freq;
devfreq->data = data;
devfreq->nb.notifier_call = devfreq_notifier_call;
if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
- mutex_unlock(&devfreq->lock);
err = set_freq_table(devfreq);
if (err < 0)
goto err_dev;
- mutex_lock(&devfreq->lock);
}
devfreq->scaling_min_freq = find_available_min_freq(devfreq);
if (!devfreq->scaling_min_freq) {
- mutex_unlock(&devfreq->lock);
err = -EINVAL;
goto err_dev;
}
devfreq->min_freq = devfreq->scaling_min_freq;
devfreq->scaling_max_freq = find_available_max_freq(devfreq);
if (!devfreq->scaling_max_freq) {
- mutex_unlock(&devfreq->lock);
err = -EINVAL;
goto err_dev;
}
devfreq->max_freq = devfreq->scaling_max_freq;
@@ -730,21 +725,19 @@ struct devfreq *devfreq_add_device(struct device *dev,
array3_size(sizeof(unsigned int),
devfreq->profile->max_state,
devfreq->profile->max_state),
GFP_KERNEL);
if (!devfreq->trans_table) {
- mutex_unlock(&devfreq->lock);
err = -ENOMEM;
goto err_dev;
}
devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
devfreq->profile->max_state,
sizeof(unsigned long),
GFP_KERNEL);
if (!devfreq->time_in_state) {
- mutex_unlock(&devfreq->lock);
err = -ENOMEM;
goto err_dev;
}
devfreq->last_stat_updated = jiffies;
@@ -752,16 +745,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
srcu_init_notifier_head(&devfreq->transition_notifier_list);
dev_set_name(&devfreq->dev, "devfreq%d",
atomic_inc_return(&devfreq_no));
err = device_add(&devfreq->dev);
- if (err) {
- mutex_unlock(&devfreq->lock);
+ if (err)
goto err_dev;
- }
-
- mutex_unlock(&devfreq->lock);
mutex_lock(&devfreq_list_lock);
governor = try_then_request_governor(devfreq->governor_name);
if (IS_ERR(governor)) {
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread