On Wed, Mar 22, 2017 at 05:25:50PM +0000, Sudeep Holla wrote:

On 22/03/17 17:09, Suzuki K Poulose wrote:

quoted

On 22/03/17 16:17, Sudeep Holla wrote:

[...]

quoted

quoted

Point taken. So we could just specify that all necessary power
domains need to be on for proper functionality for this feature and
that it's highly platform specific instead of mixing cpu/cluster
idle details here.

quoted

The key point is that the caveat in using this driver is that
the power management has to be considered on a platform specific
basis before it is configured; and appropriate actions may be
needed for it to work correctly. Without this then the driver
could cause more issues than it debugs. A user selecting this
_must_ be told about these issues

So given all the possible caveats, I think we :

1) Shouldn't enable the driver by default at runtime even if it is 
built-in. 
2) Should provide mechanisms to turn it on at boot (via
kernel commandline) or anytime later (via sysfs), which kind of puts
the responsibility back on the user : "You know what you are doing". 
3) Shouldn't turn the driver on based on "nohlt" which the user
could use it for some other purposes, without explicit intention of
turning this driver on).
4) Should document the fact that, on some
platforms, the user may have to disable CPUidle explicitly to get the
driver working. But let us not make it the default. The user with a
not so ideal platform could add "nohlt" and get it working.

Agreed on all points and well summarized.
I would like to highlight (3) and (4) as it needs to be well understood.

"nohlt" has a *different* meaning already, so using that in this
driver for something else is simple wrong as it affects the system in
unintended ways. And yes if user (mis)uses it to get things working,
it's fine but shouldn't be recommended way.

Understand this point.

I will try to use general way to constraint CPUIdle like other
drivers.

Thanks all for these good suggestions :)

Thanks,
Leo Yan