Re: [PATCH v9 2/8] PM / devfreq: Fix devfreq_notifier_call returning errno

[Leonard Crestez <leonard.crestez@nxp.com>]

On 2019-10-03 12:24 AM, Matthias Kaehlcke wrote:
> On Wed, Oct 02, 2019 at 10:25:05PM +0300, Leonard Crestez wrote:
>> Notifier callbacks shouldn't return negative errno but one of the
>> NOTIFY_OK/DONE/BAD values.
>>
>> The OPP core will ignore return values from notifiers but returning a
>> value that matches NOTIFY_STOP_MASK will stop the notification chain.
>>
>> Fix by always returning NOTIFY_OK.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 7dc899da1172..32bbf6e80380 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update);
>>    */
>>   static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>   				 void *devp)
>>   {
>>   	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
>> -	int ret;
>> +	int err = -EINVAL;
>>   
>>   	mutex_lock(&devfreq->lock);
>>   
>>   	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> -	if (!devfreq->scaling_min_freq) {
>> -		mutex_unlock(&devfreq->lock);
>> -		return -EINVAL;
>> -	}
>> +	if (!devfreq->scaling_min_freq)
>> +		goto out;
>>   
>>   	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> -	if (!devfreq->scaling_max_freq) {
>> -		mutex_unlock(&devfreq->lock);
>> -		return -EINVAL;
>> -	}
>> +	if (!devfreq->scaling_max_freq)
>> +		goto out;
>> +
>> +	err = update_devfreq(devfreq);
>>   
>> -	ret = update_devfreq(devfreq);
>> +out:
>>   	mutex_unlock(&devfreq->lock);
>> +	if (err)
>> +		dev_err(devfreq->dev.parent,
>> +			"failed to update frequency from OPP notifier (%d)\n",
>> +			err);
> 
> In case an OPP freq can't be found the log doesn't provide clues
> about what the problem could be. I couldn't find a clearly superior
> errno value though, so I guess this is as good as it gets, unless
> you want to have a dedicated message for those errors. Should be a
> rare exception anyway, and previously there was no log at all.

I guess it could happen if all OPPs are disabled after probe?

The devfreq core will attempt to switch away if the current OPP get 
disabled but if nothing else is available then printing an error and 
sticking to the current frequency seems reasonable.

It would indicate a bug somewhere else.

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