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

[Matthias Kaehlcke <mka@chromium.org>]

On Wed, Sep 18, 2019 at 04:29:04PM -0700, 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.
> 
> >  err_dev:
> 
> This code path should include
> 
> 	mutex_destroy(&devfreq->lock);
> 
> That was already missing in the original code though.
> 
> 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.

Just saw that you are already doing this in "[4/8] PM / devfreq:
Don't take lock in devfreq_add_device" :)