Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke <mka@chromium.org>]

On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> > On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> >> In general it is a better to initialize an object before making it
> >> accessible externally (through device_register).
> >>
> >> This makes it possible to avoid relying on locking a partially
> >> initialized object.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   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 323d43315d1e..b4d2bfebb140 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -587,10 +587,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);
> >>   }
> >>   
> >>   /**
> >> @@ -670,44 +672,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;
> > 
> > As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> > device registration failed, hence devfreq_dev_release() won't be
> > called to free allocated memory.
> 
> This code is not modified in the patch, it only shows up as +added 
> because diff got confused but there is an identical -removed chunk 
> higher up.
> 
> The device_register documentation mentions the following:
> 
>   * NOTE: _Never_ directly free @dev after calling this function, even
>   * if it returned an error! Always use put_device() to give up the
>   * reference initialized in this function instead.
> 
> Cleanup path then goes like this (from a hacked error in device_add):
>   dump_stack+0xdc/0x144 
>  
> 
>   devfreq_dev_release+0x38/0xc0 
>  
> 
>   device_release+0x34/0x90 
>  
> 
>   kobject_put+0x8c/0x1f0 
>  
> 
>   put_device+0x24/0x30 
>  
> 
>   devfreq_add_device+0x540/0x570 
>  
> 
>   devm_devfreq_add_device+0x60/0xd0 
>  
> 
>   imx_ddrc_probe+0x35c/0x4c8

Good to know, thanks for the pointer!

> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?

By now you should have it for most patches. For this one:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
posted on v5:

"IIUC you rely on the notifiers being removed by devfreq_dev_release().
Does dev_pm_qos_remove_notifier() behave gracefully if the notifier is
not initialized/added or do we need to use BLOCKING_NOTIFIER_INIT() or
similar?"

Could you clarify this replying to the thread? Besides that and the
nits (which are optional to fix) the patch looks good to me.