On Fri, 15 Jun 2018, Fenghua Yu wrote: > By default C0.2 is enabled so user wait can save more power but wakeup > time is slower. In some cases e.g. real time, user wants to disable C0.2 > so that user wait saves less power but wakeup time is faster.
Why is this default enabled? > A new "/sys/devices/system/cpu/cpuidle/umwait_disable_c0_2" file is > created to allow user to check if C0.2 is enabled or disabled and also > allow user to enable or disable C0.2. Value "1" in the file means C0.2 is > disabled. Value "0" means C0.2 is enabled. Can we please use straight forward positive logic and have a enable file? > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * umwait.c - control user wait Please remove these pointless file references. They get stale before its merged. > + * > + * Copyright (c) 2018, Intel Corporation. > + * Fenghua Yu <fenghua...@intel.com> > + */ > +/* > + * umwait.c adds control of user wait states that user enters through user > wait > + * instructions umwait or tpause. umwait.c adds something? > + */ > +#include <linux/cpu.h> > +#include <asm/msr.h> > + > +static int umwait_disable_c0_2; > +static DEFINE_MUTEX(umwait_lock); > + > +static ssize_t umwait_disable_c0_2_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return sprintf(buf, "%d\n", umwait_disable_c0_2); > +} > + > +static ssize_t umwait_disable_c0_2_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int disable_c0_2, cpu, ret; > + u32 msr_val; > + > + ret = kstrtou32(buf, 10, &disable_c0_2); > + if (ret) > + return ret; > + if (disable_c0_2 != 1 && disable_c0_2 != 0) > + return -EINVAL; > + > + mutex_lock(&umwait_lock); > + umwait_disable_c0_2 = disable_c0_2; > + /* > + * No global umwait maximum time limit (0 in bits 31-0). > + * Enable or disable C0.2 based on global setting (bit 0) on all CPUs. > + */ > + msr_val = umwait_disable_c0_2 & UMWAIT_CONTROL_C02_MASK; That mask is there because the variable can only have 0 and 1 as content.... > + for_each_online_cpu(cpu) > + wrmsr_on_cpu(cpu, MSR_IA32_UMWAIT_CONTROL, msr_val, 0); This lacks protection against CPU hotplug. > + mutex_unlock(&umwait_lock); > + > + return count; Thanks, tglx