[PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device

[PATCH 0/5] PM / devfreq: Don't take lock in devfreq_add_device

[Leonard Crestez]

Right now the devfreq_add_device takes devfreq->lock as soon as the device is
allocated, this is awkward and unnecessary. In general an object should be
initialized in isolation and only be made available to the system when it's in
a consistent state.

Locking the device during initialization causes problems (lockdep warnings)
when interacting with other subsystems that also use heavy locking. There are
also a few fields (such as trans_table) which are initialized after
device_register even through they're readable from sysfs, these are now
allocated earlier.

This was spawned by the attempt to add dev_pm_qos support. I split this series
here because it might benefit from separate discussion.

Notifiers from pm_qos are executed under a single global dev_pm_qos_mtx and
need to take devfreq->lock, this means that in order to prevent possible
deadlocks all initialization of dev_pm_qos must be performed outside the
devfreq->lock.

PM QoS requests from sysfs should be initialized before the device is
registered (because they're touched from sysfs) but all of that is currently
done with devfreq->lock held!

This series makes some tricky changes but the end results is easier to
understand and maintain. For example it removes a scary unlock/lock pair
around set_freq_table, maybe this also caused problems in the past?

Alternative solutions exist: all PM QoS setup could be done after device_add
just like governor setup and the min/max_freq_store could return an error if
the qos request is not yet properly initialized.

It might even be possible to modify dev_pm_qos to call notifiers without
holding internal locks? That is generally a good idea for all notifiers.

Changes since split from PM_QOS:
* Add a patch which moves devm from devfreq->dev->parent to devfreq->dev.
See: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=196443

Leonard Crestez (5):
  PM / devfreq: Don't fail devfreq_dev_release if not in list
  PM / devfreq: Split device_register usage
  PM / devfreq: Move more initialization before registration
  PM / devfreq: Don't use devm on parent device
  PM / devfreq: Don't take lock in devfreq_add_device

 drivers/devfreq/devfreq.c | 41 ++++++++++++---------------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

-- 
2.17.1

[PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list

[Leonard Crestez]

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

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

[Leonard Crestez]

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

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

[Leonard Crestez]

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

[PATCH 4/5] PM / devfreq: Don't use devm on parent device

[Leonard Crestez]

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

[PATCH 5/5] PM / devfreq: Don't take lock in devfreq_add_device

[Leonard Crestez]

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

Re: [PATCH 1/5] PM / devfreq: Don't fail devfreq_dev_release if not in list

[Chanwoo Choi]

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

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

[Chanwoo Choi]

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

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

[Leonard Crestez]

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

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

[Chanwoo Choi]

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