Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Thu, Mar 18, 2021 at 5:42 PM Chanwoo Choi <cw00.choi@xxxxxxxxxxx> 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@xxxxxxxxx> wrote:
> >>>
> >>> On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi <cwchoi00@xxxxxxxxx> wrote:
> >>>>
> >>>> On 21. 3. 12. 오후 7:57, Dong Aisheng wrote:
> >>>>> On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi <cw00.choi@xxxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On 3/10/21 1:56 PM, Dong Aisheng wrote:
> >>>>>>> On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi <cw00.choi@xxxxxxxxxxx> wrote:
> >>>>>>>>
> >>>>>>>> On 3/10/21 11:56 AM, Dong Aisheng wrote:
> >>>>>>>>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi <cwchoi00@xxxxxxxxx> 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@xxxxxxx>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>    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