From: Chanwoo Choi <cw00.choi@samsung.com>
To: Dong Aisheng <dongas86@gmail.com>, Chanwoo Choi <cwchoi00@gmail.com>
Cc: Dong Aisheng <aisheng.dong@nxp.com>,
Linux PM <linux-pm@vger.kernel.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@lists.infradead.org>,
Sascha Hauer <kernel@pengutronix.de>,
Shawn Guo <shawnguo@kernel.org>, dl-linux-imx <linux-imx@nxp.com>,
open list <linux-kernel@vger.kernel.org>,
myungjoo.ham@samsung.com, kyungmin.park@samsung.com,
Abel Vesa <abel.vesa@nxp.com>
Subject: Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor
Date: Thu, 18 Mar 2021 18:59:30 +0900 [thread overview]
Message-ID: <4b5a0c61-2bab-8b00-621c-25f1244fee92@samsung.com> (raw)
In-Reply-To: <0e518145-5cae-63f1-c32f-e6207fdeae54@samsung.com>
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.
>
>
>>>
>>> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-18 9:44 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-09 12:58 [PATCH 00/11] PM / devfreq: a few small fixes and improvements Dong Aisheng
2021-03-09 12:58 ` [PATCH 01/11] doc: ABI: devfreq: remove invalid central_polling description Dong Aisheng
2021-03-09 15:01 ` Chanwoo Choi
2021-03-09 12:58 ` [PATCH 02/11] PM / devfreq: remove the invalid description for get_target_freq Dong Aisheng
2021-03-09 15:02 ` Chanwoo Choi
2021-03-09 15:14 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 03/11] PM / devfreq: fix the wrong set_freq path for userspace governor Dong Aisheng
2021-03-09 15:13 ` Chanwoo Choi
2021-03-09 15:16 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 04/11] PM / devfreq: bail out early if no freq changes in devfreq_set_target Dong Aisheng
2021-03-09 15:47 ` Chanwoo Choi
2021-03-10 2:42 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 05/11] PM / devfreq: use more accurate returned new_freq as resume_freq Dong Aisheng
2021-03-09 15:53 ` Chanwoo Choi
2021-03-10 2:43 ` Dong Aisheng
2021-03-10 3:07 ` Chanwoo Choi
2021-03-10 3:04 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 06/11] PM / devfreq: drop the unnecessary low variable initialization Dong Aisheng
2021-03-09 12:58 ` [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor Dong Aisheng
2021-03-09 15:58 ` Chanwoo Choi
2021-03-09 16:12 ` Chanwoo Choi
2021-03-10 2:56 ` Dong Aisheng
2021-03-10 3:25 ` Chanwoo Choi
2021-03-10 4:56 ` Dong Aisheng
2021-03-11 7:10 ` Chanwoo Choi
2021-03-12 10:57 ` Dong Aisheng
2021-03-12 16:09 ` Chanwoo Choi
2021-03-13 6:45 ` Dong Aisheng
2021-03-18 8:03 ` Dong Aisheng
2021-03-18 9:54 ` Chanwoo Choi
2021-03-18 9:59 ` Chanwoo Choi [this message]
2021-03-18 10:51 ` Dong Aisheng
2021-03-10 2:51 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 08/11] PM / devfreq: check get_dev_status in devfreq_update_stats Dong Aisheng
2021-03-09 16:02 ` Chanwoo Choi
2021-03-09 16:20 ` Chanwoo Choi
2021-03-10 3:00 ` Dong Aisheng
2021-03-11 7:01 ` Chanwoo Choi
2021-03-09 12:58 ` [PATCH 09/11] PM / devfreq: governor: optimize simpleondemand get_target_freq Dong Aisheng
2021-03-09 16:09 ` Chanwoo Choi
2021-03-10 3:02 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 10/11] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status Dong Aisheng
2021-03-09 16:22 ` Chanwoo Choi
2021-03-09 12:58 ` [PATCH 11/11] PM / devfreq: imx8m-ddrc: drop polling_ms Dong Aisheng
2021-03-09 16:23 ` Chanwoo Choi
2021-03-10 3:03 ` Dong Aisheng
2021-03-09 12:58 ` [PATCH 00/11] PM / devfreq: a few small fixes and improvements Dong Aisheng
2021-03-09 12:58 ` [PATCH 01/11] doc: ABI: devfreq: remove invalid central_polling description Dong Aisheng
2021-03-09 12:58 ` [PATCH 02/11] PM / devfreq: remove the invalid description for get_target_freq Dong Aisheng
2021-03-09 12:58 ` [PATCH 03/11] PM / devfreq: fix the wrong set_freq path for userspace governor Dong Aisheng
2021-03-09 12:58 ` [PATCH 04/11] PM / devfreq: bail out early if no freq changes in devfreq_set_target Dong Aisheng
2021-03-09 12:58 ` [PATCH 05/11] PM / devfreq: use more accurate returned new_freq as resume_freq Dong Aisheng
2021-03-09 12:58 ` [PATCH 06/11] PM / devfreq: drop the unnecessary low variable initialization Dong Aisheng
2021-03-09 12:58 ` [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor Dong Aisheng
2021-03-09 12:58 ` [PATCH 08/11] PM / devfreq: check get_dev_status in devfreq_update_stats Dong Aisheng
2021-03-09 12:58 ` [PATCH 09/11] PM / devfreq: governor: optimize simpleondemand get_target_freq Dong Aisheng
2021-03-09 12:58 ` [PATCH 10/11] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status Dong Aisheng
2021-03-09 12:58 ` [PATCH 11/11] PM / devfreq: imx8m-ddrc: drop polling_ms Dong Aisheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4b5a0c61-2bab-8b00-621c-25f1244fee92@samsung.com \
--to=cw00.choi@samsung.com \
--cc=abel.vesa@nxp.com \
--cc=aisheng.dong@nxp.com \
--cc=cwchoi00@gmail.com \
--cc=dongas86@gmail.com \
--cc=kernel@pengutronix.de \
--cc=kyungmin.park@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).