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

On 19. 10. 31. 오후 10:31, Leonard Crestez wrote:

On 31.10.2019 05:10, Chanwoo Choi wrote:

quoted

Hi Leonard,

This patch didn't get the acked-by from devfreq maintainer.
I think that we need to discuss this patch with more time.
Also, it is possible to make it as the separate patch
from this series.

IMHO, if you make the separate patch for this and
resend the separate patch on later, I think that
we can merge the remained patch related to PM_QOS.

The devfreq initialization cleanups are required for dev_pm_qos support, 
otherwise lockdep warnings are triggered. I can post the cleanups as a 
separate series but the PM QoS one would depend on the cleanups.

Do you prefer multiple smaller series?

After read the v10, I think v9 is better than v10
for this issue. 

I try to order my patches with uncontroversial fixes and cleanups first 
so in theory the earlier parts could be applied separately. It's very 
rare to see series partially applied though.

Earlier objection was that devm should be kept, I think this can be 
accomplished by splitting device_register into device_initialize and 
device_add.

quoted

On 19. 10. 3. 오전 4:25, Leonard Crestez wrote:

quoted

In general it is a better to initialize an object before making it
accessible externally (through device_register).

This makes it possible to avoid remove locking the partially initialized
object and simplifies the code. However devm is not available before
device_register (only after the device_initialize step) so the two
allocations need to be managed manually.

Signed-off-by: Leonard Crestez <redacted>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
  drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++----------------
  1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 3e0e936185a3..0b40f40ee7aa 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c

@@ -591,10 +591,12 @@ static void devfreq_dev_release(struct device *dev)
  	mutex_unlock(&devfreq_list_lock);
  
  	if (devfreq->profile->exit)
  		devfreq->profile->exit(devfreq->dev.parent);
  
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
  	mutex_destroy(&devfreq->lock);
  	kfree(devfreq);
  }
  
  /**

@@ -674,44 +676,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;

err_out -> err_dev
When failed to register, have to free resource.

quoted

quoted

+	}
+
  	mutex_unlock(&devfreq->lock);
  
  	mutex_lock(&devfreq_list_lock);
  
  	governor = try_then_request_governor(devfreq->governor_name);

@@ -737,14 +738,20 @@ 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);

It is not proper to return on the middle 
of the exception handling. Need to consider more clean method.

quoted

quoted

+
  err_dev:
+	/*
+	 * Cleanup path for errors that happen before registration.
+	 * Otherwise we rely on devfreq_dev_release.
+	 */
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
  	kfree(devfreq);
  err_out:
  	return ERR_PTR(err);
  }
  EXPORT_SYMBOL(devfreq_add_device);

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel