lkml.org 
[lkml]   [2021]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2021-03-18 09:04    [W:5.210 / U:0.928 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site