On 3/10/21 11:56 AM, Dong Aisheng wrote:

On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi [off-list ref] wrote:

quoted

On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:

quoted

On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:

quoted

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.

Without checking, device can switch to ondemand governor if it does not support.

Am i missed something?

Regards
Aisheng

quoted

quoted

quoted

      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