Messages in this thread | | | Subject | Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor | From | Chanwoo Choi <> | Date | Sat, 13 Mar 2021 01:09:51 +0900 |
| |
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.
> >> 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
| |