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

[Sibi Sankar]

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

> Cheers,
> MyungJoo

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