Re: [PATCH 3/8] PM / devfreq: Move more initialization before registration

[Leonard Crestez <leonard.crestez@nxp.com>]

On 19.09.2019 02:29, Matthias Kaehlcke wrote:
> Hi Leonard,
> 
> On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote:
>> In general it is a better to initialize an object before making it
>> accessible externally (through device_register).
>>
>> This make it possible to avoid relying on locking a partially
>> initialized object.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index a715f27f35fd..57a217fc92de 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev)
>>   
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>>   	mutex_destroy(&devfreq->lock);
>> +	kfree(devfreq->time_in_state);
>> +	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   }
>>   
>>   /**
>>    * devfreq_add_device() - Add devfreq feature to the device
>> @@ -671,44 +673,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_register(&devfreq->dev);
>> -	if (err) {
>> -		mutex_unlock(&devfreq->lock);
>> -		put_device(&devfreq->dev);
>> -		goto err_out;
>> -	}
>> -
>> -	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
>> +	devfreq->trans_table = kzalloc(
>>   			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);
>> +	devfreq->time_in_state = kcalloc(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_register(&devfreq->dev);
>> +	if (err) {
>> +		mutex_unlock(&devfreq->lock);
>> +		put_device(&devfreq->dev);
>> +		goto err_out;
> 
>    		goto err_dev;
> 
>> +	}
>> +
>>   	mutex_unlock(&devfreq->lock);
>>   
>>   	mutex_lock(&devfreq_list_lock);
>>   
>>   	governor = try_then_request_governor(devfreq->governor_name);
>> @@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   
>>   	return devfreq;
>>   
>>   err_init:
>>   	mutex_unlock(&devfreq_list_lock);
>> -err_devfreq:
>>   	devfreq_remove_device(devfreq);
>> -	devfreq = NULL;
>> +	return ERR_PTR(err);
> 
> The two return paths in the unwind part are unorthodox, but I
> see why they are needed. Maybe add an empty line between the two paths
> to make it a bit more evident that they are separate.

Old code did "devfreq = NULL" just so that the later kfree did nothing. 
There were already two unwind paths, it's just that the second one was 
less obvious. I will add a comment.

>>   err_dev:
> 
> This code path should include
> 
> 	mutex_destroy(&devfreq->lock);
> 
> That was already missing in the original code though.

Yes, that would be a separate patch.

> Actually with the later device registration the mutex could be
> initialized later and doesn't need to be held. This would
> obsolete the mutex_unlock() calls in the error paths
Next patch already removes mutex_lock before device_register (that's the 
purpose of the cleanup). If you're suggesting to move mutex_init around 
it's not clear what would be gained?

--
Regards,
Leonard