[PATCH 07/11] PM / devfreq: check get_dev_status before start monitor
Chanwoo Choi
cw00.choi at samsung.com
Thu Mar 18 09:59:30 GMT 2021
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 at gmail.com> wrote:
>>>
>>> On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi <cwchoi00 at 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 at 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 at 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 at 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 at 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
More information about the linux-arm-kernel
mailing list