Re: [PATCH v4] PM / devfreq: Restart previous governor if new governor fails to start

From: Sibi Sankar
Date: Wed Mar 06 2019 - 12:44:14 EST

On 2019-03-05 12:48, MyungJoo Ham wrote:
Hey MyungJoo, Kyungmin
Did you get a chance to think about how you
want this fix implemented?

On 2019-02-19 10:42, Sibi Sankar wrote:
Hey MyungJoo,

On 12/14/18 7:15 AM, MyungJoo Ham wrote:
From: Saravana Kannan <skannan@xxxxxxxxxxxxxx>

If the new governor fails to start, switch back to old governor so
that the
devfreq state is not left in some weird limbo.

Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
Signed-off-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
Reviewed-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>


In overall, the idea and the implementation looks good.

However, I have a question:

What if the following line fails?

+ df->governor->event_handler(df, DEVFREQ_GOV_START,
+ NULL);

Don't we still need something to handle for such events?

The original discussion went as follows:
governor_store is expected to be used only on cases
where devfreq_add_device() succeeded i.e prev->governor
is expected to be present and DEVFREQ_GOV_START is
expected to succeed. Hence falling back to the previous
governor seems like a sensible idea.

This would also prevent DEVFREQ_GOV_STOP from being called
on a governor were DEVFREQ_GOV_START had failed which is

That being said DEVFREQ_GOV_START can still fail for the
prev-governor due to some change in state of the system.
Do you want to handle this case by clearing the state of
governor rather than switching to previous governor?

If moving back to previous governor fails after
failing for "next" governor, we may assume it's fatal
and stop the device; we can simply return errors.

In such a case, df->governor may need to be NULL as well.

Thanks. Will make the necessary changes in the
next re-spin.


-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.