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. > 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