Re: [PATCH 2/5] PM / devfreq: Split device_register usage

From: Leonard Crestez <leonard.crestez@nxp.com>
To: Chanwoo Choi <cw00.choi@samsung.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: "Kyungmin Park" <kyungmin.park@samsung.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Matthias Kaehlcke" <mka@chromium.org>,
	"Artur Świgoń" <a.swigon@partner.samsung.com>,
	"Georgi Djakov" <georgi.djakov@linaro.org>,
	"Viresh Kumar" <viresh.kumar@linaro.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH 2/5] PM / devfreq: Split device_register usage
Date: Mon, 2 Dec 2019 04:45:16 +0000	[thread overview]
Message-ID: <VI1PR04MB7023135B9D53D86A101F1895EE430@VI1PR04MB7023.eurprd04.prod.outlook.com> (raw)
In-Reply-To: 69d37137-8c7a-ddcc-f692-09546c79ff7d@samsung.com

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