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 <donga...@gmail.com> wrote: >>> >>> On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi <cwcho...@gmail.com> wrote: >>>> >>>> On 21. 3. 12. 오후 7:57, Dong Aisheng wrote: >>>>> On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi <cw00.c...@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.c...@samsung.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> On 3/10/21 11:56 AM, Dong Aisheng wrote: >>>>>>>>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi <cwcho...@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.d...@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