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
    On Thu, Mar 18, 2021 at 5:42 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
    >
    > 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 <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
    > >
    > > 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.

    Okay, thanks
    Then i will drop this patch and resend series with your other comments
    addressed.

    Regards
    Aisheng

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

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