Messages in this thread | | | From | Dong Aisheng <> | Date | Thu, 18 Mar 2021 16:03:22 +0800 | Subject | Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor |
| |
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 > > 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
| |