[2/5] PM / devfreq: Split device_register usage
diff mbox series

Message ID 85ccf6afe5db556c610ce2b47ccc38132b6671f6.1573686315.git.leonard.crestez@nxp.com
State New
Headers show
Series
  • PM / devfreq: Don't take lock in devfreq_add_device
Related show

Commit Message

Leonard Crestez Nov. 13, 2019, 11:21 p.m. UTC
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(-)

Comments

Chanwoo Choi Dec. 2, 2019, 1:08 a.m. UTC | #1
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
Leonard Crestez Dec. 2, 2019, 4:45 a.m. UTC | #2
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
Chanwoo Choi Dec. 2, 2019, 5:02 a.m. UTC | #3
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
> 
>

Patch
diff mbox series

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);