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

[Matthias Kaehlcke <mka@chromium.org>]

On Thu, Sep 19, 2019 at 06:52:07PM +0000, Leonard Crestez wrote:
> 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?

As per my earlier reply to self: I didn't look at the next patch
before writing this, it's all good, nothing to do here :)

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