Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Chanwoo Choi <cw00.choi@samsung.com>]

On 3/18/21 6:54 PM, Chanwoo Choi wrote:
> On 3/18/21 5:03 PM, Dong Aisheng wrote:
>> Hi Chanwoo,
>>
>> On Sat, Mar 13, 2021 at 2:45 PM Dong Aisheng <dongas86@gmail.com> wrote:
>>>
>>> On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>>>>
>>>> On 21. 3. 12. 오후 7:57, Dong Aisheng wrote:
>>>>> On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>>>
>>>>>> On 3/10/21 1:56 PM, Dong Aisheng wrote:
>>>>>>> On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>>>>>
>>>>>>>> On 3/10/21 11:56 AM, Dong Aisheng wrote:
>>>>>>>>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
>>>>>>>>>>> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
>>>>>>>>>>>> The devfreq monitor depends on the device to provide load information
>>>>>>>>>>>> by .get_dev_status() to calculate the next target freq.
>>>>>>>>>>>>
>>>>>>>>>>>> And this will cause changing governor to simple ondemand fail
>>>>>>>>>>>> if device can't support.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>    drivers/devfreq/devfreq.c                 | 10 +++++++---
>>>>>>>>>>>>    drivers/devfreq/governor.h                |  2 +-
>>>>>>>>>>>>    drivers/devfreq/governor_simpleondemand.c |  3 +--
>>>>>>>>>>>>    3 files changed, 9 insertions(+), 6 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>>>>>>> index 7231fe6862a2..d1787b6c7d7c 100644
>>>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>>>>>>> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct
>>>>>>>>>>>> *work)
>>>>>>>>>>>>     * to be called from governor in response to DEVFREQ_GOV_START
>>>>>>>>>>>>     * event when device is added to devfreq framework.
>>>>>>>>>>>>     */
>>>>>>>>>>>> -void devfreq_monitor_start(struct devfreq *devfreq)
>>>>>>>>>>>> +int devfreq_monitor_start(struct devfreq *devfreq)
>>>>>>>>>>>>    {
>>>>>>>>>>>>        if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
>>>>>>>>>>>> -        return;
>>>>>>>>>>>> +        return 0;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if (!devfreq->profile->get_dev_status)
>>>>>>>>>>>> +        return -EINVAL;
>>>>>>>>>>
>>>>>>>>>> Again, I think that get_dev_status is not used for all governors.
>>>>>>>>>> So that it cause the governor start fail. Don't check whether
>>>>>>>>>> .get_dev_status is NULL or not.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm not quite understand your point.
>>>>>>>>> it is used by governor_simpleondemand.c and tegra_devfreq_governor.
>>>>>>>>> get_target_freq -> devfreq_update_stats -> get_dev_status
>>>>>>>>
>>>>>>>> The devfreq can add the new governor by anyone.
>>>>>>>> So these functions like devfreq_monitor_* have to support
>>>>>>>> the governors and also must support the governor to be added
>>>>>>>> in the future.
>>>>>>>
>>>>>>> Yes, but devfreq_monitor_* is only used by polling mode, right?
>>>>>>> The governor using it has to implement get_dev_status unless
>>>>>>> there's an exception in the future.
>>>>>>>
>>>>>>> Currently this patch wants to address the issue that user can switch
>>>>>>> to ondemand governor (polling mode) by sysfs even devices does
>>>>>>> not support it (no get_dev_status implemented).
>>>>>>
>>>>>> As I commented, I'll fix this issue. If devfreq driver doesn't implement
>>>>>> the .get_dev_status, don't show it via available_governors. I think that
>>>>>> it is fundamental solution to fix this issue.
>>>>>
>>>>> Sounds good
>>>>>
>>>>>> So on this version,
>>>>>> don't add the this conditional statement on this function
>>>>>>
>>>>>
>>>>> Almost all this patch did is adding a checking for get_dev_status.
>>>>> So do you mean drop this patch?
>>>>> I wonder it's still a necessary checking to explicitly tell devfreq monitor
>>>>> users that get_dev_status is needed during governor startup.
>>>>
>>>> I think that the it is enough to check .get_dev_status in
>>>> devfreq_update_stats. We have to check it on where it is used.
>>>>
>>>
>>> I think the drawback of only checking .get_dev_status in
>>> devfreq_update_stats is:
>>> 1. devfreq will still register successfully and ondemand governor starts ok
>>> 2. ondemand governor will still be shown in sysfs which is something
>>> you want to fix
>>> 3. devfreq will end up printing endless error messages in devfreq_monitor worker
>>>     "dvfs failed with (%d) error" as the possible missing .get_dev_status
> 
> I think that devfreq_monitor_start have to handle only work instance.
> This approach is too considering the deep check list.
> I want to resolve this periodical error log with different solution.
> 
> Actually, we have to reject the registration of devfreq device
> when calling devfreq_add_device instead of checking .get_dev_status
> in devfreq_monitor_start().


I'll reject the registration of devfreq device if the mandatory
function pointer of struct devfreq_dev_profile are not initialized.
- .get_dev_status
   If some governors like simple_ondemand, have to initialize it.
   So, I need to add the new flag to specify this feature.
- .target is mandatory for all devfreq devices. I'll check it.

> 
> 
>>>
>>> So i wonder if you don't like changing the common devfreq_monitor_start in order
>>> to make it look common for all governors, then we probably still need
>>> to fix it in
>>> ondemand governor in order to avoid the possible above issues.
>>>
>>> static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
>>>                                 unsigned int event, void *data)
>>> {
>>>         switch (event) {
>>>         case DEVFREQ_GOV_START:
>>>                 if (!devfreq->profile->get_dev_status)
>>>                         return -EINVAL;
>>>
>>>                 return devfreq_monitor_start(devfreq);
>>> ...
>>> }
>>>
>>> How do you think?
>>
>> Any feedback?
>>
>> I'm waiting for your confirmation whether dropping this one,
>> then I can re-sent the series.
>>
>> Regards
>> Aisheng
>>
>>>
>>> Regards
>>> Aisheng
>>>
>>>
>>>>>
>>>>>> And on next version, please use the capital letter for first character
>>>>>> on patch title as following:
>>>>>>
>>>>>> - PM / devfreq: Check get_dev_status before start monitor
>>>>>>
>>>>>
>>>>> Okay to me.
>>>>> Thanks for the suggestion.
>>>>>
>>>>> Regards
>>>>> Aisheng
>>>>>
>>>>>>>
>>>>>>> Regards
>>>>>>> Aisheng
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Without checking, device can switch to ondemand governor if it does not support.
>>>>>>>>>
>>>>>>>>> Am i missed something?
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Aisheng
>>>>>>>>>
>>>>>>>>>>>>        switch (devfreq->profile->timer) {
>>>>>>>>>>>>        case DEVFREQ_TIMER_DEFERRABLE:
>>>>>>>>>>>> @@ -495,12 +498,13 @@ void devfreq_monitor_start(struct devfreq *devfreq)
>>>>>>>>>>>>            INIT_DELAYED_WORK(&devfreq->work, devfreq_monitor);
>>>>>>>>>>>>            break;
>>>>>>>>>>>>        default:
>>>>>>>>>>>> -        return;
>>>>>>>>>>>> +        return -EINVAL;
>>>>>>>>>>>>        }
>>>>>>>>>>>>        if (devfreq->profile->polling_ms)
>>>>>>>>>>>>            queue_delayed_work(devfreq_wq, &devfreq->work,
>>>>>>>>>>>>                msecs_to_jiffies(devfreq->profile->polling_ms));
>>>>>>>>>>>> +    return 0;
>>>>>>>>>>>>    }
>>>>>>>>>>>>    EXPORT_SYMBOL(devfreq_monitor_start);
>>>>>>>>>>>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>>>>>>>>>>>> index 5cee3f64fe2b..31af6d072a10 100644
>>>>>>>>>>>> --- a/drivers/devfreq/governor.h
>>>>>>>>>>>> +++ b/drivers/devfreq/governor.h
>>>>>>>>>>>> @@ -75,7 +75,7 @@ struct devfreq_governor {
>>>>>>>>>>>>                    unsigned int event, void *data);
>>>>>>>>>>>>    };
>>>>>>>>>>>> -void devfreq_monitor_start(struct devfreq *devfreq);
>>>>>>>>>>>> +int devfreq_monitor_start(struct devfreq *devfreq);
>>>>>>>>>>>>    void devfreq_monitor_stop(struct devfreq *devfreq);
>>>>>>>>>>>>    void devfreq_monitor_suspend(struct devfreq *devfreq);
>>>>>>>>>>>>    void devfreq_monitor_resume(struct devfreq *devfreq);
>>>>>>>>>>>> diff --git a/drivers/devfreq/governor_simpleondemand.c
>>>>>>>>>>>> b/drivers/devfreq/governor_simpleondemand.c
>>>>>>>>>>>> index d57b82a2b570..ea287b57cbf3 100644
>>>>>>>>>>>> --- a/drivers/devfreq/governor_simpleondemand.c
>>>>>>>>>>>> +++ b/drivers/devfreq/governor_simpleondemand.c
>>>>>>>>>>>> @@ -89,8 +89,7 @@ static int devfreq_simple_ondemand_handler(struct
>>>>>>>>>>>> devfreq *devfreq,
>>>>>>>>>>>>    {
>>>>>>>>>>>>        switch (event) {
>>>>>>>>>>>>        case DEVFREQ_GOV_START:
>>>>>>>>>>>> -        devfreq_monitor_start(devfreq);
>>>>>>>>>>>> -        break;
>>>>>>>>>>>> +        return devfreq_monitor_start(devfreq);
>>>>>>>>>>>>        case DEVFREQ_GOV_STOP:
>>>>>>>>>>>>            devfreq_monitor_stop(devfreq);
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Need to handle the all points of devfreq_monitor_start() usage.
>>>>>>>>>>> please check the tegra30-devfreq.c for this update.
>>>>>>>>>>>
>>>>>>>>>>> $ grep -rn "devfreq_monitor_start" drivers/
>>>>>>>>>>> drivers/devfreq/governor_simpleondemand.c:92:
>>>>>>>>>>> devfreq_monitor_start(devfreq);
>>>>>>>>>>> drivers/devfreq/tegra30-devfreq.c:744:
>>>>>>>>>>> devfreq_monitor_start(devfreq);
>>>>>>>>>>> ......
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Best Regards,
>>>>>>>>>> Samsung Electronics
>>>>>>>>>> Chanwoo Choi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Best Regards,
>>>>>>>> Chanwoo Choi
>>>>>>>> Samsung Electronics
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best Regards,
>>>>>> Chanwoo Choi
>>>>>> Samsung Electronics
>>>>
>>>>
>>>> --
>>>> Best Regards,
>>>> Samsung Electronics
>>>> Chanwoo Choi
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics