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

From: Sibi Sankar
Date: Tue Feb 19 2019 - 00:12:18 EST


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>

Hello,

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

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?


Cheers,
MyungJoo



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