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

[Matthias Kaehlcke <mka@chromium.org>]

On Mon, Sep 23, 2019 at 07:56:51PM +0000, Leonard Crestez wrote:
> On 23.09.2019 22:11, Matthias Kaehlcke wrote:
> > 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?"
> 
> Sorry for missing that.
> > Could you clarify this replying to the thread? Besides that and the
> > nits (which are optional to fix) the patch looks good to me.
> 
> The blocking_notifier_head structs are managed by PM QoS inside 
> dev_pm_qos_constraints_allocate and dev_pm_qos_constraints_destroy. The 
> devfreq subsystem only registers a notifier_block, that's a 
> NULL-terminated singly-linked list for which zero-initialization from 
> kzalloc should be sufficient.

Right, thanks for the clarification!

> But now that I look at this again I should warn and return NOTIFY_DONE 
> from devfreq_qos_notifier_call instead of propagating a negative errno.

Ack