Hi David,
On Thu, Feb 20, 2025 at 1:21 PM David Arcari <darcari@xxxxxxxxxx> wrote:
>
>
> Hi Rafael,
>
> On 2/19/25 4:27 PM, Rafael J. Wysocki wrote:
> > On Thu, Feb 13, 2025 at 5:07 PM David Arcari <darcari@xxxxxxxxxx> wrote:
> >>
> >> Since commit 18734958e9bf ("intel_idle: Use ACPI _CST for processor models
> >> without C-state tables") the intel_idle driver has had the ability to use
> >> the ACPI _CST to populate C-states when the processor model is not
> >> recognized. However, even when the processor model is recognized (native
> >> mode) there are cases where it is useful to make the driver ignore the per
> >> cpu idle states in lieu of ACPI C-states (such as specific application
> >> performance). Add the 'no_native' module parameter to provide this
> >> functionality.
> >>
> >> Cc: Jonathan Corbet <corbet@xxxxxxx>
> >> Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >> Cc: Len Brown <lenb@xxxxxxxxxx>
> >> Cc: David Arcari <darcari@xxxxxxxxxx>
> >> Cc: Artem Bityutskiy <dedekind1@xxxxxxxxx>
> >> Cc: Prarit Bhargava <prarit@xxxxxxxxxx>
> >> Cc: linux-doc@xxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Signed-off-by: David Arcari <darcari@xxxxxxxxxx>
> >> ---
> >> v4: fix !CONFIG_ACPI_PROCESSOR_CSTATE compilation issue
> >> v3: more documentation cleanup
> >> v2: renamed parameter, cleaned up documentation
> >>
> >> Documentation/admin-guide/pm/intel_idle.rst | 18 +++++++++++++-----
> >> drivers/idle/intel_idle.c | 16 ++++++++++++++++
> >> 2 files changed, 29 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/admin-guide/pm/intel_idle.rst b/Documentation/admin-guide/pm/intel_idle.rst
> >> index 39bd6ecce7de..5940528146eb 100644
> >> --- a/Documentation/admin-guide/pm/intel_idle.rst
> >> +++ b/Documentation/admin-guide/pm/intel_idle.rst
> >> @@ -192,11 +192,19 @@ even if they have been enumerated (see :ref:`cpu-pm-qos` in
> >> Documentation/admin-guide/pm/cpuidle.rst).
> >> Setting ``max_cstate`` to 0 causes the ``intel_idle`` initialization to fail.
> >>
> >> -The ``no_acpi`` and ``use_acpi`` module parameters (recognized by ``intel_idle``
> >> -if the kernel has been configured with ACPI support) can be set to make the
> >> -driver ignore the system's ACPI tables entirely or use them for all of the
> >> -recognized processor models, respectively (they both are unset by default and
> >> -``use_acpi`` has no effect if ``no_acpi`` is set).
> >> +The ``no_acpi``, ``use_acpi`` and ``no_native`` module parameters are
> >> +recognized by ``intel_idle`` if the kernel has been configured with ACPI
> >> +support. In the case that ACPI is not configured these flags have no impact
> >> +on functionality.
> >> +
> >> +``no_acpi`` - Do not use ACPI at all. Only native mode is available, no
> >> +ACPI mode.
> >> +
> >> +``use_acpi`` - No-op in ACPI mode, the driver will consult ACPI tables for
> >> +C-states on/off status in native mode.
> >> +
> >> +``no_native`` - Work only in ACPI mode, no native mode available (ignore
> >> +all custom tables).
> >>
> >> The value of the ``states_off`` module parameter (0 by default) represents a
> >> list of idle states to be disabled by default in the form of a bitmask.
> >> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> >> index 118fe1d37c22..b0be5ef43ffc 100644
> >> --- a/drivers/idle/intel_idle.c
> >> +++ b/drivers/idle/intel_idle.c
> >> @@ -1695,6 +1695,10 @@ static bool force_use_acpi __read_mostly; /* No effect if no_acpi is set. */
> >> module_param_named(use_acpi, force_use_acpi, bool, 0444);
> >> MODULE_PARM_DESC(use_acpi, "Use ACPI _CST for building the idle states list");
> >>
> >> +static bool no_native __read_mostly; /* No effect if no_acpi is set. */
> >> +module_param_named(no_native, no_native, bool, 0444);
> >> +MODULE_PARM_DESC(no_native, "Ignore cpu specific (native) idle states in lieu of ACPI idle states");
> >> +
> >> static struct acpi_processor_power acpi_state_table __initdata;
> >>
> >> /**
> >> @@ -1834,6 +1838,11 @@ static bool __init intel_idle_off_by_default(unsigned int flags, u32 mwait_hint)
> >> }
> >> return true;
> >> }
> >> +
> >> +static inline bool ignore_native(void)
> >> +{
> >> + return no_native & !no_acpi;
> >> +}
> >> #else /* !CONFIG_ACPI_PROCESSOR_CSTATE */
> >> #define force_use_acpi (false)
> >>
> >> @@ -1843,6 +1852,7 @@ static inline bool intel_idle_off_by_default(unsigned int flags, u32 mwait_hint)
> >> {
> >> return false;
> >> }
> >> +static inline bool ignore_native(void) { return false; }
> >> #endif /* !CONFIG_ACPI_PROCESSOR_CSTATE */
> >>
> >> /**
> >> @@ -2328,6 +2338,12 @@ static int __init intel_idle_init(void)
> >> pr_debug("MWAIT substates: 0x%x\n", mwait_substates);
> >>
> >> icpu = (const struct idle_cpu *)id->driver_data;
> >> + if (ignore_native()) {
> >> + if (icpu) {
> >> + pr_debug("ignoring native cpu idle states\n");
> >> + icpu = NULL;
> >> + }
> >> + }
> >
> > Why not
> >
> > + if (icpu && ignore_native()) {
> > + pr_debug("disregarding built-in CPU idle states table\n");
> > + icpu = NULL;
> > + }
>
> That's cleaner. I'll submit a v5 shortly.
>
> Should the pr_debug be a pr_info? I've waffled on this, but think that
> pr_info is probably a good idea. Or do you prefer pr_debug?
I think that this is debug mostly as it allows one to verify that
no_native is taken into account as expected.
