Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency
@ 2020-06-25 22:49 Srinivas Pandruvada
  2020-06-26  8:49 ` Borislav Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: Srinivas Pandruvada @ 2020-06-25 22:49 UTC (permalink / raw)
  To: rjw, viresh.kumar, lenb, dsmythies, bp, tglx, mingo, hpa, peterz
  Cc: linux-pm, linux-kernel, x86, Srinivas Pandruvada

By default intel_pstate driver disables energy efficiency by setting
MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP mode.
This CPU model is also shared by Coffee Lake desktop CPUs. This allows
these systems to reach maximum possible frequency. But this adds power
penalty, which some customers don't want. They want some way to enable/
disable dynamically.

So, add an additional attribute "energy_efficiency_enable" under
/sys/devices/system/cpu/intel_pstate/ for these CPU models. This allows
to read and write bit 19 ("Disable Energy Efficiency Optimization") in
the MSR IA32_POWER_CTL.

This attribute is present in both HWP and non-HWP mode as this has an
effect in both modes. Refer to Intel Software Developer's manual for
details. The scope of this bit is package wide. Also these systems
support only one package. So read/write MSR on the current CPU is
enough.

Suggested-by: Len Brown <lenb@kernel.org>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
v3 update
Moved the MSR bit definition to msr-index.h from intel_pstate.c as Doug
wanted. Offline checking with Borislav, for MSR defintion it is
fine to move to  msr-index.h even for single user of the definition. But
here the MSR definition is already in msr-index.h, but adding the MSR bit
definition also.

 Documentation/admin-guide/pm/intel_pstate.rst |  9 ++++
 arch/x86/include/asm/msr-index.h              |  1 +
 drivers/cpufreq/intel_pstate.c                | 47 ++++++++++++++++++-
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-guide/pm/intel_pstate.rst
index 39d80bc29ccd..1ca2684a94d7 100644
--- a/Documentation/admin-guide/pm/intel_pstate.rst
+++ b/Documentation/admin-guide/pm/intel_pstate.rst
@@ -431,6 +431,15 @@ argument is passed to the kernel in the command line.
 	supported in the current configuration, writes to this attribute will
 	fail with an appropriate error.
 
+``energy_efficiency_enable``
+	This attribute is only present on platforms, which has CPUs matching
+	Kaby Lake or Coffee Lake desktop CPU model. By default
+	"energy_efficiency" is disabled on these CPU models in HWP mode by this
+	driver. Enabling energy efficiency may limit maximum operating
+	frequency in both HWP and non HWP mode. In non HWP mode, this attribute
+	has an effect in turbo range only. But in HWP mode, this attribute also
+	has an effect in non turbo range.
+
 Interpretation of Policy Attributes
 -----------------------------------
 
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index e8370e64a155..fec86ad14f8d 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -254,6 +254,7 @@
 #define MSR_PEBS_FRONTEND		0x000003f7
 
 #define MSR_IA32_POWER_CTL		0x000001fc
+#define MSR_IA32_POWER_CTL_BIT_EE	19
 
 #define MSR_IA32_MC0_CTL		0x00000400
 #define MSR_IA32_MC0_STATUS		0x00000401
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 8e23a698ce04..daa1d9c12098 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1218,6 +1218,42 @@ static ssize_t store_hwp_dynamic_boost(struct kobject *a,
 	return count;
 }
 
+static ssize_t show_energy_efficiency_enable(struct kobject *kobj,
+					     struct kobj_attribute *attr,
+					     char *buf)
+{
+	u64 power_ctl;
+	int enable;
+
+	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
+	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >> MSR_IA32_POWER_CTL_BIT_EE;
+	return sprintf(buf, "%d\n", !enable);
+}
+
+static ssize_t store_energy_efficiency_enable(struct kobject *a,
+					      struct kobj_attribute *b,
+					      const char *buf, size_t count)
+{
+	u64 power_ctl;
+	u32 input;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &input);
+	if (ret)
+		return ret;
+
+	mutex_lock(&intel_pstate_driver_lock);
+	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
+	if (input)
+		power_ctl &= ~BIT(MSR_IA32_POWER_CTL_BIT_EE);
+	else
+		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
+	wrmsrl(MSR_IA32_POWER_CTL, power_ctl);
+	mutex_unlock(&intel_pstate_driver_lock);
+
+	return count;
+}
+
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
 
@@ -1228,6 +1264,7 @@ define_one_global_rw(min_perf_pct);
 define_one_global_ro(turbo_pct);
 define_one_global_ro(num_pstates);
 define_one_global_rw(hwp_dynamic_boost);
+define_one_global_rw(energy_efficiency_enable);
 
 static struct attribute *intel_pstate_attributes[] = {
 	&status.attr,
@@ -1241,6 +1278,8 @@ static const struct attribute_group intel_pstate_attr_group = {
 	.attrs = intel_pstate_attributes,
 };
 
+static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[];
+
 static void __init intel_pstate_sysfs_expose_params(void)
 {
 	struct kobject *intel_pstate_kobject;
@@ -1273,6 +1312,12 @@ static void __init intel_pstate_sysfs_expose_params(void)
 				       &hwp_dynamic_boost.attr);
 		WARN_ON(rc);
 	}
+
+	if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids)) {
+		rc = sysfs_create_file(intel_pstate_kobject,
+				       &energy_efficiency_enable.attr);
+		WARN_ON(rc);
+	}
 }
 /************************** sysfs end ************************/
 
@@ -1288,8 +1333,6 @@ static void intel_pstate_hwp_enable(struct cpudata *cpudata)
 		cpudata->epp_default = intel_pstate_get_epp(cpudata, 0);
 }
 
-#define MSR_IA32_POWER_CTL_BIT_EE	19
-
 /* Disable energy efficiency optimization */
 static void intel_pstate_disable_ee(int cpu)
 {
-- 
2.25.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency
  2020-06-25 22:49 [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency Srinivas Pandruvada
@ 2020-06-26  8:49 ` Borislav Petkov
  2020-06-26  9:12   ` Srinivas Pandruvada
  2020-06-26 10:22   ` Peter Zijlstra
  0 siblings, 2 replies; 10+ messages in thread
From: Borislav Petkov @ 2020-06-26  8:49 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: rjw, viresh.kumar, lenb, dsmythies, tglx, mingo, hpa, peterz,
	linux-pm, linux-kernel, x86

On Thu, Jun 25, 2020 at 03:49:31PM -0700, Srinivas Pandruvada wrote:
> By default intel_pstate driver disables energy efficiency by setting
> MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP mode.
> This CPU model is also shared by Coffee Lake desktop CPUs. This allows
> these systems to reach maximum possible frequency. But this adds power
> penalty, which some customers don't want. They want some way to enable/
> disable dynamically.
> 
> So, add an additional attribute "energy_efficiency_enable" under
> /sys/devices/system/cpu/intel_pstate/ for these CPU models. This allows
> to read and write bit 19 ("Disable Energy Efficiency Optimization") in
> the MSR IA32_POWER_CTL.

Yes, this is how functionality behind MSRs should be made available to
userspace - not poking at naked MSRs. Good.

> This attribute is present in both HWP and non-HWP mode as this has an
> effect in both modes. Refer to Intel Software Developer's manual for
> details. The scope of this bit is package wide. Also these systems
> support only one package. So read/write MSR on the current CPU is
> enough.
> 
> Suggested-by: Len Brown <lenb@kernel.org>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v3 update
> Moved the MSR bit definition to msr-index.h from intel_pstate.c as Doug
> wanted. Offline checking with Borislav, for MSR defintion it is
> fine to move to  msr-index.h even for single user of the definition. But
> here the MSR definition is already in msr-index.h, but adding the MSR bit
> definition also.

Yes.

Btw, no need for the "offline checking" - you can do this on the mailing
list just fine.

>  Documentation/admin-guide/pm/intel_pstate.rst |  9 ++++
>  arch/x86/include/asm/msr-index.h              |  1 +
>  drivers/cpufreq/intel_pstate.c                | 47 ++++++++++++++++++-
>  3 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-guide/pm/intel_pstate.rst
> index 39d80bc29ccd..1ca2684a94d7 100644
> --- a/Documentation/admin-guide/pm/intel_pstate.rst
> +++ b/Documentation/admin-guide/pm/intel_pstate.rst
> @@ -431,6 +431,15 @@ argument is passed to the kernel in the command line.
>  	supported in the current configuration, writes to this attribute will
>  	fail with an appropriate error.
>  
> +``energy_efficiency_enable``
> +	This attribute is only present on platforms, which has CPUs matching

						which have

> +	Kaby Lake or Coffee Lake desktop CPU model. By default
> +	"energy_efficiency" is disabled on these CPU models in HWP mode by this
> +	driver. Enabling energy efficiency may limit maximum operating
> +	frequency in both HWP and non HWP mode. In non HWP mode, this attribute
> +	has an effect in turbo range only. But in HWP mode, this attribute also
> +	has an effect in non turbo range.

Those last two sentences could be simplified - read strange.

> +
>  Interpretation of Policy Attributes
>  -----------------------------------
>  
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e8370e64a155..fec86ad14f8d 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -254,6 +254,7 @@
>  #define MSR_PEBS_FRONTEND		0x000003f7
>  
>  #define MSR_IA32_POWER_CTL		0x000001fc
> +#define MSR_IA32_POWER_CTL_BIT_EE	19

Sort that MSR in - I know, the rest is not sorted either but we can
start somewhere. So pls put it...

#define MSR_LBR_SELECT                  0x000001c8
#define MSR_LBR_TOS                     0x000001c9

<--- here.

#define MSR_LBR_NHM_FROM                0x00000680
#define MSR_LBR_NHM_TO                  0x000006c0


>  #define MSR_IA32_MC0_CTL		0x00000400
>  #define MSR_IA32_MC0_STATUS		0x00000401
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 8e23a698ce04..daa1d9c12098 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1218,6 +1218,42 @@ static ssize_t store_hwp_dynamic_boost(struct kobject *a,
>  	return count;
>  }
>  
> +static ssize_t show_energy_efficiency_enable(struct kobject *kobj,
> +					     struct kobj_attribute *attr,
> +					     char *buf)
> +{
> +	u64 power_ctl;
> +	int enable;
> +
> +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >> MSR_IA32_POWER_CTL_BIT_EE;

So you can simplify to:

	enable = !!(power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE));

methinks.

> +	return sprintf(buf, "%d\n", !enable);

If this bit is called

	"Disable Energy Efficiency Optimization"

why do you call your function and sysfs file "enable"? This is making it
more confusing.

Why don't you call it simply: "energy_efficiency" and have it intuitive:

1 - enabled
0 - disabled

?

> +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> +					      struct kobj_attribute *b,
> +					      const char *buf, size_t count)
> +{
> +	u64 power_ctl;
> +	u32 input;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &input);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&intel_pstate_driver_lock);
> +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> +	if (input)

This is too lax - it will be enabled for any !0 value. Please accept
only 0 and 1.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency
  2020-06-26  8:49 ` Borislav Petkov
@ 2020-06-26  9:12   ` Srinivas Pandruvada
  2020-06-26 10:22   ` Peter Zijlstra
  1 sibling, 0 replies; 10+ messages in thread
From: Srinivas Pandruvada @ 2020-06-26  9:12 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: rjw, viresh.kumar, lenb, dsmythies, tglx, mingo, hpa, peterz,
	linux-pm, linux-kernel, x86

On Fri, 2020-06-26 at 10:49 +0200, Borislav Petkov wrote:
> On Thu, Jun 25, 2020 at 03:49:31PM -0700, Srinivas Pandruvada wrote:
> > By default intel_pstate driver disables energy efficiency by
> > setting
> > MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP
> > mode.
> > This CPU model is also shared by Coffee Lake desktop CPUs. This
> > allows
> > these systems to reach maximum possible frequency. But this adds
> > power
> > penalty, which some customers don't want. They want some way to
> > enable/
> > disable dynamically.
> > 
> > So, add an additional attribute "energy_efficiency_enable" under
> > /sys/devices/system/cpu/intel_pstate/ for these CPU models. This
> > allows
> > to read and write bit 19 ("Disable Energy Efficiency Optimization")
> > in
> > the MSR IA32_POWER_CTL.
> 
> 

[...]

> > +``energy_efficiency_enable``
> > +	This attribute is only present on platforms, which has CPUs
> > matching
> 
> 						which have
> 
Thanks, I will fix that.

> > +	Kaby Lake or Coffee Lake desktop CPU model. By default
> > +	"energy_efficiency" is disabled on these CPU models in HWP mode
> > by this
> > +	driver. Enabling energy efficiency may limit maximum operating
> > +	frequency in both HWP and non HWP mode. In non HWP mode, this
> > attribute
> > +	has an effect in turbo range only. But in HWP mode, this
> > attribute also
> > +	has an effect in non turbo range.
> 
> Those last two sentences could be simplified - read strange.
I will try to address this.

[...]

> > @@ -254,6 +254,7 @@
> >  #define MSR_PEBS_FRONTEND		0x000003f7
> >  
> >  #define MSR_IA32_POWER_CTL		0x000001fc
> > +#define MSR_IA32_POWER_CTL_BIT_EE	19
> 
> Sort that MSR in - I know, the rest is not sorted either but we can
> start somewhere. So pls put it...
> 
I will.

> #define MSR_LBR_SELECT                  0x000001c8
> #define MSR_LBR_TOS                     0x000001c9
> 
> <--- here.
> 
> 

[...]

> > +
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >>
> > MSR_IA32_POWER_CTL_BIT_EE;
> 
> So you can simplify to:
> 
> 	enable = !!(power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE));
> 
> methinks.
> 
Better.

> > +	return sprintf(buf, "%d\n", !enable);
> 
> If this bit is called
> 
> 	"Disable Energy Efficiency Optimization"
> 
> why do you call your function and sysfs file "enable"? This is making
> it
> more confusing.
> 
> Why don't you call it simply: "energy_efficiency" and have it
> intuitive:
> 
> 1 - enabled
> 0 - disabled
> 
I think your suggestion is good. The one other attributes under this
directory has similar style. I will get rid of "_enable".

> ?
> 
> > +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> > +					      struct kobj_attribute *b,
> > +					      const char *buf, size_t
> > count)
> > +{
> > +	u64 power_ctl;
> > +	u32 input;
> > +	int ret;
> > +
> > +	ret = kstrtouint(buf, 10, &input);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&intel_pstate_driver_lock);
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	if (input)
> 
> This is too lax - it will be enabled for any !0 value. Please accept
> only 0 and 1.
> 
OK.


Thanks for the review.


- Srinivas



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency
  2020-06-26  8:49 ` Borislav Petkov
  2020-06-26  9:12   ` Srinivas Pandruvada
@ 2020-06-26 10:22   ` Peter Zijlstra
  2020-06-26 10:44     ` [PATCH] lib: Extend kstrtobool() to accept "true"/"false" Peter Zijlstra
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2020-06-26 10:22 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Srinivas Pandruvada, rjw, viresh.kumar, lenb, dsmythies, tglx,
	mingo, hpa, linux-pm, linux-kernel, x86

On Fri, Jun 26, 2020 at 10:49:03AM +0200, Borislav Petkov wrote:
> On Thu, Jun 25, 2020 at 03:49:31PM -0700, Srinivas Pandruvada wrote:
> > +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> > +					      struct kobj_attribute *b,
> > +					      const char *buf, size_t count)
> > +{
> > +	u64 power_ctl;
> > +	u32 input;
> > +	int ret;
> > +
> > +	ret = kstrtouint(buf, 10, &input);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&intel_pstate_driver_lock);
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	if (input)
> 
> This is too lax - it will be enabled for any !0 value. Please accept
> only 0 and 1.

kstrtobool() ftw

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] lib: Extend kstrtobool() to accept "true"/"false"
  2020-06-26 10:22   ` Peter Zijlstra
@ 2020-06-26 10:44     ` Peter Zijlstra
  2020-06-26 11:10       ` Srinivas Pandruvada
                         ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Peter Zijlstra @ 2020-06-26 10:44 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Srinivas Pandruvada, rjw, viresh.kumar, lenb, dsmythies, tglx,
	mingo, hpa, linux-pm, linux-kernel, x86, jic23, keescook, akpm

On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote:

> > This is too lax - it will be enabled for any !0 value. Please accept
> > only 0 and 1.
> 
> kstrtobool() ftw

And looking at that, I find it really strange it does not in fact accept
"true" / "false", so how about this?

---
Subject: lib: Extend kstrtobool() to accept "true"/"false"

Extend the strings recognised by kstrtobool() to cover:

  - 1/0
  - y/n
  - yes/no	(new)
  - t/f		(new)
  - true/false  (new)
  - on/off

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 lib/kstrtox.c | 60 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 15 deletions(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..b8b950325097 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -325,9 +325,17 @@ EXPORT_SYMBOL(kstrtos8);
  * @s: input string
  * @res: result
  *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
- * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
- * pointed to by res is updated upon finding a match.
+ * This return return 0 on success, otherwise it will return -EINVAL.
+ * It will accept (case invariant):
+ *
+ *  - 1/0
+ *  - y/n
+ *  - yes/no
+ *  - t/f
+ *  - true/false
+ *  - on/off
+ *
+ * and set @*res to either true/false respectively.
  */
 int kstrtobool(const char *s, bool *res)
 {
@@ -335,30 +343,52 @@ int kstrtobool(const char *s, bool *res)
 		return -EINVAL;
 
 	switch (s[0]) {
+	case 't':
+	case 'T':
+		if (!s[1] || !strcasecmp(s, "true"))
+			goto have_true;
+
+		break;
+
 	case 'y':
 	case 'Y':
+		if (!s[1] || !strcasecmp(s, "yes"))
+			goto have_true;
+
+		break;
+
 	case '1':
+have_true:
 		*res = true;
 		return 0;
+
+	case 'f':
+	case 'F':
+		if (!s[1] || !strcasecmp(s, "false"))
+			goto have_false;
+
+		break;
 	case 'n':
 	case 'N':
+		if (!s[1] || !strcasecmp(s, "no"))
+			goto have_false;
+
+		break;
 	case '0':
+have_false:
 		*res = false;
 		return 0;
+
 	case 'o':
 	case 'O':
-		switch (s[1]) {
-		case 'n':
-		case 'N':
-			*res = true;
-			return 0;
-		case 'f':
-		case 'F':
-			*res = false;
-			return 0;
-		default:
-			break;
-		}
+		if (!strcasecmp(s, "on"))
+			goto have_true;
+
+		if (!strcasecmp(s, "off"))
+			goto have_false;
+
+		break;
+
 	default:
 		break;
 	}

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] lib: Extend kstrtobool() to accept "true"/"false"
  2020-06-26 10:44     ` [PATCH] lib: Extend kstrtobool() to accept "true"/"false" Peter Zijlstra
@ 2020-06-26 11:10       ` Srinivas Pandruvada
  2020-06-26 15:49       ` Rafael J. Wysocki
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Srinivas Pandruvada @ 2020-06-26 11:10 UTC (permalink / raw)
  To: Peter Zijlstra, Borislav Petkov
  Cc: rjw, viresh.kumar, lenb, dsmythies, tglx, mingo, hpa, linux-pm,
	linux-kernel, x86, jic23, keescook, akpm

On Fri, 2020-06-26 at 12:44 +0200, Peter Zijlstra wrote:
> On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote:
> 
> > > This is too lax - it will be enabled for any !0 value. Please
> > > accept
> > > only 0 and 1.
> > 
> > kstrtobool() ftw
> 
> And looking at that, I find it really strange it does not in fact
> accept
> "true" / "false", so how about this?
looks good.

Thanks,
Srinvas

> 
> ---
> Subject: lib: Extend kstrtobool() to accept "true"/"false"
> 
> Extend the strings recognised by kstrtobool() to cover:
> 
>   - 1/0
>   - y/n
>   - yes/no	(new)
>   - t/f		(new)
>   - true/false  (new)
>   - on/off
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  lib/kstrtox.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-----
> ----------
>  1 file changed, 45 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..b8b950325097 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -325,9 +325,17 @@ EXPORT_SYMBOL(kstrtos8);
>   * @s: input string
>   * @res: result
>   *
> - * This routine returns 0 iff the first character is one of
> 'Yy1Nn0', or
> - * [oO][NnFf] for "on" and "off". Otherwise it will return
> -EINVAL.  Value
> - * pointed to by res is updated upon finding a match.
> + * This return return 0 on success, otherwise it will return
> -EINVAL.
> + * It will accept (case invariant):
> + *
> + *  - 1/0
> + *  - y/n
> + *  - yes/no
> + *  - t/f
> + *  - true/false
> + *  - on/off
> + *
> + * and set @*res to either true/false respectively.
>   */
>  int kstrtobool(const char *s, bool *res)
>  {
> @@ -335,30 +343,52 @@ int kstrtobool(const char *s, bool *res)
>  		return -EINVAL;
>  
>  	switch (s[0]) {
> +	case 't':
> +	case 'T':
> +		if (!s[1] || !strcasecmp(s, "true"))
> +			goto have_true;
> +
> +		break;
> +
>  	case 'y':
>  	case 'Y':
> +		if (!s[1] || !strcasecmp(s, "yes"))
> +			goto have_true;
> +
> +		break;
> +
>  	case '1':
> +have_true:
>  		*res = true;
>  		return 0;
> +
> +	case 'f':
> +	case 'F':
> +		if (!s[1] || !strcasecmp(s, "false"))
> +			goto have_false;
> +
> +		break;
>  	case 'n':
>  	case 'N':
> +		if (!s[1] || !strcasecmp(s, "no"))
> +			goto have_false;
> +
> +		break;
>  	case '0':
> +have_false:
>  		*res = false;
>  		return 0;
> +
>  	case 'o':
>  	case 'O':
> -		switch (s[1]) {
> -		case 'n':
> -		case 'N':
> -			*res = true;
> -			return 0;
> -		case 'f':
> -		case 'F':
> -			*res = false;
> -			return 0;
> -		default:
> -			break;
> -		}
> +		if (!strcasecmp(s, "on"))
> +			goto have_true;
> +
> +		if (!strcasecmp(s, "off"))
> +			goto have_false;
> +
> +		break;
> +
>  	default:
>  		break;
>  	}


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] lib: Extend kstrtobool() to accept "true"/"false"
  2020-06-26 10:44     ` [PATCH] lib: Extend kstrtobool() to accept "true"/"false" Peter Zijlstra
  2020-06-26 11:10       ` Srinivas Pandruvada
@ 2020-06-26 15:49       ` Rafael J. Wysocki
  2020-06-26 15:51       ` Kees Cook
  2020-06-29 12:09       ` Pavel Machek
  3 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2020-06-26 15:49 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Borislav Petkov, Srinivas Pandruvada, Rafael J. Wysocki,
	Viresh Kumar, Len Brown, Doug Smythies, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, Linux PM, Linux Kernel Mailing List,
	the arch/x86 maintainers, jic23, Kees Cook, Andrew Morton

On Fri, Jun 26, 2020 at 12:45 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote:
>
> > > This is too lax - it will be enabled for any !0 value. Please accept
> > > only 0 and 1.
> >
> > kstrtobool() ftw
>
> And looking at that, I find it really strange it does not in fact accept
> "true" / "false", so how about this?
>
> ---
> Subject: lib: Extend kstrtobool() to accept "true"/"false"
>
> Extend the strings recognised by kstrtobool() to cover:
>
>   - 1/0
>   - y/n
>   - yes/no      (new)
>   - t/f         (new)
>   - true/false  (new)
>   - on/off
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  lib/kstrtox.c | 60 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..b8b950325097 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -325,9 +325,17 @@ EXPORT_SYMBOL(kstrtos8);
>   * @s: input string
>   * @res: result
>   *
> - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> - * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
> - * pointed to by res is updated upon finding a match.
> + * This return return 0 on success, otherwise it will return -EINVAL.
> + * It will accept (case invariant):
> + *
> + *  - 1/0
> + *  - y/n
> + *  - yes/no
> + *  - t/f
> + *  - true/false
> + *  - on/off
> + *
> + * and set @*res to either true/false respectively.
>   */
>  int kstrtobool(const char *s, bool *res)
>  {
> @@ -335,30 +343,52 @@ int kstrtobool(const char *s, bool *res)
>                 return -EINVAL;
>
>         switch (s[0]) {
> +       case 't':
> +       case 'T':
> +               if (!s[1] || !strcasecmp(s, "true"))
> +                       goto have_true;
> +
> +               break;
> +
>         case 'y':
>         case 'Y':
> +               if (!s[1] || !strcasecmp(s, "yes"))
> +                       goto have_true;
> +
> +               break;
> +
>         case '1':
> +have_true:
>                 *res = true;
>                 return 0;
> +
> +       case 'f':
> +       case 'F':
> +               if (!s[1] || !strcasecmp(s, "false"))
> +                       goto have_false;
> +
> +               break;
>         case 'n':
>         case 'N':
> +               if (!s[1] || !strcasecmp(s, "no"))
> +                       goto have_false;
> +
> +               break;
>         case '0':
> +have_false:
>                 *res = false;
>                 return 0;
> +
>         case 'o':
>         case 'O':
> -               switch (s[1]) {
> -               case 'n':
> -               case 'N':
> -                       *res = true;
> -                       return 0;
> -               case 'f':
> -               case 'F':
> -                       *res = false;
> -                       return 0;
> -               default:
> -                       break;
> -               }
> +               if (!strcasecmp(s, "on"))
> +                       goto have_true;
> +
> +               if (!strcasecmp(s, "off"))
> +                       goto have_false;
> +
> +               break;
> +
>         default:
>                 break;
>         }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] lib: Extend kstrtobool() to accept "true"/"false"
  2020-06-26 10:44     ` [PATCH] lib: Extend kstrtobool() to accept "true"/"false" Peter Zijlstra
  2020-06-26 11:10       ` Srinivas Pandruvada
  2020-06-26 15:49       ` Rafael J. Wysocki
@ 2020-06-26 15:51       ` Kees Cook
  2020-06-29 12:09       ` Pavel Machek
  3 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2020-06-26 15:51 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Borislav Petkov, Srinivas Pandruvada, rjw, viresh.kumar, lenb,
	dsmythies, tglx, mingo, hpa, linux-pm, linux-kernel, x86, jic23,
	akpm

On Fri, Jun 26, 2020 at 12:44:42PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote:
> 
> > > This is too lax - it will be enabled for any !0 value. Please accept
> > > only 0 and 1.
> > 
> > kstrtobool() ftw
> 
> And looking at that, I find it really strange it does not in fact accept
> "true" / "false", so how about this?
> 
> ---
> Subject: lib: Extend kstrtobool() to accept "true"/"false"
> 
> Extend the strings recognised by kstrtobool() to cover:
> 
>   - 1/0
>   - y/n
>   - yes/no	(new)
>   - t/f		(new)
>   - true/false  (new)
>   - on/off
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

There were some worries about dealing with unterminated strings when I
did the original conversion[1], but I think those all got fixed.

Reviewed-by: Kees Cook <keescook@chromium.org>

[1] https://lore.kernel.org/lkml/CAGXu5jJrFv5Y8Q_i3yFYBDmT0+pO05dS3ijB0gOn-huasxZWmA@mail.gmail.com/

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] lib: Extend kstrtobool() to accept "true"/"false"
  2020-06-26 10:44     ` [PATCH] lib: Extend kstrtobool() to accept "true"/"false" Peter Zijlstra
                         ` (2 preceding siblings ...)
  2020-06-26 15:51       ` Kees Cook
@ 2020-06-29 12:09       ` Pavel Machek
  2020-07-01  2:38         ` Andrew Morton
  3 siblings, 1 reply; 10+ messages in thread
From: Pavel Machek @ 2020-06-29 12:09 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Borislav Petkov, Srinivas Pandruvada, rjw, viresh.kumar, lenb,
	dsmythies, tglx, mingo, hpa, linux-pm, linux-kernel, x86, jic23,
	keescook, akpm

Hi!

> > > This is too lax - it will be enabled for any !0 value. Please accept
> > > only 0 and 1.
> > 
> > kstrtobool() ftw
> 
> And looking at that, I find it really strange it does not in fact accept
> "true" / "false", so how about this?
> 
> ---
> Subject: lib: Extend kstrtobool() to accept "true"/"false"
> 
> Extend the strings recognised by kstrtobool() to cover:
> 
>   - 1/0
>   - y/n
>   - yes/no	(new)
>   - t/f		(new)
>   - true/false  (new)
>   - on/off

Is it good idea to add more values there? It is easy to do, but... we don't want
people to use this by hand, and ideally everyone would just use 1/0...

I also see potential for confusion... as in echo off > enable_off_mode (ok, this is
with existing code, but...)

Plus, if programs learn to do "echo true > ..." they will stop working on older kernels.

Plus, this really should be documented somewhere, as it is kernel ABI.

IMO this does not need changing.

NAK.

Best regards,
									Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] lib: Extend kstrtobool() to accept "true"/"false"
  2020-06-29 12:09       ` Pavel Machek
@ 2020-07-01  2:38         ` Andrew Morton
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2020-07-01  2:38 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Peter Zijlstra, Borislav Petkov, Srinivas Pandruvada, rjw,
	viresh.kumar, lenb, dsmythies, tglx, mingo, hpa, linux-pm,
	linux-kernel, x86, jic23, keescook

On Mon, 29 Jun 2020 14:09:38 +0200 Pavel Machek <pavel@ucw.cz> wrote:

> > Extend the strings recognised by kstrtobool() to cover:
> > 
> >   - 1/0
> >   - y/n
> >   - yes/no	(new)
> >   - t/f		(new)
> >   - true/false  (new)
> >   - on/off
> 
> Is it good idea to add more values there? It is easy to do, but... we don't want
> people to use this by hand, and ideally everyone would just use 1/0...
> 
> I also see potential for confusion... as in echo off > enable_off_mode (ok, this is
> with existing code, but...)
> 
> Plus, if programs learn to do "echo true > ..." they will stop working on older kernels.

I'm inclined to agree with this, It is indeed an invitation to write
non-back-compatible userspace and it simply makes the kernel interface
more complex.


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 22:49 [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency Srinivas Pandruvada
2020-06-26  8:49 ` Borislav Petkov
2020-06-26  9:12   ` Srinivas Pandruvada
2020-06-26 10:22   ` Peter Zijlstra
2020-06-26 10:44     ` [PATCH] lib: Extend kstrtobool() to accept "true"/"false" Peter Zijlstra
2020-06-26 11:10       ` Srinivas Pandruvada
2020-06-26 15:49       ` Rafael J. Wysocki
2020-06-26 15:51       ` Kees Cook
2020-06-29 12:09       ` Pavel Machek
2020-07-01  2:38         ` Andrew Morton

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git