[PATCH 2/2] HID: hid-sensor-custom: Process failure of sensor_hub_set_feature()
When user modifies a custom feature value and sensor_hub_set_feature() fails, return error. Reported-by: Abaci Robot Signed-off-by: Srinivas Pandruvada --- Replaces patch: HID: hid-sensor-custom: remove useless variable by Jiapeng Chong drivers/hid/hid-sensor-custom.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c index 2628bc53ed80..58b54b127cdf 100644 --- a/drivers/hid/hid-sensor-custom.c +++ b/drivers/hid/hid-sensor-custom.c @@ -406,6 +406,8 @@ static ssize_t store_value(struct device *dev, struct device_attribute *attr, report_id; ret = sensor_hub_set_feature(sensor_inst->hsdev, report_id, index, sizeof(value), ); + if (ret) + return ret; } else return -EINVAL; -- 2.27.0
[PATCH 1/2] HID: hid-sensor-hub: Return error for hid_set_field() failure
In the function sensor_hub_set_feature(), return error when hid_set_field()
fails.
Signed-off-by: Srinivas Pandruvada
---
drivers/hid/hid-sensor-hub.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 3dd7d3246737..f9983145d4e7 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -210,16 +210,21 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device
*hsdev, u32 report_id,
buffer_size = buffer_size / sizeof(__s32);
if (buffer_size) {
for (i = 0; i < buffer_size; ++i) {
- hid_set_field(report->field[field_index], i,
- (__force __s32)cpu_to_le32(*buf32));
+ ret = hid_set_field(report->field[field_index], i,
+ (__force __s32)cpu_to_le32(*buf32));
+ if (ret)
+ goto done_proc;
+
++buf32;
}
}
if (remaining_bytes) {
value = 0;
memcpy(, (u8 *)buf32, remaining_bytes);
- hid_set_field(report->field[field_index], i,
- (__force __s32)cpu_to_le32(value));
+ ret = hid_set_field(report->field[field_index], i,
+ (__force __s32)cpu_to_le32(value));
+ if (ret)
+ goto done_proc;
}
hid_hw_request(hsdev->hdev, report, HID_REQ_SET_REPORT);
hid_hw_wait(hsdev->hdev);
--
2.27.0
Re: [PATCH] iio: hid-sensors: select IIO_TRIGGERED_BUFFER under HID_SENSOR_IIO_TRIGGER
On Wed, 2021-04-14 at 11:49 +0300, Alexandru Ardelean wrote:
> During commit 067fda1c065ff ("iio: hid-sensors: move triggered buffer
> setup into hid_sensor_setup_trigger"), the
> iio_triggered_buffer_{setup,cleanup}() functions got moved under the
> hid-sensor-trigger module.
>
> The above change works fine, if any of the sensors get built.
> However, when
> only the common hid-sensor-trigger module gets built (and none of the
> drivers), then the IIO_TRIGGERED_BUFFER symbol isn't
> selected/enforced.
>
> Previously, each driver would enforce/select the IIO_TRIGGERED_BUFFER
> symbol. With this change the HID_SENSOR_IIO_TRIGGER (for the
> hid-sensor-trigger module) will enforce that IIO_TRIGGERED_BUFFER
> gets
> selected.
>
> All HID sensor drivers select the HID_SENSOR_IIO_TRIGGER symbol. So,
> this
> change removes the IIO_TRIGGERED_BUFFER enforcement from each driver.
>
> Fixes: 067fda1c065ff ("iio: hid-sensors: move triggered buffer setup
> into hid_sensor_setup_trigger")
> Reported-by: Thomas Deutschmann
> Cc: Srinivas Pandruvada
> Signed-off-by: Alexandru Ardelean
Acked-by: Srinivas Pandruvada
> ---
> drivers/iio/accel/Kconfig | 1 -
> drivers/iio/common/hid-sensors/Kconfig | 1 +
> drivers/iio/gyro/Kconfig | 1 -
> drivers/iio/humidity/Kconfig | 1 -
> drivers/iio/light/Kconfig | 2 --
> drivers/iio/magnetometer/Kconfig | 1 -
> drivers/iio/orientation/Kconfig| 2 --
> drivers/iio/pressure/Kconfig | 1 -
> drivers/iio/temperature/Kconfig| 1 -
> 9 files changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index cceda3cecbcf..8b1723635cce 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -229,7 +229,6 @@ config DMARD10
> config HID_SENSOR_ACCEL_3D
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> - select IIO_TRIGGERED_BUFFER
> select HID_SENSOR_IIO_COMMON
> select HID_SENSOR_IIO_TRIGGER
> tristate "HID Accelerometers 3D"
> diff --git a/drivers/iio/common/hid-sensors/Kconfig
> b/drivers/iio/common/hid-sensors/Kconfig
> index 24d492567336..2a3dd3b907be 100644
> --- a/drivers/iio/common/hid-sensors/Kconfig
> +++ b/drivers/iio/common/hid-sensors/Kconfig
> @@ -19,6 +19,7 @@ config HID_SENSOR_IIO_TRIGGER
> tristate "Common module (trigger) for all HID Sensor IIO
> drivers"
> depends on HID_SENSOR_HUB && HID_SENSOR_IIO_COMMON &&
> IIO_BUFFER
> select IIO_TRIGGER
> + select IIO_TRIGGERED_BUFFER
> help
> Say yes here to build trigger support for HID sensors.
> Triggers will be send if all requested attributes were read.
> diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
> index 5824f2edf975..20b5ac7ab66a 100644
> --- a/drivers/iio/gyro/Kconfig
> +++ b/drivers/iio/gyro/Kconfig
> @@ -111,7 +111,6 @@ config FXAS21002C_SPI
> config HID_SENSOR_GYRO_3D
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> - select IIO_TRIGGERED_BUFFER
> select HID_SENSOR_IIO_COMMON
> select HID_SENSOR_IIO_TRIGGER
> tristate "HID Gyroscope 3D"
> diff --git a/drivers/iio/humidity/Kconfig
> b/drivers/iio/humidity/Kconfig
> index 6549fcf6db69..2de5494e7c22 100644
> --- a/drivers/iio/humidity/Kconfig
> +++ b/drivers/iio/humidity/Kconfig
> @@ -52,7 +52,6 @@ config HID_SENSOR_HUMIDITY
> tristate "HID Environmental humidity sensor"
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> - select IIO_TRIGGERED_BUFFER
> select HID_SENSOR_IIO_COMMON
> select HID_SENSOR_IIO_TRIGGER
> help
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 33ad4dd0b5c7..917f9becf9c7 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -256,7 +256,6 @@ config ISL29125
> config HID_SENSOR_ALS
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> - select IIO_TRIGGERED_BUFFER
> select HID_SENSOR_IIO_COMMON
> select HID_SENSOR_IIO_TRIGGER
> tristate "HID ALS"
> @@ -270,7 +269,6 @@ config HID_SENSOR_ALS
> config HID_SENSOR_PROX
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> - select IIO_TRIGGERED_BUFFER
> select HID_SENSOR_IIO_COMMON
> select HID_SENSOR_IIO_TRIGGER
> tristate "HID PROX"
> diff --git a/drivers/iio/magnetometer/Kconfig
> b/drivers/iio/magnetometer/Kconfig
> index 5d4ffd66032e..74ad5701c6c2 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/mag
Re: [thermal] 9223d0dccb: stress-ng.msg.ops_per_sec -27.4% regression
On Tue, 2021-04-13 at 21:58 +0800, kernel test robot wrote:
>
> Greeting,
>
> FYI, we noticed a -27.4% regression of stress-ng.msg.ops_per_sec due
> to commit:
>
>
> commit: 9223d0dccb8f8523754122f68316dd1a4f39f7f8 ("thermal: Move
> therm_throt there from x86/mce")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git
> master
>
>
> in testcase: stress-ng
> on test machine: 96 threads Intel(R) Xeon(R) Gold 6252 CPU @ 2.10GHz
> with 192G memory
> with following parameters:
>
> nr_threads: 10%
> disk: 1HDD
> testtime: 60s
> fs: ext4
> class: os
> test: msg
> cpufreq_governor: performance
> ucode: 0x5003006
>
>
>
Is this reproducible? This patch doesn't set any value which restricts
performance. This driver reports the threshold violations.
It is possible that somehow we reached a thermal limit where we got
many interrupts in one run.
Thanks.
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot
>
>
> Details are as below:
> ---
> --->
>
>
> To reproduce:
>
> git clone https://github.com/intel/lkp-tests.git
> cd lkp-tests
> bin/lkp installjob.yaml # job file is
> attached in this email
> bin/lkp split-job --compatible job.yaml
> bin/lkp runcompatible-job.yaml
>
> =
>
> class/compiler/cpufreq_governor/disk/fs/kconfig/nr_threads/rootfs/tbo
> x_group/test/testcase/testtime/ucode:
> os/gcc-9/performance/1HDD/ext4/x86_64-rhel-8.3/10%/debian-10.4-
> x86_64-20200603.cgz/lkp-csl-2sp5/msg/stress-ng/60s/0x5003006
>
> commit:
> 4f432e8bb1 ("x86/mce: Get rid of mcheck_intel_therm_init()")
> 9223d0dccb ("thermal: Move therm_throt there from x86/mce")
>
> 4f432e8bb15b352d 9223d0dccb8f8523754122f6831
> ---
> %stddev %change %stddev
> \ |\
> 3.042e+08 ± 4% -27.4% 2.209e+08 ± 2% stress-ng.msg.ops
>5069470 ± 4% -27.4%3682272 ± 2% stress-
> ng.msg.ops_per_sec
> 2335 ± 8% -35.0% 1517 ± 18% stress-
> ng.time.involuntary_context_switches
> 1676-4.9% 1594stress-
> ng.time.percent_of_cpu_this_job_got
> 1019-4.6% 973.25stress-
> ng.time.system_time
> 20.97 ± 2% -19.3% 16.93 ± 2% stress-
> ng.time.user_time
>1549846 ±159% +1738.5% 28494120 ± 16% stress-
> ng.time.voluntary_context_switches
>2050707 ±134% +1531.7% 33461885 ± 21% cpuidle.POLL.time
>1354230 ±171% +1585.0% 22819237 ± 19% cpuidle.POLL.usage
> 82.39+1.2% 83.37iostat.cpu.idle
> 17.20-5.3% 16.29iostat.cpu.system
> 0.04 ± 5% +0.00.05 ± 3% mpstat.cpu.all.soft%
> 0.37 ± 2% -0.10.30 ± 2% mpstat.cpu.all.usr%
>1337633 ± 28% -65.8% 457460 ± 95% numa-
> numastat.node0.local_node
> 456832 ± 26% -67.5% 148621 ± 25% numa-
> numastat.node1.local_node
> 500231 ± 26% -64.1% 179602 ± 12% numa-
> numastat.node1.numa_hit
> 82.00+1.2% 83.00vmstat.cpu.id
> 49550 ±153% +1673.1% 878579 ± 16% vmstat.system.cs
> 197941 +26.4% 250229 ± 6% vmstat.system.in
> 41605-1.4% 41020proc-
> vmstat.nr_slab_unreclaimable
>1907890 ± 15% -62.0% 724187 ± 63% proc-vmstat.numa_hit
>1821249 ± 16% -65.0% 637560 ± 71% proc-vmstat.numa_local
>1925858 ± 15% -61.7% 737459 ± 62% proc-
> vmstat.pgalloc_normal
>1796629 ± 16% -66.1% 609149 ± 75% proc-vmstat.pgfree
> 9427 ± 3% -11.6% 8337 ± 2% slabinfo.kmalloc-
> 512.active_objs
> 1182 ± 3% -11.5% 1045 ± 2% slabinfo.kmalloc-
> 512.active_slabs
> 9459 ± 3% -11.5% 8370 ± 2% slabinfo.kmalloc-
> 512.num_objs
> 1182 ± 3% -11.5% 1045 ± 2% slabinfo.kmalloc-
> 512.num_slabs
> 82005 ± 7% -24.2% 62197 ± 5% slabinfo.kmalloc-
> 64.active_objs
> 1282 ± 7% -24.1% 973.67 ± 5% slabinfo.kmalloc-
> 64.active_slabs
> 82119 ± 7% -24.1% 62339 ± 5% slabinfo.kmalloc-
> 64.num_objs
> 1282 ± 7% -24.1% 973.67 ± 5% slabinfo.kmalloc-
> 64.num_slabs
> 8300 ± 11% +41.5% 11747 ± 10% softirqs.CPU21.SCHED
> 6707 ± 23% +39.9% 9384 ± 15% softirqs.CPU41.SCHED
> 8143 ± 17% +50.5% 12255 ± 27% softirqs.CPU56.SCHED
> 8052 ± 14% +82.7% 14708 ± 29% softirqs.CPU58.SCHED
> 8701 ± 19% +70.6% 14847 ± 23% softirqs.CPU6.SCHED
> 6902 ± 18% +50.7% 10401 ± 18% softirqs.CPU72.SCHED
>
Re: [PATCH] HID: hid-sensor-custom: remove useless variable
On Mon, 2021-04-12 at 11:20 +0100, Jonathan Cameron wrote:
> On Sun, 11 Apr 2021 09:06:35 -0700
> Srinivas Pandruvada wrote:
>
> > On Sun, 2021-04-11 at 14:56 +0100, Jonathan Cameron wrote:
> > > On Fri, 09 Apr 2021 11:19:12 -0700
> > > Srinivas Pandruvada wrote:
> > >
> > > > On Fri, 2021-04-09 at 15:15 +0800, Jiapeng Chong wrote:
> > > > > Fix the following gcc warning:
> > > > >
> > > > > drivers/hid/hid-sensor-custom.c:400:7: warning: variable
> > > > > ‘ret’
> > > > > set
> > > > > but
> > > > > not used [-Wunused-but-set-variable].
> > > > >
> > > > > Reported-by: Abaci Robot
> > > > > Signed-off-by: Jiapeng Chong > > > > >
> > > > Acked-by: Srinivas Pandruvada <
> > > > srinivas.pandruv...@linux.intel.com
> > >
> > > Perhaps better to return ret if it is non zero?
> > > I can't immediately figure out if there is a reason we know that
> > > can't
> > > happen.
> > Only time it can fail when there is no report descriptor or the
> > field
> > index is >= report->maxfield.
> > But since the attribute is registered from the report descriptor
> > and
> > index, this can't happen.
> > But we can enhance sensor_hub_set_feature() to fail when
> > hid_set_field() fails. There is one case where field-
> > >logical_minimum
> > < 0 and value is out of range.
>
> I'll go with what you think. Apply as is, or handle the
> return value because we might at some later date return an error that
> can actually happen from here?
>
I will send you a patch this week to handle error condition.
Thanks,
Srinivas
> Jonathan
>
> > Thanks,
> > Srinivas
> >
> >
> > > Jonathan
> > >
> > > > > ---
> > > > > drivers/hid/hid-sensor-custom.c | 5 ++---
> > > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-sensor-custom.c
> > > > > b/drivers/hid/hid-
> > > > > sensor-custom.c
> > > > > index 2628bc5..e430673 100644
> > > > > --- a/drivers/hid/hid-sensor-custom.c
> > > > > +++ b/drivers/hid/hid-sensor-custom.c
> > > > > @@ -397,15 +397,14 @@ static ssize_t store_value(struct
> > > > > device
> > > > > *dev,
> > > > > struct device_attribute *attr,
> > > > >
> > > > > if (!strncmp(name, "value", strlen("value"))) {
> > > > > u32 report_id;
> > > > > - int ret;
> > > > >
> > > > > if (kstrtoint(buf, 0, ) != 0)
> > > > > return -EINVAL;
> > > > >
> > > > > report_id = sensor_inst-
> > > > > >fields[field_index].attribute.
> > > > >
> > > > > report_
> > > > > id;
> > > > > - ret = sensor_hub_set_feature(sensor_inst-
> > > > > >hsdev,
> > > > > report_id,
> > > > > - index,
> > > > > sizeof(value),
> > > > > );
> > > > > + sensor_hub_set_feature(sensor_inst->hsdev,
> > > > > report_id,
> > > > > index,
> > > > > +sizeof(value), );
> > > > > } else
> > > > > return -EINVAL;
> > > > >
Re: [PATCH] HID: hid-sensor-custom: remove useless variable
On Sun, 2021-04-11 at 14:56 +0100, Jonathan Cameron wrote:
> On Fri, 09 Apr 2021 11:19:12 -0700
> Srinivas Pandruvada wrote:
>
> > On Fri, 2021-04-09 at 15:15 +0800, Jiapeng Chong wrote:
> > > Fix the following gcc warning:
> > >
> > > drivers/hid/hid-sensor-custom.c:400:7: warning: variable ‘ret’
> > > set
> > > but
> > > not used [-Wunused-but-set-variable].
> > >
> > > Reported-by: Abaci Robot
> > > Signed-off-by: Jiapeng Chong
> > Acked-by: Srinivas Pandruvada
> Perhaps better to return ret if it is non zero?
> I can't immediately figure out if there is a reason we know that
> can't
> happen.
Only time it can fail when there is no report descriptor or the field
index is >= report->maxfield.
But since the attribute is registered from the report descriptor and
index, this can't happen.
But we can enhance sensor_hub_set_feature() to fail when
hid_set_field() fails. There is one case where field->logical_minimum
< 0 and value is out of range.
Thanks,
Srinivas
>
> Jonathan
>
> > > ---
> > > drivers/hid/hid-sensor-custom.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> > > sensor-custom.c
> > > index 2628bc5..e430673 100644
> > > --- a/drivers/hid/hid-sensor-custom.c
> > > +++ b/drivers/hid/hid-sensor-custom.c
> > > @@ -397,15 +397,14 @@ static ssize_t store_value(struct device
> > > *dev,
> > > struct device_attribute *attr,
> > >
> > > if (!strncmp(name, "value", strlen("value"))) {
> > > u32 report_id;
> > > - int ret;
> > >
> > > if (kstrtoint(buf, 0, ) != 0)
> > > return -EINVAL;
> > >
> > > report_id = sensor_inst->fields[field_index].attribute.
> > > report_
> > > id;
> > > - ret = sensor_hub_set_feature(sensor_inst->hsdev,
> > > report_id,
> > > - index, sizeof(value),
> > > );
> > > + sensor_hub_set_feature(sensor_inst->hsdev, report_id,
> > > index,
> > > +sizeof(value), );
> > > } else
> > > return -EINVAL;
> > >
Re: [PATCH] HID: hid-sensor-custom: remove useless variable
On Fri, 2021-04-09 at 15:15 +0800, Jiapeng Chong wrote:
> Fix the following gcc warning:
>
> drivers/hid/hid-sensor-custom.c:400:7: warning: variable ‘ret’ set
> but
> not used [-Wunused-but-set-variable].
>
> Reported-by: Abaci Robot
> Signed-off-by: Jiapeng Chong
Acked-by: Srinivas Pandruvada ---
> drivers/hid/hid-sensor-custom.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index 2628bc5..e430673 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -397,15 +397,14 @@ static ssize_t store_value(struct device *dev,
> struct device_attribute *attr,
>
> if (!strncmp(name, "value", strlen("value"))) {
> u32 report_id;
> - int ret;
>
> if (kstrtoint(buf, 0, ) != 0)
> return -EINVAL;
>
> report_id = sensor_inst->fields[field_index].attribute.
> report_
> id;
> - ret = sensor_hub_set_feature(sensor_inst->hsdev,
> report_id,
> - index, sizeof(value),
> );
> + sensor_hub_set_feature(sensor_inst->hsdev, report_id,
> index,
> +sizeof(value), );
> } else
> return -EINVAL;
>
Re: [PATCH] HID: intel-ish-hid: Remove an unused variable 'ret'
On Fri, 2021-04-02 at 18:06 +0800, Yang Li wrote:
> Fixes the following W=1 kernel build warning(s):
>
> drivers/hid/intel-ish-hid/ipc/pci-ish.c:264:6: warning: variable
> ‘ret’
> set but not used
>
> Reported-by: Abaci Robot
> Signed-off-by: Yang Li
Acked-by: Srinivas Pandruvada
> ---
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 06081cf..61efc30 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -261,7 +261,6 @@ static void __maybe_unused
> ish_resume_handler(struct work_struct *work)
> struct pci_dev *pdev = to_pci_dev(ish_resume_device);
> struct ishtp_device *dev = pci_get_drvdata(pdev);
> uint32_t fwsts = dev->ops->get_fw_status(dev);
> - int ret;
>
> if (ish_should_leave_d0i3(pdev) && !dev->suspend_flag
> && IPC_IS_ISH_ILUP(fwsts)) {
> @@ -273,7 +272,7 @@ static void __maybe_unused
> ish_resume_handler(struct work_struct *work)
>
> /* Waiting to get resume response */
> if (dev->resume_flag)
> - ret = wait_event_interruptible_timeout(dev-
> >resume_wait,
> + wait_event_interruptible_timeout(dev-
> >resume_wait,
> !dev->resume_flag,
> msecs_to_jiffies(WAIT_FOR_RESUME_ACK_MS
> ));
>
[PATCH] platform/x86: ISST: Account for increased timeout in some cases
In some cases when firmware is busy or updating, some mailbox commands
still timeout on some newer CPUs. To fix this issue, change how we
process timeout.
With this change, replaced timeout from using simple count with real
timeout in micro-seconds using ktime. When the command response takes
more than average processing time, yield to other tasks. The worst case
timeout is extended upto 1 milli-second.
Signed-off-by: Srinivas Pandruvada
---
.../intel_speed_select_if/isst_if_mbox_pci.c | 33 +--
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
index a2a2d923e60c..df1fc6c719f3 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
@@ -21,12 +21,16 @@
#define PUNIT_MAILBOX_BUSY_BIT 31
/*
- * The average time to complete some commands is about 40us. The current
- * count is enough to satisfy 40us. But when the firmware is very busy, this
- * causes timeout occasionally. So increase to deal with some worst case
- * scenarios. Most of the command still complete in few us.
+ * The average time to complete mailbox commands is less than 40us. Most of
+ * the commands complete in few micro seconds. But the same firmware handles
+ * requests from all power management features.
+ * We can create a scenario where we flood the firmware with requests then
+ * the mailbox response can be delayed for 100s of micro seconds. So define
+ * two timeouts. One for average case and one for long.
+ * If the firmware is taking more than average, just call cond_resched().
*/
-#define OS_MAILBOX_RETRY_COUNT 100
+#define OS_MAILBOX_TIMEOUT_AVG_US 40
+#define OS_MAILBOX_TIMEOUT_MAX_US 1000
struct isst_if_device {
struct mutex mutex;
@@ -35,11 +39,13 @@ struct isst_if_device {
static int isst_if_mbox_cmd(struct pci_dev *pdev,
struct isst_if_mbox_cmd *mbox_cmd)
{
- u32 retries, data;
+ s64 tm_delta = 0;
+ ktime_t tm;
+ u32 data;
int ret;
/* Poll for rb bit == 0 */
- retries = OS_MAILBOX_RETRY_COUNT;
+ tm = ktime_get();
do {
ret = pci_read_config_dword(pdev, PUNIT_MAILBOX_INTERFACE,
);
@@ -48,11 +54,14 @@ static int isst_if_mbox_cmd(struct pci_dev *pdev,
if (data & BIT_ULL(PUNIT_MAILBOX_BUSY_BIT)) {
ret = -EBUSY;
+ tm_delta = ktime_us_delta(ktime_get(), tm);
+ if (tm_delta > OS_MAILBOX_TIMEOUT_AVG_US)
+ cond_resched();
continue;
}
ret = 0;
break;
- } while (--retries);
+ } while (tm_delta < OS_MAILBOX_TIMEOUT_MAX_US);
if (ret)
return ret;
@@ -74,7 +83,8 @@ static int isst_if_mbox_cmd(struct pci_dev *pdev,
return ret;
/* Poll for rb bit == 0 */
- retries = OS_MAILBOX_RETRY_COUNT;
+ tm_delta = 0;
+ tm = ktime_get();
do {
ret = pci_read_config_dword(pdev, PUNIT_MAILBOX_INTERFACE,
);
@@ -83,6 +93,9 @@ static int isst_if_mbox_cmd(struct pci_dev *pdev,
if (data & BIT_ULL(PUNIT_MAILBOX_BUSY_BIT)) {
ret = -EBUSY;
+ tm_delta = ktime_us_delta(ktime_get(), tm);
+ if (tm_delta > OS_MAILBOX_TIMEOUT_AVG_US)
+ cond_resched();
continue;
}
@@ -96,7 +109,7 @@ static int isst_if_mbox_cmd(struct pci_dev *pdev,
mbox_cmd->resp_data = data;
ret = 0;
break;
- } while (--retries);
+ } while (tm_delta < OS_MAILBOX_TIMEOUT_MAX_US);
return ret;
}
--
2.25.4
Re: [PATCH] thermal/drivers/netlink: Add the temperature when crossing a trip point
Hi Daniel, On Sat, 2021-03-27 at 21:25 +0100, Daniel Lezcano wrote: > Hi Srinivas, > > On 27/03/2021 18:46, Srinivas Pandruvada wrote: > > On Fri, 2021-03-26 at 17:37 +0100, Rafael J. Wysocki wrote: > > > On Thu, Mar 25, 2021 at 8:38 PM Daniel Lezcano > > > wrote: > > > > The slope of the temperature increase or decrease can be high > > > > and > > > > when > > > > the temperature crosses the trip point, there could be a > > > > significant > > > > difference between the trip temperature and the measured > > > > temperatures. > > > > > > > > That forces the userspace to read the temperature back right > > > > after > > > > receiving a trip violation notification. > > > > > > > > In order to be efficient, give the temperature which resulted > > > > in > > > > the > > > > trip violation. > > > > > > > > Signed-off-by: Daniel Lezcano > > > > > > Srinivas, what do you think? > > > > - IMO netlink message should also be treated as we treat other > > ABIs. So > > add only when this is a must. Although here GENL version is > > incremented > > , users are not that careful. At least on x86, we know users > > created > > their own applications. > > I don't see where is the problem. The protocol is still compatible > with > the previous version, so that does not break the existing AFAICT. > That > is done on purpose. The size of netlink message is changed. This is not a good argument to just adding members at the end. The point I am trying that netlink now is an ABI, which should go through same process as we are adding/changing a sysfs attributes. > > There is a new attribute added, the application using the previous > version will just not be aware of its presence and parse the message > without getting the temperature. > > > - Here the concern is temperature is changing so fast then netlink > > + > > user space processing latency is enough to change further to read > > temperature again. Atleast we assume that and read temperature > > again. > > So not sure that this is the right approach to add another field > > for > > the temperature. > > I'm not sure to understand your comment. Whatever the mechanism > (interrupt based or polling), the temperature is read in any case by > the > call to thermal_zone_device_update() which in turns calls > handle_trip_point and then send the message. So, why not add the > temperature as the userspace is interested on getting the temperature > anyway. > For instance, in polling mode, (usually set to 1000ms), the trip > point > crossing is detected with a long delay and during this time the > temperature could be far beyond the trip temperature. > cosnumer > It is not about netlink latency but about avoiding a back and forth > when > the trip point is crossed and the temperature wavering around. That is the reasoning given to add this change in the description that slope of change is high so user is forced to read temperature again as the trip temp +- current temp is significant. If slope is very high then, when the temperature passed in the messages is already stale in few ms, so the user space will still end up in re-reading again. The reason you want to add temperature is that you sampled the latest temperature during thermal_zone_device_update(), which may be different than the trip temperature. So provide temperature also to user space consumer, so it can decide whether to use as is or re-read sample. This change will save one additional read from user space in some cases and also allow disable notifications of trip if the temperature is hovering around trip. This is not to say that user can space totally can avoid another read. Then fine with me. Thanks, Srinivas > > > > > > --- > > > > drivers/thermal/thermal_core.c| 6 -- > > > > drivers/thermal/thermal_netlink.c | 11 ++- > > > > drivers/thermal/thermal_netlink.h | 8 > > > > include/uapi/linux/thermal.h | 2 +- > > > > 4 files changed, 15 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/thermal/thermal_core.c > > > > b/drivers/thermal/thermal_core.c > > > > index 996c038f83a4..948020ef51b1 100644 > > > > --- a/drivers/thermal/thermal_core.c > > > > +++ b/drivers/thermal/thermal_core.c > > > > @@ -430,10 +430,12 @@ static void handle_thermal_trip(struct > > > > thermal_zone_device *tz, int trip) > > > >
Re: [PATCH] thermal/drivers/netlink: Add the temperature when crossing a trip point
On Fri, 2021-03-26 at 17:37 +0100, Rafael J. Wysocki wrote:
> On Thu, Mar 25, 2021 at 8:38 PM Daniel Lezcano
> wrote:
> > The slope of the temperature increase or decrease can be high and
> > when
> > the temperature crosses the trip point, there could be a
> > significant
> > difference between the trip temperature and the measured
> > temperatures.
> >
> > That forces the userspace to read the temperature back right after
> > receiving a trip violation notification.
> >
> > In order to be efficient, give the temperature which resulted in
> > the
> > trip violation.
> >
> > Signed-off-by: Daniel Lezcano
>
> Srinivas, what do you think?
- IMO netlink message should also be treated as we treat other ABIs. So
add only when this is a must. Although here GENL version is incremented
, users are not that careful. At least on x86, we know users created
their own applications.
- Here the concern is temperature is changing so fast then netlink +
user space processing latency is enough to change further to read
temperature again. Atleast we assume that and read temperature again.
So not sure that this is the right approach to add another field for
the temperature.
Thanks,
Srinivas
>
> > ---
> > drivers/thermal/thermal_core.c| 6 --
> > drivers/thermal/thermal_netlink.c | 11 ++-
> > drivers/thermal/thermal_netlink.h | 8
> > include/uapi/linux/thermal.h | 2 +-
> > 4 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 996c038f83a4..948020ef51b1 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -430,10 +430,12 @@ static void handle_thermal_trip(struct
> > thermal_zone_device *tz, int trip)
> > if (tz->last_temperature != THERMAL_TEMP_INVALID) {
> > if (tz->last_temperature < trip_temp &&
> > tz->temperature >= trip_temp)
> > - thermal_notify_tz_trip_up(tz->id, trip);
> > + thermal_notify_tz_trip_up(tz->id, trip,
> > + tz->temperature);
> > if (tz->last_temperature >= trip_temp &&
> > tz->temperature < (trip_temp - hyst))
> > - thermal_notify_tz_trip_down(tz->id, trip);
> > + thermal_notify_tz_trip_down(tz->id, trip,
> > + tz-
> > >temperature);
> > }
> >
> > if (type == THERMAL_TRIP_CRITICAL || type ==
> > THERMAL_TRIP_HOT)
> > diff --git a/drivers/thermal/thermal_netlink.c
> > b/drivers/thermal/thermal_netlink.c
> > index 1234dbe95895..a16dd4d5d710 100644
> > --- a/drivers/thermal/thermal_netlink.c
> > +++ b/drivers/thermal/thermal_netlink.c
> > @@ -121,7 +121,8 @@ static int thermal_genl_event_tz(struct param
> > *p)
> > static int thermal_genl_event_tz_trip_up(struct param *p)
> > {
> > if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id)
> > ||
> > - nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_ID, p-
> > >trip_id))
> > + nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_ID, p-
> > >trip_id) ||
> > + nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TEMP, p-
> > >temp))
> > return -EMSGSIZE;
> >
> > return 0;
> > @@ -285,16 +286,16 @@ int thermal_notify_tz_disable(int tz_id)
> > return
> > thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_DISABLE, );
> > }
> >
> > -int thermal_notify_tz_trip_down(int tz_id, int trip_id)
> > +int thermal_notify_tz_trip_down(int tz_id, int trip_id, int temp)
> > {
> > - struct param p = { .tz_id = tz_id, .trip_id = trip_id };
> > + struct param p = { .tz_id = tz_id, .trip_id = trip_id,
> > .temp = temp };
> >
> > return
> > thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DOWN, );
> > }
> >
> > -int thermal_notify_tz_trip_up(int tz_id, int trip_id)
> > +int thermal_notify_tz_trip_up(int tz_id, int trip_id, int temp)
> > {
> > - struct param p = { .tz_id = tz_id, .trip_id = trip_id };
> > + struct param p = { .tz_id = tz_id, .trip_id = trip_id,
> > .temp = temp };
> >
> > return
> > thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_UP, );
> > }
> > diff --git a/drivers/thermal/thermal_netlink.h
> > b/drivers/thermal/thermal_netlink.h
> > index 828d1dddfa98..e554f76291f4 100644
> > --- a/drivers/thermal/thermal_netlink.h
> > +++ b/drivers/thermal/thermal_netlink.h
> > @@ -11,8 +11,8 @@ int thermal_notify_tz_create(int tz_id, const
> > char *name);
> > int thermal_notify_tz_delete(int tz_id);
> > int thermal_notify_tz_enable(int tz_id);
> > int thermal_notify_tz_disable(int tz_id);
> > -int thermal_notify_tz_trip_down(int tz_id, int id);
> > -int thermal_notify_tz_trip_up(int tz_id, int id);
> > +int thermal_notify_tz_trip_down(int tz_id, int id, int temp);
> > +int thermal_notify_tz_trip_up(int tz_id,
Re: [PATCH 00/25] Rid W=1 warnings from HID
On Wed, 2021-03-24 at 17:33 +, Lee Jones wrote: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > For changes in drivers/hid/intel-ish-hid folder Acked-by: Srinivas Pandruvada > Lee Jones (25): > HID: intel-ish-hid: Remove unused variable 'err' > HID: ishtp-hid-client: Move variable to where it's actually used > HID: intel-ish-hid: pci-ish: Remove unused variable 'ret' > HID: intel-ish: Supply some missing param descriptions > HID: intel-ish: Fix a naming disparity and a formatting error > HID: usbhid: Repair a formatting issue in a struct description > HID: intel-ish-hid: Fix a little doc-rot > HID: usbhid: hid-pidff: Demote a couple kernel-doc abuses > HID: hid-alps: Correct struct misnaming > HID: intel-ish-hid: Fix potential copy/paste error > HID: hid-core: Fix incorrect function name in header > HID: intel-ish-hid: ipc: Correct fw_reset_work_fn() function name > in > header > HID: ishtp-hid-client: Fix incorrect function name > report_bad_packet() > HID: hid-kye: Fix incorrect function name for kye_tablet_enable() > HID: hid-picolcd_core: Remove unused variable 'ret' > HID: hid-logitech-hidpp: Fix conformant kernel-doc header and > demote > abuses > HID: hid-uclogic-rdesc: Kernel-doc is for functions and structs > HID: hid-thrustmaster: Demote a bunch of kernel-doc abuses > HID: hid-uclogic-params: Ensure function names are present and > correct > in kernel-doc headers > HID: hid-sensor-custom: Remove unused variable 'ret' > HID: wacom_sys: Demote kernel-doc abuse > HID: hid-sensor-hub: Remove unused struct member 'quirks' > HID: hid-sensor-hub: Move 'hsdev' description to correct struct > definition > HID: intel-ish-hid: ishtp-fw-loader: Fix a bunch of formatting > issues > HID: ishtp-hid-client: Fix 'suggest-attribute=format' compiler > warning > > drivers/hid/hid-alps.c | 2 +- > drivers/hid/hid-core.c | 2 +- > drivers/hid/hid-kye.c| 2 +- > drivers/hid/hid-logitech-hidpp.c | 7 +-- > drivers/hid/hid-picolcd_core.c | 5 +-- > drivers/hid/hid-sensor-custom.c | 5 +-- > drivers/hid/hid-sensor-hub.c | 4 +- > drivers/hid/hid-thrustmaster.c | 24 +-- > drivers/hid/hid-uclogic-params.c | 8 ++-- > drivers/hid/hid-uclogic-rdesc.c | 2 +- > drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- > drivers/hid/intel-ish-hid/ipc/pci-ish.c | 3 +- > drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 45 ++-- > > drivers/hid/intel-ish-hid/ishtp-hid-client.c | 11 +++-- > drivers/hid/intel-ish-hid/ishtp-hid.c| 2 +- > drivers/hid/intel-ish-hid/ishtp-hid.h| 9 +--- > drivers/hid/intel-ish-hid/ishtp/bus.c| 9 +++- > drivers/hid/intel-ish-hid/ishtp/client.c | 5 +-- > drivers/hid/intel-ish-hid/ishtp/hbm.c| 4 +- > drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 4 +- > drivers/hid/usbhid/hid-pidff.c | 4 +- > drivers/hid/usbhid/usbkbd.c | 2 +- > drivers/hid/wacom_sys.c | 2 +- > include/linux/intel-ish-client-if.h | 8 +++- > 24 files changed, 90 insertions(+), 81 deletions(-) > > Cc: Alexandre Torgue > Cc: Anssi Hannula > Cc: Benjamin Tissoires > Cc: "Bruno Prémont" > Cc: "Christian König" > Cc: Daniel Drubin > Cc: Dario Pagani > Cc: dri-de...@lists.freedesktop.org > Cc: Henrik Rydberg > Cc: Jiri Kosina > Cc: Jonathan Cameron > Cc: Kai-Heng Feng > Cc: Kim Kuparinen > Cc: "Krzysztof Wilczyński" > Cc: Lee Jones > Cc: linaro-mm-...@lists.linaro.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-...@vger.kernel.org > Cc: linux-in...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-...@vger.kernel.org > Cc: Lopez Casado > Cc: "L. Vinyard, Jr" > Cc: Masaki Ota > Cc: Maxime Coquelin > Cc: message to > Cc: Michael Haboustak > Cc: Rushikesh S Kadam > Cc: Srinivas Pandruvada > Cc: Sumit Semwal > Cc: "Uwe Kleine-König" > Cc: Vojtech Pavlik > Cc: Zhang Lixu
[PATCH v2] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume
From: Ye Xiang
The individual sensor drivers implemented in the ISH firmware needs
capability to take special actions when there is a change in the system
standby state. The ISH core firmware passes this notification to
individual sensor drivers in response to the OS request via connected
standby bit in the SYSTEM_STATE_STATUS command.
This change sets CONNECTED_STANDBY_STATE_BIT bit to 1 during suspend
callback and clears during resume callback.
Signed-off-by: Ye Xiang
[srinivas.pandruv...@linux.intel.com: changelog rewrite]
Acked-by: Srinivas Pandruvada
---
v2:
changed changelog to be more clear
Changed the name in the signed-off to match "From"
drivers/hid/intel-ish-hid/ishtp/hbm.c | 6 +++---
drivers/hid/intel-ish-hid/ishtp/hbm.h | 1 +
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/hbm.c
b/drivers/hid/intel-ish-hid/ishtp/hbm.c
index 30a91d068306..dbfae60f2621 100644
--- a/drivers/hid/intel-ish-hid/ishtp/hbm.c
+++ b/drivers/hid/intel-ish-hid/ishtp/hbm.c
@@ -914,7 +914,7 @@ static inline void fix_cl_hdr(struct ishtp_msg_hdr *hdr,
size_t length,
/*** Suspend and resume notification ***/
static uint32_t current_state;
-static uint32_t supported_states = 0 | SUSPEND_STATE_BIT;
+static uint32_t supported_states = SUSPEND_STATE_BIT |
CONNECTED_STANDBY_STATE_BIT;
/**
* ishtp_send_suspend() - Send suspend message to FW
@@ -933,7 +933,7 @@ void ishtp_send_suspend(struct ishtp_device *dev)
memset(_status_msg, 0, len);
state_status_msg.hdr.cmd = SYSTEM_STATE_STATUS;
state_status_msg.supported_states = supported_states;
- current_state |= SUSPEND_STATE_BIT;
+ current_state |= (SUSPEND_STATE_BIT | CONNECTED_STANDBY_STATE_BIT);
dev->print_log(dev, "%s() sends SUSPEND notification\n", __func__);
state_status_msg.states_status = current_state;
@@ -959,7 +959,7 @@ void ishtp_send_resume(struct ishtp_device *dev)
memset(_status_msg, 0, len);
state_status_msg.hdr.cmd = SYSTEM_STATE_STATUS;
state_status_msg.supported_states = supported_states;
- current_state &= ~SUSPEND_STATE_BIT;
+ current_state &= ~(CONNECTED_STANDBY_STATE_BIT | SUSPEND_STATE_BIT);
dev->print_log(dev, "%s() sends RESUME notification\n", __func__);
state_status_msg.states_status = current_state;
diff --git a/drivers/hid/intel-ish-hid/ishtp/hbm.h
b/drivers/hid/intel-ish-hid/ishtp/hbm.h
index 7c445b203f2a..08f3f3ceb18c 100644
--- a/drivers/hid/intel-ish-hid/ishtp/hbm.h
+++ b/drivers/hid/intel-ish-hid/ishtp/hbm.h
@@ -235,6 +235,7 @@ struct dma_xfer_hbm {
#define SYSTEM_STATE_QUERY_SUBSCRIBERS 0x3
#define SYSTEM_STATE_STATE_CHANGE_REQ 0x4
/*indicates suspend and resume states*/
+#define CONNECTED_STANDBY_STATE_BIT(1<<0)
#define SUSPEND_STATE_BIT (1<<1)
struct ish_system_states_header {
--
2.25.4
Re: [PATCH] tools/power/x86/intel-speed-select: drop __DATE__ and __TIME__ macros
On Fri, 2021-03-12 at 10:57 -0300, Antonio Terceiro wrote:
> These macros introduce nondeterminism in builds, and break
> reproducible
> builds.
Thanks. I will include this change in my pull request.
-Srinivas
>
> Signed-off-by: Antonio Terceiro
> Cc: Srinivas Pandruvada
> ---
> tools/power/x86/intel-speed-select/isst-config.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/tools/power/x86/intel-speed-select/isst-config.c
> b/tools/power/x86/intel-speed-select/isst-config.c
> index 582feb88eca3..c50c754a656e 100644
> --- a/tools/power/x86/intel-speed-select/isst-config.c
> +++ b/tools/power/x86/intel-speed-select/isst-config.c
> @@ -2791,7 +2791,6 @@ static void usage(void)
> static void print_version(void)
> {
> fprintf(outf, "Version %s\n", version_str);
> - fprintf(outf, "Build date %s time %s\n", __DATE__, __TIME__);
> exit(0);
> }
>
Re: [PATCH V2 1/25] x86/cpufeatures: Enumerate Intel Hybrid Technology feature bit
On Wed, 2021-03-10 at 14:25 -0800, Ricardo Neri wrote: > On Wed, Mar 10, 2021 at 09:01:47PM +0100, Borislav Petkov wrote: > > On Wed, Mar 10, 2021 at 11:46:44AM -0800, Ricardo Neri wrote: > > > But this series provides the use case, right? Kan's patches > > > handle PMU counters > > > that may differ cross types of CPUs. In patch 2, > > > get_hybrid_params() > > > needs to check first if X86_FEATURE_HYBRID_CPU is enabled before > > > querying the hybrid parameters. Otherwise, we would need to rely > > > on the > > > maximum level of CPUID, which may not be reliable. > > > > On Wed, Mar 10, 2021 at 11:33:54AM -0800, Srinivas Pandruvada > > wrote: > > > We are working on changes to P-State driver for hybrid CPUs using > > > this > > > define. They are still work in progress. > > > But this patch can be submitted later with our set of changes. > > > > Answering to both with a single mail: > > > > I don't have a problem with X86_FEATURE_HYBRID_CPU - I simply don't > > want > > to show "hybrid_cpu" in /proc/cpuinfo unless there's a valid use > > case > > for userspace to know that it is running on a hybrid CPU. > > Ah, I get your point now. You would like to see > > #define X86_FEATURE_HYBRID_CPU (18*32+15) /* "" This > part has CPUs of more than one type */ > > Right? Now your first comment makes sense. > > Srinivas, Kan, I don't think we need to expose "hybrid_cpu" in > /proc/cpuinfo, do we? I don't see any need. Thanks, Srinivas > > Thanks and BR, > Ricardo
Re: [PATCH V2 1/25] x86/cpufeatures: Enumerate Intel Hybrid Technology feature bit
On Wed, 2021-03-10 at 17:53 +0100, Borislav Petkov wrote: > On Wed, Mar 10, 2021 at 08:37:37AM -0800, kan.li...@linux.intel.com > wrote: > > From: Ricardo Neri > > > > Add feature enumeration to identify a processor with Intel Hybrid > > Technology: one in which CPUs of more than one type are the same > > package. > > On a hybrid processor, all CPUs support the same homogeneous (i.e., > > symmetric) instruction set. All CPUs enumerate the same features in > > CPUID. > > Thus, software (user space and kernel) can run and migrate to any > > CPU in > > the system as well as utilize any of the enumerated features > > without any > > change or special provisions. The main difference among CPUs in a > > hybrid > > processor are power and performance properties. > > > > Cc: Andi Kleen > > Cc: Kan Liang > > Cc: "Peter Zijlstra (Intel)" > > Cc: "Rafael J. Wysocki" > > Cc: "Ravi V. Shankar" > > Cc: Srinivas Pandruvada > > Cc: linux-kernel@vger.kernel.org > > Reviewed-by: Len Brown > > Reviewed-by: Tony Luck > > Signed-off-by: Ricardo Neri > > --- > > Changes since v1 (as part of patchset for perf change for > > Alderlake) > > * None > > > > Changes since v1 (in a separate posting): > > * Reworded commit message to clearly state what is Intel Hybrid > >Technology. Stress that all CPUs can run the same instruction > >set and support the same features. > > --- > > arch/x86/include/asm/cpufeatures.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/x86/include/asm/cpufeatures.h > > b/arch/x86/include/asm/cpufeatures.h > > index cc96e26d69f7..e7cfc9eedf8d 100644 > > --- a/arch/x86/include/asm/cpufeatures.h > > +++ b/arch/x86/include/asm/cpufeatures.h > > @@ -374,6 +374,7 @@ > > #define X86_FEATURE_MD_CLEAR (18*32+10) /* VERW > > clears CPU buffers */ > > #define X86_FEATURE_TSX_FORCE_ABORT(18*32+13) /* "" > > TSX_FORCE_ABORT */ > > #define X86_FEATURE_SERIALIZE (18*32+14) /* SERIALIZE > > instruction */ > > +#define X86_FEATURE_HYBRID_CPU (18*32+15) /* This part > > has CPUs of more than one type */ > > /* "" This > ... > > unless you have a valid use case for "hybrid_cpu" being present > there. We are working on changes to P-State driver for hybrid CPUs using this define. They are still work in progress. But this patch can be submitted later with our set of changes. Thanks, Srinivas > > Thx. >
Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume
On Tue, 2021-03-09 at 11:47 +0800, Ye, Xiang wrote: > Hi Srinivas, Jiri > > On Mon, Mar 08, 2021 at 08:00:41AM -0800, Srinivas Pandruvada wrote: > > On Mon, 2021-03-08 at 11:26 +0100, Jiri Kosina wrote: > > > On Wed, 3 Mar 2021, Ye Xiang wrote: > > > > > > > ISH firmware uses connected standby state bit > > > > (CONNECTED_STANDBY_STATE_BIT bit 1) > > > > to notify current power state to sensors instead of suspend > > > > state > > > > bit (bit 0). > > > > So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT > > > > to > > > > firmware > > > > to be compatible with the previous version. > > > > > > Could you please make the changelog more verbose -- namely what > > > user-visible issue this is fixing? > > Xiang, > > > > I think this change is for related to Elkhart Lake for support of > > connected standby (keep listening for sensor events during Linux > > suspend for some sensors). In this way some sensor can wake up the > > system. > This change is for all ISH platform. Currently, ISH firmware use > both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to identify > system state. It is related to system wake up by ISH and it enable > each > sensor in ISH to be notified the current system state, when system > state > change. What will sensors do with this additional information? I think the individual sensors in ISH can decide whether to power OFF or ON based on this information to save power during system suspend to idle. Thanks, Srinivas > > Thanks > Ye Xiang > > > > > > > > > > > >
Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition
On Mon, 2021-03-08 at 17:16 +0100, Jiri Kosina wrote: > On Mon, 8 Mar 2021, Srinivas Pandruvada wrote: > > > > > A remove callback is only ever called for a bound device. So > > > > there > > > > is no > > > > need to check for device or driver being NULL. > > > > > > Srinivas, any objections to this patchset? The cleanups look good > > > to > > > me. > > Sorry, I missed this series. > > No objection for taking these patches. > > Thanks. Applied with your Acked-by: > If you disagree with that interpretation of your statement above, > please > holler :) I agree. For record: Acked-by: Srinivas Pandruvada Thanks, Srinivas > > Thanks, >
Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition
On Mon, 2021-03-08 at 11:07 +0100, Jiri Kosina wrote: > On Sat, 6 Feb 2021, Uwe Kleine-König wrote: > > > A remove callback is only ever called for a bound device. So there > > is no > > need to check for device or driver being NULL. > > Srinivas, any objections to this patchset? The cleanups look good to > me. Sorry, I missed this series. No objection for taking these patches. Thanks, Srinivas > Thanks, >
Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume
On Mon, 2021-03-08 at 11:26 +0100, Jiri Kosina wrote: > On Wed, 3 Mar 2021, Ye Xiang wrote: > > > ISH firmware uses connected standby state bit > > (CONNECTED_STANDBY_STATE_BIT bit 1) > > to notify current power state to sensors instead of suspend state > > bit (bit 0). > > So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to > > firmware > > to be compatible with the previous version. > > Could you please make the changelog more verbose -- namely what > user-visible issue this is fixing? Xiang, I think this change is for related to Elkhart Lake for support of connected standby (keep listening for sensor events during Linux suspend for some sensors). In this way some sensor can wake up the system. Thanks, Srinivas > > Thanks. >
Re: [PATCH 2/5] iio: hid-sensor-als: Support change sensitivity in illuminance too.
On Sun, 2021-02-28 at 14:45 +, Jonathan Cameron wrote: > On Sat, 27 Feb 2021 17:26:40 -0800 > Ronald Tschalär wrote: > > > Recent MacBook Pro's specify the usage of the change sensitivity > > field > > as illuminance (with a change sensitivity modifier) rather than as > > light. > > > > Signed-off-by: Ronald Tschalär > This looks fine to me though it the hid sensors spec never fails to > surprise > me in the different slight variants of the same thing that come up. > > Illuminance is at least fairly well defined, but who knows what for > the DATA_LIGHT > version takes? The current implementations are deploying using "HID_USAGE_SENSOR_LIGHT_ILLUM" usage id 0xD1 for input. So this is natural to use the same usage id for sensitivity. So patch looks good to me. But most implementation choose to use DATA_LIGHT for the sensitivity. probably referring to change in quantity of light without referring to area. There are no obvious units specified for DATA_LIGHT in the spec. Thanks, Srinivas > > Anyhow, lets give time for Srinivas to sanity check this as he's much > more familiar > with that spec than I am. > > Jonathan > > > --- > > drivers/iio/light/hid-sensor-als.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/iio/light/hid-sensor-als.c > > b/drivers/iio/light/hid-sensor-als.c > > index a21c827e4953d..849ee37dcd866 100644 > > --- a/drivers/iio/light/hid-sensor-als.c > > +++ b/drivers/iio/light/hid-sensor-als.c > > @@ -252,6 +252,14 @@ static int als_parse_report(struct > > platform_device *pdev, > > HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_AB > > S | > > HID_USAGE_SENSOR_DATA_LIGHT, > > >common_attributes.sensitivity); > > + > > + if (st->common_attributes.sensitivity.index < 0) > > + sensor_hub_input_get_attribute_info(hsdev, > > + HID_FEATURE_REPORT, usage_id, > > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSIT > > IVITY_ABS | > > + HID_USAGE_SENSOR_LIGHT_ILLUM, > > + >common_attributes.sensitivity); > > + > > dev_dbg(>dev, "Sensitivity index:report %d:%d\n", > > st->common_attributes.sensitivity.index, > > st->common_attributes.sensitivity.report_id);
Re: [RFC PATCH] powercap: Add Hygon Fam18h RAPL support
On Thu, 2021-02-25 at 21:01 +0800, Pu Wen wrote:
> Enable Hygon Fam18h RAPL support for the power capping framework.
>
If this patch is tested and works on this processor, not sure why this
is RFC?
Thanks,
Srinivas
> Signed-off-by: Pu Wen
> ---
> drivers/powercap/intel_rapl_common.c | 1 +
> drivers/powercap/intel_rapl_msr.c| 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/powercap/intel_rapl_common.c
> b/drivers/powercap/intel_rapl_common.c
> index fdda2a737186..73cf68af9770 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -1069,6 +1069,7 @@ static const struct x86_cpu_id rapl_ids[]
> __initconst = {
>
> X86_MATCH_VENDOR_FAM(AMD, 0x17, _defaults_amd),
> X86_MATCH_VENDOR_FAM(AMD, 0x19, _defaults_amd),
> + X86_MATCH_VENDOR_FAM(HYGON, 0x18, _defaults_amd),
> {}
> };
> MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
> diff --git a/drivers/powercap/intel_rapl_msr.c
> b/drivers/powercap/intel_rapl_msr.c
> index 78213d4b5b16..cc3b22881bfe 100644
> --- a/drivers/powercap/intel_rapl_msr.c
> +++ b/drivers/powercap/intel_rapl_msr.c
> @@ -150,6 +150,7 @@ static int rapl_msr_probe(struct platform_device
> *pdev)
> case X86_VENDOR_INTEL:
> rapl_msr_priv = _msr_priv_intel;
> break;
> + case X86_VENDOR_HYGON:
> case X86_VENDOR_AMD:
> rapl_msr_priv = _msr_priv_amd;
> break;
Re: [PATCH] HID: intel-ish-hid: ipc: Add Tiger Lake H PCI device ID
On Thu, 2021-02-04 at 16:33 +0800, You-Sheng Yang wrote:
> Added Tiger Lake H PCI device ID to the supported device list.
>
> Signed-off-by: You-Sheng Yang
Acked-by: Srinivas Pandruvada
> ---
> drivers/hid/intel-ish-hid/ipc/hw-ish.h | 1 +
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> index 1fb294ca463e..21b0e6123754 100644
> --- a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> +++ b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> @@ -27,6 +27,7 @@
> #define CMP_H_DEVICE_ID 0x06FC
> #define EHL_Ax_DEVICE_ID 0x4BB3
> #define TGL_LP_DEVICE_ID 0xA0FC
> +#define TGL_H_DEVICE_ID 0x43FC
>
> #define REVISION_ID_CHT_A0 0x6
> #define REVISION_ID_CHT_Ax_SI 0x0
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index c6d48a8648b7..6dea657b7b15 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -37,6 +37,7 @@ static const struct pci_device_id ish_pci_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, CMP_H_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, EHL_Ax_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, TGL_LP_DEVICE_ID)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, TGL_H_DEVICE_ID)},
> {0, }
> };
> MODULE_DEVICE_TABLE(pci, ish_pci_tbl);
Re: [PATCH 1/2] powercap/intel_rapl: Use topology interface in rapl_add_package()
On Fri, 2021-02-05 at 13:45 +0100, Rafael J. Wysocki wrote:
> On Sat, Jan 23, 2021 at 11:07 AM Yunfeng Ye
> wrote:
> > It's not a good way to access phys_proc_id and cpu_die_id directly.
> > So using topology_physical_package_id(cpu) and topology_die_id(cpu)
> > instead.
> >
> > Signed-off-by: Yunfeng Ye
>
> Srinivas, Rui, any concerns?
Looks good.
Thanks,
Srinivas
>
> > ---
> > drivers/powercap/intel_rapl_common.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index c9e57237d778..5f3d39b8212a 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -1309,7 +1309,6 @@ struct rapl_package *rapl_add_package(int
> > cpu, struct rapl_if_priv *priv)
> > {
> > int id = topology_logical_die_id(cpu);
> > struct rapl_package *rp;
> > - struct cpuinfo_x86 *c = _data(cpu);
> > int ret;
> >
> > if (!rapl_defaults)
> > @@ -1326,10 +1325,11 @@ struct rapl_package *rapl_add_package(int
> > cpu, struct rapl_if_priv *priv)
> >
> > if (topology_max_die_per_package() > 1)
> > snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH,
> > -"package-%d-die-%d", c->phys_proc_id, c-
> > >cpu_die_id);
> > +"package-%d-die-%d",
> > +topology_physical_package_id(cpu),
> > topology_die_id(cpu));
> > else
> > snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH,
> > "package-%d",
> > -c->phys_proc_id);
> > +topology_physical_package_id(cpu));
> >
> > /* check if the package contains valid domains */
> > if (rapl_detect_domains(rp, cpu) || rapl_defaults-
> > >check_unit(rp, cpu)) {
> > --
> > 2.27.0
> >
Re: [PATCH 2/2] powercap/intel_rapl: Use topology interface in rapl_init_domains()
On Fri, 2021-02-05 at 13:46 +0100, Rafael J. Wysocki wrote:
> On Sat, Jan 23, 2021 at 11:07 AM Yunfeng Ye
> wrote:
> > It's not a good way to access the phys_proc_id of cpuinfo directly.
> > So using topology_physical_package_id(cpu) instead.
> >
> > Signed-off-by: Yunfeng Ye
>
> Srinivas, Rui, any concerns?
Looks good.
Thanks,
Srinivas
>
> > ---
> > drivers/powercap/intel_rapl_common.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index 5f3d39b8212a..adcb3927 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -547,7 +547,7 @@ static void rapl_init_domains(struct
> > rapl_package *rp)
> >
> > if (i == RAPL_DOMAIN_PLATFORM && rp->id > 0) {
> > snprintf(rd->name, RAPL_DOMAIN_NAME_LENGTH,
> > "psys-%d",
> > - cpu_data(rp-
> > >lead_cpu).phys_proc_id);
> > + topology_physical_package_id(rp-
> > >lead_cpu));
> > } else
> > snprintf(rd->name, RAPL_DOMAIN_NAME_LENGTH,
> > "%s",
> > rapl_domain_names[i]);
> > --
> > 2.27.0
> >
Re: [PATCH] HID: intel-ish-hid: ipc: Add Tiger Lake H PCI device ID
On Thu, 2021-02-04 at 16:33 +0800, You-Sheng Yang wrote:
> Added Tiger Lake H PCI device ID to the supported device list.
>
> Signed-off-by: You-Sheng Yang
Did you get chance to verify on a platform?
Do you see sensors enumerated in /sys/bus/iio?
Thanks,
Srinivas
> ---
> drivers/hid/intel-ish-hid/ipc/hw-ish.h | 1 +
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> index 1fb294ca463e..21b0e6123754 100644
> --- a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> +++ b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> @@ -27,6 +27,7 @@
> #define CMP_H_DEVICE_ID 0x06FC
> #define EHL_Ax_DEVICE_ID 0x4BB3
> #define TGL_LP_DEVICE_ID 0xA0FC
> +#define TGL_H_DEVICE_ID 0x43FC
>
> #define REVISION_ID_CHT_A0 0x6
> #define REVISION_ID_CHT_Ax_SI 0x0
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index c6d48a8648b7..6dea657b7b15 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -37,6 +37,7 @@ static const struct pci_device_id ish_pci_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, CMP_H_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, EHL_Ax_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, TGL_LP_DEVICE_ID)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, TGL_H_DEVICE_ID)},
> {0, }
> };
> MODULE_DEVICE_TABLE(pci, ish_pci_tbl);
Re: [PATCH] platform/x86/intel-uncore-freq: Add Sapphire Rapids server support
On Wed, 2021-02-03 at 13:43 +0200, Artem Bityutskiy wrote:
> From: Artem Bityutskiy
>
> Sapphire Rapids uncore frequency control is the same as Skylake and
> Ice Lake.
> Add the Sapphire Rapids CPU model number to the match array.
>
> Signed-off-by: Artem Bityutskiy
> Reviewed-by: Tony Luck
Acked-by: Srinivas Pandruvada
> ---
> drivers/platform/x86/intel-uncore-frequency.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/intel-uncore-frequency.c
> b/drivers/platform/x86/intel-uncore-frequency.c
> index 12d5ab7e1f5d..3ee4c5c8a64f 100644
> --- a/drivers/platform/x86/intel-uncore-frequency.c
> +++ b/drivers/platform/x86/intel-uncore-frequency.c
> @@ -377,6 +377,7 @@ static const struct x86_cpu_id
> intel_uncore_cpu_ids[] = {
> X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
> X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, NULL),
> X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D, NULL),
> + X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
> {}
> };
>
Re: [PATCH v3 2/2] thermal: Move therm_throt there from x86/mce
On Tue, 2021-02-02 at 13:10 +0100, Borislav Petkov wrote:
> On Mon, Feb 01, 2021 at 11:10:29AM -0800, Srinivas Pandruvada wrote:
> > Only user of the above interface is in drivers/thermal/intel.
> > So why can't we move these to
> > drivers/thermal/intel/thermal_interrupt.h
> > or similar?
>
> Sure, see below.
>
Thanks.
Reviewed-by: Srinivas Pandruvada
> ---
> From: Borislav Petkov
> Date: Thu, 7 Jan 2021 13:29:05 +0100
> Subject: [PATCH] thermal: Move therm_throt there from x86/mce
>
> This functionality has nothing to do with MCE, move it to the thermal
> framework and untangle it from MCE.
>
> Requested-by: Peter Zijlstra
> Signed-off-by: Borislav Petkov
> ---
> arch/x86/Kconfig | 4 ---
> arch/x86/include/asm/mce.h| 16 --
> arch/x86/include/asm/thermal.h| 13
> arch/x86/kernel/cpu/intel.c | 3 ++
> arch/x86/kernel/cpu/mce/Makefile | 2 --
> arch/x86/kernel/cpu/mce/intel.c | 1 -
> arch/x86/kernel/irq.c | 21
> drivers/thermal/intel/Kconfig | 4 +++
> drivers/thermal/intel/Makefile| 1 +
> .../thermal/intel}/therm_throt.c | 32 ++---
> --
> drivers/thermal/intel/thermal_interrupt.h | 15 +
> drivers/thermal/intel/x86_pkg_temp_thermal.c | 4 ++-
> 12 files changed, 69 insertions(+), 47 deletions(-)
> create mode 100644 arch/x86/include/asm/thermal.h
> rename {arch/x86/kernel/cpu/mce =>
> drivers/thermal/intel}/therm_throt.c (97%)
> create mode 100644 drivers/thermal/intel/thermal_interrupt.h
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 21f851179ff0..9989db3a9bf5 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1158,10 +1158,6 @@ config X86_MCE_INJECT
> If you don't know what a machine check is and you don't do
> kernel
> QA it is safe to say n.
>
> -config X86_THERMAL_VECTOR
> - def_bool y
> - depends on X86_MCE_INTEL
> -
> source "arch/x86/events/Kconfig"
>
> config X86_LEGACY_VM86
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index def9aa5e1fa4..ddfb3cad8dff 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -288,22 +288,6 @@ extern void (*mce_threshold_vector)(void);
> /* Deferred error interrupt handler */
> extern void (*deferred_error_int_vector)(void);
>
> -/*
> - * Thermal handler
> - */
> -
> -void intel_init_thermal(struct cpuinfo_x86 *c);
> -
> -/* Interrupt Handler for core thermal thresholds */
> -extern int (*platform_thermal_notify)(__u64 msr_val);
> -
> -/* Interrupt Handler for package thermal thresholds */
> -extern int (*platform_thermal_package_notify)(__u64 msr_val);
> -
> -/* Callback support of rate control, return true, if
> - * callback has rate control */
> -extern bool (*platform_thermal_package_rate_control)(void);
> -
> /*
> * Used by APEI to report memory error via /dev/mcelog
> */
> diff --git a/arch/x86/include/asm/thermal.h
> b/arch/x86/include/asm/thermal.h
> new file mode 100644
> index ..ddbdefd5b94f
> --- /dev/null
> +++ b/arch/x86/include/asm/thermal.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_THERMAL_H
> +#define _ASM_X86_THERMAL_H
> +
> +#ifdef CONFIG_X86_THERMAL_VECTOR
> +void intel_init_thermal(struct cpuinfo_x86 *c);
> +bool x86_thermal_enabled(void);
> +void intel_thermal_interrupt(void);
> +#else
> +static inline void intel_init_thermal(struct cpuinfo_x86 *c) { }
> +#endif
> +
> +#endif /* _ASM_X86_THERMAL_H */
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index 59a1e3ce3f14..71221af87cb1 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
>
> #ifdef CONFIG_X86_64
> #include
> @@ -719,6 +720,8 @@ static void init_intel(struct cpuinfo_x86 *c)
> tsx_disable();
>
> split_lock_init();
> +
> + intel_init_thermal(c);
> }
>
> #ifdef CONFIG_X86_32
> diff --git a/arch/x86/kernel/cpu/mce/Makefile
> b/arch/x86/kernel/cpu/mce/Makefile
> index 9f020c994154..015856abdbb1 100644
> --- a/arch/x86/kernel/cpu/mce/Makefile
> +++ b/arch/x86/kernel/cpu/mce/Makefile
> @@ -9,8 +9,6 @@ obj-$(CONFIG_X86_MCE_THRESHOLD) += threshold.o
> mce-inject-y := inject.o
> obj-$(CONFIG_X86_MCE_INJECT) += mce-inject.o
>
> -obj-$(CONFIG_X86_THERMAL_VEC
Re: [PATCH v3 0/2] Move ...mce/therm_throt.c to drivers/thermal/
On Mon, 2021-02-01 at 15:27 +0100, Borislav Petkov wrote:
> From: Borislav Petkov
>
> Hi,
>
> I know I already committed those but
>
> https://lkml.kernel.org/r/20210201032427.GB12524@xsang-OptiPlex-9020
>
> made me look at the IRQ handler registration. And it does happen per
> CPU
> which is particularly daft and unneeded. And it used to do that
> before
> that change too, for some unknown reason.
>
> So I decided to not have a handler registration but simply call the
> thermal interrupt handler if all is setup and before that issue the
> message about the unexpected IRQ.
>
> I did test it by sending bogus thermal interrupts before and after
> registration - see hunk below - and it looks good:
>
> [0.136608] 0: Sending thermal IRQ
> [0.136760] CPU0: Unexpected LVT thermal interrupt!
> [0.136917] CPU0: Thermal monitoring enabled (TM1)
> [0.137071] 1: Sending thermal IRQ
> [0.043243] 0: Sending thermal IRQ
> [0.043243] 1: Sending thermal IRQ
> [0.043243] 0: Sending thermal IRQ
> [0.043243] 1: Sending thermal IRQ
> [0.043243] 0: Sending thermal IRQ
> [0.043243] 1: Sending thermal IRQ
> [0.043243] 0: Sending thermal IRQ
> [0.043243] 1: Sending thermal IRQ
> [0.043243] 0: Sending thermal IRQ
> [0.043243] 1: Sending thermal IRQ
> [0.149436] thermal_sys: Registered thermal governor 'fair_share'
> [0.149437] thermal_sys: Registered thermal governor 'bang_bang'
> [0.149595] thermal_sys: Registered thermal governor 'step_wise'
> [0.149753] thermal_sys: Registered thermal governor 'user_space'
> [0.445717] ACPI: \_SB_.PR00: _OSC native thermal LVT Acked
> [0.707539] thermal LNXTHERM:00: registered as thermal_zone0
>
> Logic in patch 1 got a bit simplified too.
>
[...]
>
> Borislav Petkov (2):
> x86/mce: Get rid of mcheck_intel_therm_init()
> thermal: Move therm_throt there from x86/mce
>
I have applied this series and tested. Didn't find any functional
issues. But I have one comment on patch 2/2.
Tested-by: Srinivas Pandruvada
Thanks,
Srinivas
> arch/x86/Kconfig | 4 --
> arch/x86/include/asm/mce.h| 22 --
> arch/x86/include/asm/thermal.h| 24 +++
> arch/x86/kernel/cpu/intel.c | 3 ++
> arch/x86/kernel/cpu/mce/Makefile | 2 -
> arch/x86/kernel/cpu/mce/core.c| 1 -
> arch/x86/kernel/cpu/mce/intel.c | 1 -
> arch/x86/kernel/irq.c | 21 ++
> drivers/thermal/intel/Kconfig | 4 ++
> drivers/thermal/intel/Makefile| 1 +
> .../thermal/intel}/therm_throt.c | 41 -
> --
> drivers/thermal/intel/x86_pkg_temp_thermal.c | 3 +-
> 12 files changed, 64 insertions(+), 63 deletions(-)
> create mode 100644 arch/x86/include/asm/thermal.h
> rename {arch/x86/kernel/cpu/mce =>
> drivers/thermal/intel}/therm_throt.c (96%)
>
> 2nd:rc6-therm_throt
>
Re: [PATCH v3 2/2] thermal: Move therm_throt there from x86/mce
On Mon, 2021-02-01 at 15:27 +0100, Borislav Petkov wrote:
> From: Borislav Petkov
>
> This functionality has nothing to do with MCE, move it to the thermal
> framework and untangle it from MCE.
>
>
[...]
> /*
> * Used by APEI to report memory error via /dev/mcelog
> */
> diff --git a/arch/x86/include/asm/thermal.h
> b/arch/x86/include/asm/thermal.h
> new file mode 100644
> index ..36f97bff62e1
> --- /dev/null
> +++ b/arch/x86/include/asm/thermal.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_THERMAL_H
> +#define _ASM_X86_THERMAL_H
> +
> +/* Interrupt Handler for package thermal thresholds */
> +extern int (*platform_thermal_package_notify)(__u64 msr_val);
> +
> +/* Interrupt Handler for core thermal thresholds */
> +extern int (*platform_thermal_notify)(__u64 msr_val);
> +
> +/* Callback support of rate control, return true, if
> + * callback has rate control */
> +extern bool (*platform_thermal_package_rate_control)(void);
> +
Only user of the above interface is in drivers/thermal/intel.
So why can't we move these to drivers/thermal/intel/thermal_interrupt.h
or similar?
Thanks,
Srinivas
> +#ifdef CONFIG_X86_THERMAL_VECTOR
> +void intel_init_thermal(struct cpuinfo_x86 *c);
> +bool x86_thermal_enabled(void);
> +void intel_thermal_interrupt(void);
> +#else
> +static inline void intel_init_thermal(struct cpuinfo_x86 *c) { }
> +#endif
> +
> +
> +#endif /* _ASM_X86_THERMAL_H */
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index 59a1e3ce3f14..71221af87cb1 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
>
> #ifdef CONFIG_X86_64
> #include
> @@ -719,6 +720,8 @@ static void init_intel(struct cpuinfo_x86 *c)
> tsx_disable();
>
> split_lock_init();
> +
> + intel_init_thermal(c);
> }
>
> #ifdef CONFIG_X86_32
> diff --git a/arch/x86/kernel/cpu/mce/Makefile
> b/arch/x86/kernel/cpu/mce/Makefile
> index 9f020c994154..015856abdbb1 100644
> --- a/arch/x86/kernel/cpu/mce/Makefile
> +++ b/arch/x86/kernel/cpu/mce/Makefile
> @@ -9,8 +9,6 @@ obj-$(CONFIG_X86_MCE_THRESHOLD) += threshold.o
> mce-inject-y := inject.o
> obj-$(CONFIG_X86_MCE_INJECT) += mce-inject.o
>
> -obj-$(CONFIG_X86_THERMAL_VECTOR) += therm_throt.o
> -
> obj-$(CONFIG_ACPI_APEI) += apei.o
>
> obj-$(CONFIG_X86_MCELOG_LEGACY) += dev-mcelog.o
> diff --git a/arch/x86/kernel/cpu/mce/intel.c
> b/arch/x86/kernel/cpu/mce/intel.c
> index c2476fe0682e..e309476743b7 100644
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
> @@ -531,7 +531,6 @@ static void intel_imc_init(struct cpuinfo_x86 *c)
>
> void mce_intel_feature_init(struct cpuinfo_x86 *c)
> {
> - intel_init_thermal(c);
> intel_init_cmci();
> intel_init_lmce();
> intel_ppin_init(c);
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index c5dd50369e2f..d4ad344e80bf 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
>
> #define CREATE_TRACE_POINTS
> #include
> @@ -374,3 +375,23 @@ void fixup_irqs(void)
> }
> }
> #endif
> +
> +#ifdef CONFIG_X86_THERMAL_VECTOR
> +static void smp_thermal_vector(void)
> +{
> + if (x86_thermal_enabled())
> + intel_thermal_interrupt();
> + else
> + pr_err("CPU%d: Unexpected LVT thermal interrupt!\n",
> +smp_processor_id());
> +}
> +
> +DEFINE_IDTENTRY_SYSVEC(sysvec_thermal)
> +{
> + trace_thermal_apic_entry(THERMAL_APIC_VECTOR);
> + inc_irq_stat(irq_thermal_count);
> + smp_thermal_vector();
> + trace_thermal_apic_exit(THERMAL_APIC_VECTOR);
> + ack_APIC_irq();
> +}
> +#endif
> diff --git a/drivers/thermal/intel/Kconfig
> b/drivers/thermal/intel/Kconfig
> index 8025b21f43fa..ce4f59213c7a 100644
> --- a/drivers/thermal/intel/Kconfig
> +++ b/drivers/thermal/intel/Kconfig
> @@ -8,6 +8,10 @@ config INTEL_POWERCLAMP
> enforce idle time which results in more package C-state
> residency. The
> user interface is exposed via generic thermal framework.
>
> +config X86_THERMAL_VECTOR
> + def_bool y
> + depends on X86 && CPU_SUP_INTEL && X86_LOCAL_APIC
> +
> config X86_PKG_TEMP_THERMAL
> tristate "X86 package temperature thermal driver"
> depends on X86_THERMAL_VECTOR
> diff --git a/drivers/thermal/intel/Makefile
> b/drivers/thermal/intel/Makefile
> index 0d9736ced5d4..ff2ad30ef397 100644
> --- a/drivers/thermal/intel/Makefile
> +++ b/drivers/thermal/intel/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_INTEL_QUARK_DTS_THERMAL) +=
> intel_quark_dts_thermal.o
> obj-$(CONFIG_INT340X_THERMAL) += int340x_thermal/
> obj-$(CONFIG_INTEL_BXT_PMIC_THERMAL) += intel_bxt_pmic_thermal.o
> obj-$(CONFIG_INTEL_PCH_THERMAL) += intel_pch_thermal.o
>
Re: [PATCH 2/3] hid-sensor-common: Add relative sensitivity check
On Sun, 2021-01-31 at 11:26 +, Jonathan Cameron wrote:
> On Fri, 29 Jan 2021 00:35:49 +0800
> "Ye, Xiang" wrote:
>
> > Hi Srinivas andd Jonathan
> >
> > Thanks for the review.
> >
> > On Sun, Jan 24, 2021 at 08:20:12AM -0800, Srinivas Pandruvada
> > wrote:
> > > On Sun, 2021-01-24 at 13:14 +, Jonathan Cameron wrote:
> > > > On Wed, 20 Jan 2021 15:47:05 +0800
> > > > Ye Xiang wrote:
> > > >
> > > > > Some hid sensors may use relative sensitivity such as als
> > > > > sensor.
> > > > > This patch add relative sensitivity check for all hid-
> > > > > sensors.
> > > > >
> > > > > Signed-off-by: Ye Xiang
> > > > > ---
> > > > > .../iio/common/hid-sensors/hid-sensor-attributes.c| 11
> > > > > ++-
> > > > > include/linux/hid-sensor-ids.h| 1 +
> > > > > 2 files changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-
> > > > > attributes.c
> > > > > b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > > > > index d349ace2e33f..b685c292a179 100644
> > > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > > > > @@ -480,7 +480,7 @@ int
> > > > > hid_sensor_parse_common_attributes(struct
> > > > > hid_sensor_hub_device *hsdev,
> > > > >
> > > > > /*
> > > > >* Set Sensitivity field ids, when there is no
> > > > > individualsha512sum
> > > > > modifier, will
> > > > > - * check absolute sensitivity of data field
> > > > > + * check absolute sensitivity and relative sensitivity
> > > > > of data
> > > > > field
> > > > >*/
> > > > > for (i = 0; i < sensitivity_addresses_len && st-
> > > > > > sensitivity.index < 0; i++) {
> > > > > sensor_hub_input_get_attribute_info(hsdev,
> > > > > @@ -488,6 +488,15 @@ int
> > > > > hid_sensor_parse_common_attributes(struct
> > > > > hid_sensor_hub_device *hsdev,
> > > > > HID_USAGE_SENSOR_DATA_MOD_CHANG
> > > > > E_SENSIT
> > > > > IVITY_ABS |
> > > > > sensitivity_addresses[i
> > > > > ],
> > > > > >sensitivity);
> > > > > +
> > > > > + if (st->sensitivity.index >= 0)
> > > > > + break;
> > > > > +
> > > > > + sensor_hub_input_get_attribute_info(hsdev,
> > > > > + HID_FEATURE_REPORT, usage_id,
> > > > > + HID_USAGE_SENSOR_DATA_MOD_CHANG
> > > > > E_SENSIT
> > > > > IVITY_REL_PCT |
> > > > > + sensitivity_addresses[i
> > > > > ],
> > > > > + >sensitivity);
> > > >
> > > > We can't provide the value to userspace without reflecting the
> > > > difference between
> > > > the two ways of expressing it.
> > > >
> > > > It seems there are 3 ways sensitivity is expressed.
> > > > 1. Raw value in same units as the measurement (easy one and
> > > > what is
> > > > currently reported)
> > > > 2. Percentage of range - also relatively easy to transform into
> > > > the
> > > > same as 1.
> > > > 3. Percentage of prior reading.. This one doesn't fit in any
> > > > existing ABI, so
> > > >unfortunately we'll have to invent something new along the
> > > > lines
> > > > of
> > > >*_hysteresis_relative
> >
> > yes, the 3th version sensitivity (Percentage of prior reading) is
> > what we
> > are using for als sensor now. the 1th version sensitivity is common
> > used
> > by other hid sensors. Do you have suggestion or reference about
> > how to add *_hysteresis_relative field to iio model?
>
> Follow through how elements of iio_chan_info_enum in
> incl
Re: [PATCH v2 2/2] thermal: Move therm_throt there from x86/mce
On Mon, 2021-01-25 at 18:18 +0100, Borislav Petkov wrote: > On Mon, Jan 25, 2021 at 09:14:35AM -0800, Srinivas Pandruvada wrote: > > Can the handler, processing architectural features via thermal > > interrupt, reside in arch/x86 folder or need to be > > drivers/thermal/intel? > > Look at... > > > > > @@ -718,7 +699,7 @@ void intel_init_thermal(struct cpuinfo_x86 > > > > *c) > > > > | > > > > PACKAGE_THERM_INT_HIGH_ENABLE), h); > > > > } > > > > > > > > - smp_thermal_vector = intel_thermal_interrupt; > > > > + thermal_set_handler(intel_thermal_interrupt); > > ... here. ^ > > This should answer your question. Thanks for the answer. -Srinivas
Re: [PATCH v2 2/2] thermal: Move therm_throt there from x86/mce
Hi All,
On Mon, 2021-01-25 at 23:42 +0800, Zhang Rui wrote:
> Hi, Borislav,
>
> Thanks for the patch. CC Srinivas.
>
Thanks for adding me.
> On Mon, 2021-01-25 at 14:05 +0100, Borislav Petkov wrote:
> > From: Borislav Petkov
> >
> > This functionality has nothing to do with MCE, move it to the
> > thermal(unfortunately)
> > framework and untangle it from MCE.
> >
Agreed.
But I have a question on how we should handle some new features.
Although interrupt handled in this code is called "Thermal interrupt",
this interrupt is also used for notification for some other power
/energy efficiency optimization events via IA32_THERM_STATUS and
IA32_PACKAGE_THERM_STATUS.
For example (From SDM):
IA32_PACKAGE_THERM_STATUS
Bit 26: Hardware Feedback Interface Structure Change Status
Can the handler, processing architectural features via thermal
interrupt, reside in arch/x86 folder or need to be
drivers/thermal/intel?
> Agreed.
>
> just one question,
> there are many overlaps between this kernel thermal throttling code
> and
> the x86_pkg_temp_thermal driver, is it possible to combine these two
> pieces of code altogether?
May not be a good idea as the events are not just for temperature
notifications.
Thanks,
Srinivas
>
> thanks,
> rui
>
>
> > Have thermal_set_handler() check the build-time assigned default
> > handler
> > stub was the one used before therm_throt assigns a new one.
> >
> > Requested-by: Peter Zijlstra
> > Signed-off-by: Borislav Petkov
> > ---
> > arch/x86/Kconfig | 4 ---
> > arch/x86/include/asm/irq.h| 4 +++
> > arch/x86/include/asm/mce.h| 16 --
> > arch/x86/include/asm/thermal.h| 21 ++
> > arch/x86/kernel/cpu/intel.c | 3 ++
> > arch/x86/kernel/cpu/mce/Makefile | 2 --
> > arch/x86/kernel/cpu/mce/intel.c | 1 -
> > arch/x86/kernel/irq.c | 29
> > +++
> > drivers/thermal/intel/Kconfig | 4 +++
> > drivers/thermal/intel/Makefile| 1 +
> > .../thermal/intel}/therm_throt.c | 25 ++-
> > -
> > drivers/thermal/intel/x86_pkg_temp_thermal.c | 3 +-
> > 12 files changed, 67 insertions(+), 46 deletions(-)
> > create mode 100644 arch/x86/include/asm/thermal.h
> > rename {arch/x86/kernel/cpu/mce =>
> > drivers/thermal/intel}/therm_throt.c (97%)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 21f851179ff0..9989db3a9bf5 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1158,10 +1158,6 @@ config X86_MCE_INJECT
> > If you don't know what a machine check is and you don't do
> > kernel
> > QA it is safe to say n.
> >
> > -config X86_THERMAL_VECTOR
> > - def_bool y
> > - depends on X86_MCE_INTEL
> > -
> > source "arch/x86/events/Kconfig"
> >
> > config X86_LEGACY_VM86
> > diff --git a/arch/x86/include/asm/irq.h
> > b/arch/x86/include/asm/irq.h
> > index 528c8a71fe7f..ad65fe7dceb1 100644
> > --- a/arch/x86/include/asm/irq.h
> > +++ b/arch/x86/include/asm/irq.h
> > @@ -53,4 +53,8 @@ void arch_trigger_cpumask_backtrace(const struct
> > cpumask *mask,
> > #define arch_trigger_cpumask_backtrace
> > arch_trigger_cpumask_backtrace
> > #endif
> >
> > +#ifdef CONFIG_X86_THERMAL_VECTOR
> > +void thermal_set_handler(void (*handler)(void));
> > +#endif
> > +
> > #endif /* _ASM_X86_IRQ_H */
> > diff --git a/arch/x86/include/asm/mce.h
> > b/arch/x86/include/asm/mce.h
> > index def9aa5e1fa4..ddfb3cad8dff 100644
> > --- a/arch/x86/include/asm/mce.h
> > +++ b/arch/x86/include/asm/mce.h
> > @@ -288,22 +288,6 @@ extern void (*mce_threshold_vector)(void);
> > /* Deferred error interrupt handler */
> > extern void (*deferred_error_int_vector)(void);
> >
> > -/*
> > - * Thermal handler
> > - */
> > -
> > -void intel_init_thermal(struct cpuinfo_x86 *c);
> > -
> > -/* Interrupt Handler for core thermal thresholds */
> > -extern int (*platform_thermal_notify)(__u64 msr_val);
> > -
> > -/* Interrupt Handler for package thermal thresholds */
> > -extern int (*platform_thermal_package_notify)(__u64 msr_val);
> > -
> > -/* Callback support of rate control, return true, if
> > - * callback has rate control */
> > -extern bool (*platform_thermal_package_rate_control)(void);
> > -
> > /*
> > * Used by APEI to report memory error via /dev/mcelog
> > */
> > diff --git a/arch/x86/include/asm/thermal.h
> > b/arch/x86/include/asm/thermal.h
> > new file mode 100644
> > index ..58b0e0a4af6e
> > --- /dev/null
> > +++ b/arch/x86/include/asm/thermal.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_X86_THERMAL_H
> > +#define _ASM_X86_THERMAL_H
> > +
> > +/* Interrupt Handler for package thermal thresholds */
> > +extern int (*platform_thermal_package_notify)(__u64 msr_val);
> > +
> > +/* Interrupt Handler for core thermal thresholds */
> > +extern int
Re: [PATCH 2/3] hid-sensor-common: Add relative sensitivity check
On Sun, 2021-01-24 at 13:14 +, Jonathan Cameron wrote:
> On Wed, 20 Jan 2021 15:47:05 +0800
> Ye Xiang wrote:
>
> > Some hid sensors may use relative sensitivity such as als sensor.
> > This patch add relative sensitivity check for all hid-sensors.
> >
> > Signed-off-by: Ye Xiang
> > ---
> > .../iio/common/hid-sensors/hid-sensor-attributes.c| 11
> > ++-
> > include/linux/hid-sensor-ids.h| 1 +
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > index d349ace2e33f..b685c292a179 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > @@ -480,7 +480,7 @@ int hid_sensor_parse_common_attributes(struct
> > hid_sensor_hub_device *hsdev,
> >
> > /*
> > * Set Sensitivity field ids, when there is no individual
> > modifier, will
> > -* check absolute sensitivity of data field
> > +* check absolute sensitivity and relative sensitivity of data
> > field
> > */
> > for (i = 0; i < sensitivity_addresses_len && st-
> > >sensitivity.index < 0; i++) {
> > sensor_hub_input_get_attribute_info(hsdev,
> > @@ -488,6 +488,15 @@ int hid_sensor_parse_common_attributes(struct
> > hid_sensor_hub_device *hsdev,
> > HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSIT
> > IVITY_ABS |
> > sensitivity_addresses[i],
> > >sensitivity);
> > +
> > + if (st->sensitivity.index >= 0)
> > + break;
> > +
> > + sensor_hub_input_get_attribute_info(hsdev,
> > + HID_FEATURE_REPORT, usage_id,
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSIT
> > IVITY_REL_PCT |
> > + sensitivity_addresses[i],
> > + >sensitivity);
>
> We can't provide the value to userspace without reflecting the
> difference between
> the two ways of expressing it.
>
> It seems there are 3 ways sensitivity is expressed.
> 1. Raw value in same units as the measurement (easy one and what is
> currently reported)
> 2. Percentage of range - also relatively easy to transform into the
> same as 1.
> 3. Percentage of prior reading.. This one doesn't fit in any
> existing ABI, so
>unfortunately we'll have to invent something new along the lines
> of
>*_hysteresis_relative
This is why it was not added before when I developed. But later few
years back there was a patch to add this by one of our developer. There
was some discussion, I thought it was decided it is OK to add.
But I agree, we should add new ABI as you suggested. Now almost every
laptop has HID sensors, better to address this.
Thanks,
Srinivas
> Jonathan
>
>
>
> > }
> >
> > st->raw_hystersis = -1;
> > diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> > sensor-ids.h
> > index 3bbdbccc5805..ac631159403a 100644
> > --- a/include/linux/hid-sensor-ids.h
> > +++ b/include/linux/hid-sensor-ids.h
> > @@ -149,6 +149,7 @@
> > /* Per data field properties */
> > #define HID_USAGE_SENSOR_DATA_MOD_NONE
> > 0x00
> > #define HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS
> > 0x1000
> > +#define
> > HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_REL_PCT0xE
> > 000
> >
> > /* Power state enumerations */
> > #define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x20085
> > 0
Re: [PATCH 2/2][v2] cpufreq: intel_pstate: Get percpu max freq via HWP MSR register if available
On Mon, 2021-01-11 at 15:43 +0800, Chen Yu wrote:
> Currently when turbo is disabled(either by BIOS or by the user), the
> intel_pstate
> driver reads the max frequency from the package-wide
> MSR_PLATFORM_INFO(0xce) register.
> However on asymmetric platforms it is possible in theory that small
> and big core with
> HWP enabled might have different max cpu frequency
max non-turbo frequency (although code call max_freq).
Thanks,
Srinivas
> , because the MSR_HWP_CAPABILITIES
> is percpu scope according to Intel Software Developer Manual.
>
> The turbo max freq is already percpu basis in current code, thus make
> similar change
> to the max non-turbo frequency as well.
>
> Reported-by: Wendy Wang
> Signed-off-by: Chen Yu
> ---
> v2: per Srinivas' suggestion, avoid duplicated assignment of
> max_pstate.
> --
> drivers/cpufreq/intel_pstate.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index bd3dd1be73ba..f2d18991d969 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1725,11 +1725,9 @@ static void
> intel_pstate_max_within_limits(struct cpudata *cpu)
> static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
> {
> cpu->pstate.min_pstate = pstate_funcs.get_min();
> - cpu->pstate.max_pstate = pstate_funcs.get_max();
> cpu->pstate.max_pstate_physical =
> pstate_funcs.get_max_physical();
> cpu->pstate.turbo_pstate = pstate_funcs.get_turbo();
> cpu->pstate.scaling = pstate_funcs.get_scaling();
> - cpu->pstate.max_freq = cpu->pstate.max_pstate * cpu-
> >pstate.scaling;
>
> if (hwp_active && !hwp_mode_bdw) {
> unsigned int phy_max, current_max, guar_state;
> @@ -1737,8 +1735,12 @@ static void
> intel_pstate_get_cpu_pstates(struct cpudata *cpu)
> intel_pstate_get_hwp_max(cpu, _max, _max,
> _state);
> cpu->pstate.turbo_freq = phy_max * cpu-
> >pstate.scaling;
> cpu->pstate.turbo_pstate = phy_max;
> + cpu->pstate.max_pstate = guar_state;
> + cpu->pstate.max_freq = guar_state * cpu-
> >pstate.scaling;
> } else {
> cpu->pstate.turbo_freq = cpu->pstate.turbo_pstate *
> cpu->pstate.scaling;
> + cpu->pstate.max_pstate = pstate_funcs.get_max();
> + cpu->pstate.max_freq = cpu->pstate.max_pstate * cpu-
> >pstate.scaling;
> }
>
> if (pstate_funcs.get_aperf_mperf_shift)
Re: [PATCH] cpufreq: intel_pstate: Use HWP capabilities in intel_cpufreq_adjust_perf()
On Tue, 2021-01-05 at 19:20 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> If turbo P-states cannot be used, either due to the configuration of
> the processor, or because intel_pstate is not allowed to used them,
> the maximum available P-state with HWP enabled corresponds to the
> HWP_CAP.GUARANTEED value which is not static. It can be adjusted by
> an out-of-band agent or during an Intel Speed Select performance
> level change, so long as it remains less than or equal to
> HWP_CAP.MAX.
>
> However, if turbo P-states cannot be used,
> intel_cpufreq_adjust_perf()
> always uses pstate.max_pstate (set during the initialization of the
> driver only) as the maximum available P-state, so it may miss a
> change
> of the HWP_CAP.GUARANTEED value.
>
> Prevent that from happening by modifyig intel_cpufreq_adjust_perf()
> to always read the "guaranteed" and "maximum turbo" performance
> levels from the cached HWP_CAP value.
>
> Signed-off-by: Rafael J. Wysocki
Acked-by: Srinivas Pandruvada
> ---
> drivers/cpufreq/intel_pstate.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2653,12 +2653,13 @@ static void intel_cpufreq_adjust_perf(un
> unsigned long capacity)
> {
> struct cpudata *cpu = all_cpu_data[cpunum];
> + u64 hwp_cap = READ_ONCE(cpu->hwp_cap_cached);
> int old_pstate = cpu->pstate.current_pstate;
> int cap_pstate, min_pstate, max_pstate, target_pstate;
>
> update_turbo_state();
> - cap_pstate = global.turbo_disabled ? cpu->pstate.max_pstate :
> - cpu-
> >pstate.turbo_pstate;
> + cap_pstate = global.turbo_disabled ?
> HWP_GUARANTEED_PERF(hwp_cap) :
> +
> HWP_HIGHEST_PERF(hwp_cap);
>
> /* Optimization: Avoid unnecessary divisions. */
>
>
>
>
Re: [PATCH v3 0/6] add timestamp channel for hid-sensors
On Tue, 2021-01-05 at 15:21 +0800, Ye Xiang wrote: > This patch series add a timestamp channel for hid sensors, > including gravity sensor, gyro sensor, magnetometer sensor, > ambient light sensor, inclinometer sensor, and rotation sensor. > > With this patch series, user can get the time when sensor yield > a sample. > I think this series is v1 for upstream not v3. Thanks, Srinivas > --- > v3: > - hid-sensor-magn-3d: fix iio_val buffer len issue. > - hid-sensor-accel-3d: refine commit message > > v2: > - remove unrelated changes. > > Ye Xiang (6): > iio: hid-sensor-accel-3d: Add timestamp channel for gravity sensor > iio: hid-sensor-gyro-3d: Add timestamp channel > iio: hid-sensor-als: Add timestamp channel > iio: hid-sensor-magn-3d: Add timestamp channel > iio: hid-sensor-incl-3d: Add timestamp channel > iio: hid-sensor-rotation: Add timestamp channel > > drivers/iio/accel/hid-sensor-accel-3d.c | 6 ++- > drivers/iio/gyro/hid-sensor-gyro-3d.c | 40 +--- > drivers/iio/light/hid-sensor-als.c | 39 --- > drivers/iio/magnetometer/hid-sensor-magn-3d.c | 48 - > -- > drivers/iio/orientation/hid-sensor-incl-3d.c | 43 ++--- > drivers/iio/orientation/hid-sensor-rotation.c | 46 ++--- > - > 6 files changed, 134 insertions(+), 88 deletions(-) >
Re: [PATCH 0/2] hid: intel-ish-hid: ipc: enable OOB support for EHL
On Mon, 2021-01-04 at 16:12 +0100, Jiri Kosina wrote: > On Wed, 16 Dec 2020, Zhang Lixu wrote: > > > The EHL (Elkhart Lake) based platforms provide a OOB (Out of band) > > service, which allows wakup device when the system is in S5 (Soft- > > Off > > state). This OOB service can be enabled/disabled from BIOS > > settings. > > > > These two patches is to enable this feature for EHL platform. > > > > We have tested these patches on both ISH platforms and EHL > > platforms, > > it works fine. > > > > Zhang Lixu (2): > > hid: intel-ish-hid: ipc: finish power flow for EHL OOB > > hid: intel-ish-hid: ipc: Address EHL Sx resume issues > > > > drivers/hid/intel-ish-hid/ipc/hw-ish.h | 1 + > > drivers/hid/intel-ish-hid/ipc/ipc.c | 27 + > > drivers/hid/intel-ish-hid/ipc/pci-ish.c | 54 > > - > > 3 files changed, 81 insertions(+), 1 deletion(-) > > Srinivas, can I please get your Acked-by / Reviewed-by for this? Acked on individual patches. Thanks, Srinivas > Thanks, >
Re: [PATCH 2/2] hid: intel-ish-hid: ipc: Address EHL Sx resume issues
On Wed, 2020-12-16 at 14:36 +0800, Zhang Lixu wrote:
> When OOB is disabled, FW will be power gated when system is in
> S3/S4/S5
> which is the same behavior with legacy ISH FW.
> When OOB is enabled, FW will always power on which is totally
> different
> comparing to legacy ISH FW.
>
> So NO_D3 flag is not enough to check FW's status after resume.
> Here we can use IPC FW status register to check host link status.
> If it is false, it means FW get reset after power gated, need go
> through
> the whole initialization flow; If it is true, it means FW is alive,
> just
> set host ready bit to let fw know host is up.
>
> Co-developed-by: Wei Jiang
> Signed-off-by: Wei Jiang
> Signed-off-by: Even Xu
> Signed-off-by: Zhang Lixu
Acked-by: Srinivas Pandruvada
> ---
> drivers/hid/intel-ish-hid/ipc/hw-ish.h | 1 +
> drivers/hid/intel-ish-hid/ipc/ipc.c | 27
> +
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 6 +-
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> index 1fb294ca463e..111ad259ba74 100644
> --- a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> +++ b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> @@ -81,5 +81,6 @@ struct ishtp_device *ish_dev_init(struct pci_dev
> *pdev);
> int ish_hw_start(struct ishtp_device *dev);
> void ish_device_disable(struct ishtp_device *dev);
> int ish_disable_dma(struct ishtp_device *dev);
> +void ish_set_host_ready(struct ishtp_device *dev);
>
> #endif /* _ISHTP_HW_ISH_H_ */
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-
> ish-hid/ipc/ipc.c
> index a45ac7fa417b..47bbeb8b492b 100644
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -193,6 +193,33 @@ static void ish_clr_host_rdy(struct ishtp_device
> *dev)
> ish_reg_write(dev, IPC_REG_HOST_COMM, host_status);
> }
>
> +static bool ish_chk_host_rdy(struct ishtp_device *dev)
> +{
> + uint32_t host_status = ish_reg_read(dev, IPC_REG_HOST_COMM);
> +
> + return (host_status & IPC_HOSTCOMM_READY_BIT);
> +}
> +
> +/**
> + * ish_set_host_ready() - reconfig ipc host registers
> + * @dev: ishtp device pointer
> + *
> + * Set host to ready state
> + * This API is called in some case:
> + * fw is still on, but ipc is powered down.
> + * such as OOB case.
> + *
> + * Return: 0 for success else error fault code
> + */
> +void ish_set_host_ready(struct ishtp_device *dev)
> +{
> + if (ish_chk_host_rdy(dev))
> + return;
> +
> + ish_set_host_rdy(dev);
> + set_host_ready(dev);
> +}
> +
> /**
> * _ishtp_read_hdr() - Read message header
> * @dev: ISHTP device pointer
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index c9c5488e44cb..8cb40696984a 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -259,11 +259,15 @@ static void __maybe_unused
> ish_resume_handler(struct work_struct *work)
> {
> struct pci_dev *pdev = to_pci_dev(ish_resume_device);
> struct ishtp_device *dev = pci_get_drvdata(pdev);
> + uint32_t fwsts = dev->ops->get_fw_status(dev);
> int ret;
>
> - if (ish_should_leave_d0i3(pdev) && !dev->suspend_flag) {
> + if (ish_should_leave_d0i3(pdev) && !dev->suspend_flag
> + && IPC_IS_ISH_ILUP(fwsts)) {
> disable_irq_wake(pdev->irq);
>
> + ish_set_host_ready(dev);
> +
> ishtp_send_resume(dev);
>
> /* Waiting to get resume response */
Re: [PATCH 1/2] hid: intel-ish-hid: ipc: finish power flow for EHL OOB
On Wed, 2020-12-16 at 14:36 +0800, Zhang Lixu wrote:
> The EHL (Elkhart Lake) based platforms provide a OOB (Out of band)
> service, which allows wakup device when the system is in S5 (Soft-Off
> state). This OOB service can be enabled/disabled from BIOS settings.
> When
> enabled, the ISH device gets PME wake capability. To enable PME
> wakeup,
> driver also needs to enable ACPI GPE bit.
>
> Once wakeup, BIOS will clear the wakeup bit to identify wakeup is
> successful. So driver need to re-enable it in resume function to
> keep the next wakeup capability.
>
> Since this feature is only present on EHL, we use EHL PCI device id
> to
> enable this feature.
>
> Co-developed-by: Najumon Ba
> Signed-off-by: Najumon Ba
> Signed-off-by: Even Xu
> Signed-off-by: Zhang Lixu
Acked-by: Srinivas Pandruvada
> ---
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 48
> +
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index c6d48a8648b7..c9c5488e44cb 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -5,6 +5,7 @@
> * Copyright (c) 2014-2016, Intel Corporation.
> */
>
> +#include
> #include
> #include
> #include
> @@ -111,6 +112,42 @@ static inline bool ish_should_leave_d0i3(struct
> pci_dev *pdev)
> return !pm_resume_via_firmware() || pdev->device ==
> CHV_DEVICE_ID;
> }
>
> +static int enable_gpe(struct device *dev)
> +{
> +#ifdef CONFIG_ACPI
> + acpi_status acpi_sts;
> + struct acpi_device *adev;
> + struct acpi_device_wakeup *wakeup;
> +
> + adev = ACPI_COMPANION(dev);
> + if (!adev) {
> + dev_err(dev, "get acpi handle failed\n");
> + return -ENODEV;
> + }
> + wakeup = >wakeup;
> +
> + acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> >gpe_number);
> + if (ACPI_FAILURE(acpi_sts)) {
> + dev_err(dev, "enable ose_gpe failed\n");
> + return -EIO;
> + }
> +
> + return 0;
> +#else
> + return -ENODEV;
> +#endif
> +}
> +
> +static void enable_pme_wake(struct pci_dev *pdev)
> +{
> + if ((pci_pme_capable(pdev, PCI_D0) ||
> + pci_pme_capable(pdev, PCI_D3hot) ||
> + pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(
> >dev)) {
> + pci_pme_active(pdev, true);
> + dev_dbg(>dev, "ish ipc driver pme wake
> enabled\n");
> + }
> +}
> +
> /**
> * ish_probe() - PCI driver probe callback
> * @pdev: pci device
> @@ -179,6 +216,10 @@ static int ish_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> init_waitqueue_head(>suspend_wait);
> init_waitqueue_head(>resume_wait);
>
> + /* Enable PME for EHL */
> + if (pdev->device == EHL_Ax_DEVICE_ID)
> + enable_pme_wake(pdev);
> +
> ret = ish_init(ishtp);
> if (ret)
> return ret;
> @@ -317,6 +358,13 @@ static int __maybe_unused ish_resume(struct
> device *device)
> struct pci_dev *pdev = to_pci_dev(device);
> struct ishtp_device *dev = pci_get_drvdata(pdev);
>
> + /* add this to finish power flow for EHL */
> + if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> + pci_set_power_state(pdev, PCI_D0);
> + enable_pme_wake(pdev);
> + dev_dbg(dev->devc, "set power state to D0 for
> ehl\n");
> + }
> +
> ish_resume_device = device;
> dev->resume_flag = 1;
>
Re: tools/power/x86/intel-speed-select: Fixes for regression
On Mon, 2021-01-04 at 12:56 +0100, Hans de Goede wrote:
> Hi,
>
> On 12/21/20 8:18 AM, Srinivas Pandruvada wrote:
> > One side-effect of fixing the scaling frequency limits using the
> > commit eacc9c5a927e ("cpufreq: intel_pstate: Fix
> > intel_pstate_get_hwp_max()
> > for turbo disabled") causes stale HWP_CAP.GUARANTEED to be used as
> > max.
> > Without processing HWP interrupts, user space needs to be able to
> > update
> > a new max while Intel SST is in use. This is not a problem as the
> > change of guaranteed is caused by user space action, so user space
> > knows
> > that guarantee will change.
> >
> > This series causes user space to trigger scaling_max_freq update
> > with
> > the new base_frequency.
> >
> >
> > Srinivas Pandruvada (2):
> > tools/power/x86/intel-speed-select: Set scaling_max_freq to
> > base_frequency
> > tools/power/x86/intel-speed-select: Set higher of
> > cpuinfo_max_freq or
> > base_frequency
>
> Thank you for your patch-series, I've applied the series to my
> review-hans branch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
>
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
I thought of applying these to the next 5.11-rc release.
If you want to queue for the next merge window (5.12) then I have other
enhancements, which I will send a PULL request.
Thanks,
Srinivas
>
> Regards,
>
> Hans
>
Re: tools/power/x86/intel-speed-select: Fixes for regression
Hi Hans,
On Mon, 2021-01-04 at 12:57 +0100, Hans de Goede wrote:
> Hi,
>
> On 1/4/21 12:56 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 12/21/20 8:18 AM, Srinivas Pandruvada wrote:
> > > One side-effect of fixing the scaling frequency limits using the
> > > commit eacc9c5a927e ("cpufreq: intel_pstate: Fix
> > > intel_pstate_get_hwp_max()
> > > for turbo disabled") causes stale HWP_CAP.GUARANTEED to be used
> > > as max.
> > > Without processing HWP interrupts, user space needs to be able to
> > > update
> > > a new max while Intel SST is in use. This is not a problem as the
> > > change of guaranteed is caused by user space action, so user
> > > space knows
> > > that guarantee will change.
> > >
> > > This series causes user space to trigger scaling_max_freq update
> > > with
> > > the new base_frequency.
> > >
> > >
> > > Srinivas Pandruvada (2):
> > > tools/power/x86/intel-speed-select: Set scaling_max_freq to
> > > base_frequency
> > > tools/power/x86/intel-speed-select: Set higher of
> > > cpuinfo_max_freq or
> > > base_frequency
> >
> > Thank you for your patch-series, I've applied the series to my
> > review-hans branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> >
> > Note it will show up in my review-hans branch once I've pushed my
> > local branch there, which might take a while.
> >
> > Once I've run some tests on this branch the patches there will be
> > added to the platform-drivers-x86/for-next branch and eventually
> > will be included in the pdx86 pull-request to Linus for the next
> > merge-window.
>
> Note this is a templated reply, I will also cherry-pick these into
> the
> fixes branch in this case.
>
Then you can ignore my previous email.
Thanks,
Srinivas
> Regards,
>
> Hans
>
Re: [PATCH 1/2] thermal: int340x: Add critical callback to override default shutdown behavior
On Mon, 2020-12-21 at 17:03 +0100, Daniel Lezcano wrote: > On 21/12/2020 16:00, Kai-Heng Feng wrote: > > On Mon, Dec 21, 2020 at 9:59 PM Daniel Lezcano > > wrote: > > > > > > On 21/12/2020 14:52, Kai-Heng Feng wrote: > > > > We are seeing thermal shutdown on Intel based mobile > > > > workstations, the > > > > shutdown happens during the first trip handle in > > > > thermal_zone_device_register(): > > > > kernel: thermal thermal_zone15: critical temperature reached > > > > (101 C), shutting down > > > > > > > > However, we shouldn't do a thermal shutdown here, since > > > > 1) We may want to use a dedicated daemon, Intel's thermald in > > > > this case, > > > > to handle thermal shutdown. > > > > > > > > 2) For ACPI based system, _CRT doesn't mean shutdown unless > > > > it's inside > > > > ThermalZone namespace. ACPI Spec, 11.4.4 _CRT (Critical > > > > Temperature): > > > > "... If this object it present under a device, the device’s > > > > driver > > > > evaluates this object to determine the device’s critical > > > > cooling > > > > temperature trip point. This value may then be used by the > > > > device’s > > > > driver to program an internal device temperature sensor trip > > > > point." > > > > > > > > So a "critical trip" here merely means we should take a more > > > > aggressive > > > > cooling method. > > > > > > > > As int340x device isn't present under ACPI ThermalZone, > > > > override the > > > > default .critical callback to prevent surprising thermal > > > > shutdown. > > > > > > > > Signed-off-by: Kai-Heng Feng > > > > > > I'll submit those changes for v5.11-rc1 and change the subject > > > by: > > > > > > thermal: int340x: Fix unexpected shutdown at critical temperature > > > thermal: pch: Fix unexpected shutdown at critical temperature > > > > > > Sounds good ? > > > > Sounds good to me. Thanks! > > > > Kai-Heng > > Rui, Srinivas? Are you ok with the changes ? I have minor comment in one of the patch. But that is harmless. So changes are fine. Thanks, Srinivas > >
Re: [PATCH 1/2] thermal: int340x: Add critical callback to override default shutdown behavior
On Mon, 2020-12-21 at 21:52 +0800, Kai-Heng Feng wrote:
> We are seeing thermal shutdown on Intel based mobile workstations,
> the
> shutdown happens during the first trip handle in
> thermal_zone_device_register():
> kernel: thermal thermal_zone15: critical temperature reached (101 C),
> shutting down
>
> However, we shouldn't do a thermal shutdown here, since
> 1) We may want to use a dedicated daemon, Intel's thermald in this
> case,
> to handle thermal shutdown.
>
> 2) For ACPI based system, _CRT doesn't mean shutdown unless it's
> inside
> ThermalZone namespace. ACPI Spec, 11.4.4 _CRT (Critical Temperature):
> "... If this object it present under a device, the device’s driver
> evaluates this object to determine the device’s critical cooling
> temperature trip point. This value may then be used by the device’s
> driver to program an internal device temperature sensor trip point."
>
> So a "critical trip" here merely means we should take a more
> aggressive
> cooling method.
>
> As int340x device isn't present under ACPI ThermalZone, override the
> default .critical callback to prevent surprising thermal shutdown.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 6
> ++
> .../thermal/intel/int340x_thermal/int340x_thermal_zone.c | 6
> ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 823354a1a91a..9778a6dba939 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -431,9 +431,15 @@ static int int3400_thermal_change_mode(struct
> thermal_zone_device *thermal,
> return result;
> }
>
> +static void int3400_thermal_critical(struct thermal_zone_device
> *thermal)
> +{
> + dev_dbg(>device, "%s: critical temperature
> reached\n", thermal->type);
> +}
> +
> static struct thermal_zone_device_ops int3400_thermal_ops = {
> .get_temp = int3400_thermal_get_temp,
> .change_mode = int3400_thermal_change_mode,
> + .critical = int3400_thermal_critical,
> };
You don't need for int3400 device. This is a fake sensor.
>
> static struct thermal_zone_params int3400_thermal_params = {
> diff --git
> a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> index 6e479deff76b..d1248ba943a4 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -146,12 +146,18 @@ static int int340x_thermal_get_trip_hyst(struct
> thermal_zone_device *zone,
> return 0;
> }
>
> +static void int340x_thermal_critical(struct thermal_zone_device
> *zone)
> +{
> + dev_dbg(>device, "%s: critical temperature reached\n",
> zone->type);
> +}
> +
> static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
> .get_temp = int340x_thermal_get_zone_temp,
> .get_trip_temp = int340x_thermal_get_trip_temp,
> .get_trip_type = int340x_thermal_get_trip_type,
> .set_trip_temp = int340x_thermal_set_trip_temp,
> .get_trip_hyst = int340x_thermal_get_trip_hyst,
> + .critical = int340x_thermal_critical,
> };
>
> static int int340x_thermal_get_trip_config(acpi_handle handle, char
> *name,
Thanks,
Srinivas
[PATCH 1/2] tools/power/x86/intel-speed-select: Set scaling_max_freq to base_frequency
When BIOS disables turbo, The scaling_max_freq in cpufreq sysfs will be
limited to config level 0 base frequency. But when user selects a higher
config levels, this will result in higher base frequency. But since
scaling_max_freq is still old base frequency, the performance will still
be limited. So when the turbo is disabled and cpufreq base_frequency is
higher than scaling_max_freq, update the scaling_max_freq to the
base_frequency.
Signed-off-by: Srinivas Pandruvada
---
.../x86/intel-speed-select/isst-config.c | 21 +++
1 file changed, 21 insertions(+)
diff --git a/tools/power/x86/intel-speed-select/isst-config.c
b/tools/power/x86/intel-speed-select/isst-config.c
index 5390158cdb40..9b6f0e6f150d 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1249,6 +1249,8 @@ static void dump_isst_config(int arg)
isst_ctdp_display_information_end(outf);
}
+static void adjust_scaling_max_from_base_freq(int cpu);
+
static void set_tdp_level_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
void *arg4)
{
@@ -1267,6 +1269,9 @@ static void set_tdp_level_for_cpu(int cpu, void *arg1,
void *arg2, void *arg3,
int pkg_id = get_physical_package_id(cpu);
int die_id = get_physical_die_id(cpu);
+ /* Wait for updated base frequencies */
+ usleep(2000);
+
fprintf(stderr, "Option is set to online/offline\n");
ctdp_level.core_cpumask_size =
alloc_cpu_set(_level.core_cpumask);
@@ -1283,6 +1288,7 @@ static void set_tdp_level_for_cpu(int cpu, void *arg1,
void *arg2, void *arg3,
if (CPU_ISSET_S(i,
ctdp_level.core_cpumask_size, ctdp_level.core_cpumask)) {
fprintf(stderr, "online cpu
%d\n", i);
set_cpu_online_offline(i, 1);
+
adjust_scaling_max_from_base_freq(i);
} else {
fprintf(stderr, "offline cpu
%d\n", i);
set_cpu_online_offline(i, 0);
@@ -1440,6 +1446,21 @@ static int set_cpufreq_scaling_min_max(int cpu, int max,
int freq)
return 0;
}
+static int no_turbo(void)
+{
+ return parse_int_file(0,
"/sys/devices/system/cpu/intel_pstate/no_turbo");
+}
+
+static void adjust_scaling_max_from_base_freq(int cpu)
+{
+ int base_freq, scaling_max_freq;
+
+ scaling_max_freq = parse_int_file(0,
"/sys/devices/system/cpu/cpu%d/cpufreq/scaling_max_freq", cpu);
+ base_freq = get_cpufreq_base_freq(cpu);
+ if (scaling_max_freq < base_freq || no_turbo())
+ set_cpufreq_scaling_min_max(cpu, 1, base_freq);
+}
+
static int set_clx_pbf_cpufreq_scaling_min_max(int cpu)
{
struct isst_pkg_ctdp_level_info *ctdp_level;
--
2.29.2
[PATCH 2/2] tools/power/x86/intel-speed-select: Set higher of cpuinfo_max_freq or base_frequency
In some case when BIOS disabled turbo, cpufreq cpuinfo_max_freq can be
lower than base_frequency at higher config level. So, in that case set
scaling_min_freq to base_frequency.
Signed-off-by: Srinivas Pandruvada
---
tools/power/x86/intel-speed-select/isst-config.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tools/power/x86/intel-speed-select/isst-config.c
b/tools/power/x86/intel-speed-select/isst-config.c
index 9b6f0e6f150d..09cb3a6672f3 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1461,6 +1461,16 @@ static void adjust_scaling_max_from_base_freq(int cpu)
set_cpufreq_scaling_min_max(cpu, 1, base_freq);
}
+static void adjust_scaling_min_from_base_freq(int cpu)
+{
+ int base_freq, scaling_min_freq;
+
+ scaling_min_freq = parse_int_file(0,
"/sys/devices/system/cpu/cpu%d/cpufreq/scaling_min_freq", cpu);
+ base_freq = get_cpufreq_base_freq(cpu);
+ if (scaling_min_freq < base_freq)
+ set_cpufreq_scaling_min_max(cpu, 0, base_freq);
+}
+
static int set_clx_pbf_cpufreq_scaling_min_max(int cpu)
{
struct isst_pkg_ctdp_level_info *ctdp_level;
@@ -1558,6 +1568,7 @@ static void set_scaling_min_to_cpuinfo_max(int cpu)
continue;
set_cpufreq_scaling_min_max_from_cpuinfo(i, 1, 0);
+ adjust_scaling_min_from_base_freq(i);
}
}
--
2.29.2
tools/power/x86/intel-speed-select: Fixes for regression
One side-effect of fixing the scaling frequency limits using the
commit eacc9c5a927e ("cpufreq: intel_pstate: Fix intel_pstate_get_hwp_max()
for turbo disabled") causes stale HWP_CAP.GUARANTEED to be used as max.
Without processing HWP interrupts, user space needs to be able to update
a new max while Intel SST is in use. This is not a problem as the
change of guaranteed is caused by user space action, so user space knows
that guarantee will change.
This series causes user space to trigger scaling_max_freq update with
the new base_frequency.
Srinivas Pandruvada (2):
tools/power/x86/intel-speed-select: Set scaling_max_freq to
base_frequency
tools/power/x86/intel-speed-select: Set higher of cpuinfo_max_freq or
base_frequency
.../x86/intel-speed-select/isst-config.c | 32 +++
1 file changed, 32 insertions(+)
--
2.29.2
Re: [PATCH] cpufreq: intel_pstate: Use most recent guaranteed performance values
On Thu, 2020-12-17 at 20:17 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> When turbo has been disabled by the BIOS, but HWP_CAP.GUARANTEED is
> changed later, user space may want to take advantage of this
> increased
> guaranteed performance.
>
> HWP_CAP.GUARANTEED is not a static value. It can be adjusted by an
> out-of-band agent or during an Intel Speed Select performance level
> change. The HWP_CAP.MAX is still the maximum achievable performance
> with turbo disabled by the BIOS, so HWP_CAP.GUARANTEED can still
> change as long as it remains less than or equal to HWP_CAP.MAX.
>
> When HWP_CAP.GUARANTEED is changed, the sysfs base_frequency
> attribute shows the most recent guaranteed frequency value. This
> attribute can be used by user space software to update the scaling
> min/max limits of the CPU.
>
> Currently, the ->setpolicy() callback already uses the latest
> HWP_CAP values when setting HWP_REQ, but the ->verify() callback will
> restrict the user settings to the to old guaranteed performance value
> which prevents user space from making use of the extra CPU capacity
> theoretically available to it after increasing HWP_CAP.GUARANTEED.
>
> To address this, read HWP_CAP in intel_pstate_verify_cpu_policy()
> to obtain the maximum P-state that can be used and use that to
> confine the policy max limit instead of using the cached and
> possibly stale pstate.max_freq value for this purpose.
>
> For consistency, update intel_pstate_update_perf_limits() to use the
> maximum available P-state returned by intel_pstate_get_hwp_max() to
> compute the maximum frequency instead of using the return value of
> intel_pstate_get_max_freq() which, again, may be stale.
>
> This issue is a side-effect of fixing the scaling frequency limits in
> commit eacc9c5a927e ("cpufreq: intel_pstate: Fix
> intel_pstate_get_hwp_max()
> for turbo disabled") which currected
corrected
Thanks,
Srinivas
> the setting of the reduced scaling
> frequency values, but caused stale HWP_CAP.GUARANTEED to be used in
> the case at hand.
>
> Fixes: eacc9c5a927e ("cpufreq: intel_pstate: Fix
> intel_pstate_get_hwp_max() for turbo disabled")
> Reported-by: Srinivas Pandruvada >
> Tested-by: Srinivas Pandruvada
> Cc: 5.8+ # 5.8+
> Signed-off-by: Rafael J. Wysocki
> ---
> drivers/cpufreq/intel_pstate.c | 16 +---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2207,9 +2207,9 @@ static void intel_pstate_update_perf_lim
> unsigned int policy_min,
> unsigned int policy_max)
> {
> - int max_freq = intel_pstate_get_max_freq(cpu);
> int32_t max_policy_perf, min_policy_perf;
> int max_state, turbo_max;
> + int max_freq;
>
> /*
>* HWP needs some special consideration, because on BDX the
> @@ -2223,6 +2223,7 @@ static void intel_pstate_update_perf_lim
> cpu->pstate.max_pstate : cpu-
> >pstate.turbo_pstate;
> turbo_max = cpu->pstate.turbo_pstate;
> }
> + max_freq = max_state * cpu->pstate.scaling;
>
> max_policy_perf = max_state * policy_max / max_freq;
> if (policy_max == policy_min) {
> @@ -2325,9 +2326,18 @@ static void intel_pstate_adjust_policy_m
> static void intel_pstate_verify_cpu_policy(struct cpudata *cpu,
> struct cpufreq_policy_data
> *policy)
> {
> + int max_freq;
> +
> update_turbo_state();
> - cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> - intel_pstate_get_max_freq(cpu));
> + if (hwp_active) {
> + int max_state, turbo_max;
> +
> + intel_pstate_get_hwp_max(cpu->cpu, _max,
> _state);
> + max_freq = max_state * cpu->pstate.scaling;
> + } else {
> + max_freq = intel_pstate_get_max_freq(cpu);
> + }
> + cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> max_freq);
>
> intel_pstate_adjust_policy_max(cpu, policy);
> }
>
>
>
Re: [PATCH] cpufreq: intel_pstate: Use the latest guaranteed freq during verify
On Thu, 2020-12-17 at 16:24 +0100, Rafael J. Wysocki wrote:
> On Thu, Dec 17, 2020 at 4:21 PM Srinivas Pandruvada
> wrote:
> >
> > On Thu, 2020-12-17 at 16:12 +0100, Rafael J. Wysocki wrote:
> > > On Thursday, December 17, 2020 3:23:44 PM CET Srinivas Pandruvada
> > > wrote:
> > > > On Thu, 2020-12-17 at 06:19 -0800, Srinivas Pandruvada wrote:
> > > > > On Thu, 2020-12-17 at 14:58 +0100, Rafael J. Wysocki wrote:
> > > > > > On Thu, Dec 17, 2020 at 11:44 AM Srinivas Pandruvada
> > > > > > wrote:
> > > > > > >
> > > > > > > This change tries to address an issue, when BIOS disabled
> > > > > > > turbo
> > > > > > > but HWP_CAP guaranteed is changed later and user space
> > > > > > > wants
> > > > > > > to
> > > > > > > take
> > > > > > > advantage of this increased guaranteed performance.
> > > > > > >
> > > > > > > The HWP_CAP.GUARANTEED value is not a static value. It
> > > > > > > can be
> > > > > > > changed
> > > > > > > by some out of band agent or during Intel Speed Select
> > > > > > > performance
> > > > > > > level change. The HWP_CAP.MAX still shows max possible
> > > > > > > performance
> > > > > > > when
> > > > > > > BIOS disabled turbo. So guaranteed can still change as
> > > > > > > long
> > > > > > > as
> > > > > > > this
> > > > > > > is
> > > > > > > same or below HWP_CAP.MAX.
> > > > > > >
> > > > > > > When guaranteed is changed, the sysfs base_frequency
> > > > > > > attributes
> > > > > > > shows
> > > > > > > the latest guaranteed frequency. This attribute can be
> > > > > > > used
> > > > > > > by
> > > > > > > user
> > > > > > > space software to update scaling min/max frequency.
> > > > > > >
> > > > > > > Currently the setpolicy callback already uses the latest
> > > > > > > HWP_CAP
> > > > > > > values when setting HWP_REQ. But the verify callback will
> > > > > > > still
> > > > > > > restrict
> > > > > > > the user settings to the to old guaranteed value. So if
> > > > > > > the
> > > > > > > guaranteed
> > > > > > > is increased, user space can't take advantage of it.
> > > > > > >
> > > > > > > To solve this similar to setpolicy callback, read the
> > > > > > > latest
> > > > > > > HWP_CAP
> > > > > > > values and use it to restrict the maximum setting. This
> > > > > > > is
> > > > > > > done
> > > > > > > by
> > > > > > > calling intel_pstate_get_hwp_max(), which already
> > > > > > > accounts
> > > > > > > for
> > > > > > > user
> > > > > > > and BIOS turbo disable to get the current max
> > > > > > > performance.
> > > > > > >
> > > > > > > This issue is side effect of fixing the issue of scaling
> > > > > > > frequency
> > > > > > > limits by the
> > > > > > > 'commit eacc9c5a927e ("cpufreq: intel_pstate:
> > > > > > > Fix intel_pstate_get_hwp_max() for turbo disabled")'
> > > > > > > The fix resulted in correct setting of reduced scaling
> > > > > > > frequencies,
> > > > > > > but this resulted in capping HWP.REQ to
> > > > > > > HWP_CAP.GUARANTEED in
> > > > > > > this
> > > > > > > case.
> > > > > > >
> > > > > > > Cc: 5.8+ # 5.8+
> > > > > > > Signed-off-by: Srinivas Pandruvada <
> > > > > > > srinivas.pandruv...@linux.intel.com>
> > > > > > > ---
> > > > > > > drivers/cpufreq/intel_pstate.c | 6 ++
> > > > > > > 1 file changed, 6 insertions(+)
> > > > >
Re: [PATCH] cpufreq: intel_pstate: Use the latest guaranteed freq during verify
On Thu, 2020-12-17 at 16:12 +0100, Rafael J. Wysocki wrote:
> On Thursday, December 17, 2020 3:23:44 PM CET Srinivas Pandruvada
> wrote:
> > On Thu, 2020-12-17 at 06:19 -0800, Srinivas Pandruvada wrote:
> > > On Thu, 2020-12-17 at 14:58 +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Dec 17, 2020 at 11:44 AM Srinivas Pandruvada
> > > > wrote:
> > > > >
> > > > > This change tries to address an issue, when BIOS disabled
> > > > > turbo
> > > > > but HWP_CAP guaranteed is changed later and user space wants
> > > > > to
> > > > > take
> > > > > advantage of this increased guaranteed performance.
> > > > >
> > > > > The HWP_CAP.GUARANTEED value is not a static value. It can be
> > > > > changed
> > > > > by some out of band agent or during Intel Speed Select
> > > > > performance
> > > > > level change. The HWP_CAP.MAX still shows max possible
> > > > > performance
> > > > > when
> > > > > BIOS disabled turbo. So guaranteed can still change as long
> > > > > as
> > > > > this
> > > > > is
> > > > > same or below HWP_CAP.MAX.
> > > > >
> > > > > When guaranteed is changed, the sysfs base_frequency
> > > > > attributes
> > > > > shows
> > > > > the latest guaranteed frequency. This attribute can be used
> > > > > by
> > > > > user
> > > > > space software to update scaling min/max frequency.
> > > > >
> > > > > Currently the setpolicy callback already uses the latest
> > > > > HWP_CAP
> > > > > values when setting HWP_REQ. But the verify callback will
> > > > > still
> > > > > restrict
> > > > > the user settings to the to old guaranteed value. So if the
> > > > > guaranteed
> > > > > is increased, user space can't take advantage of it.
> > > > >
> > > > > To solve this similar to setpolicy callback, read the latest
> > > > > HWP_CAP
> > > > > values and use it to restrict the maximum setting. This is
> > > > > done
> > > > > by
> > > > > calling intel_pstate_get_hwp_max(), which already accounts
> > > > > for
> > > > > user
> > > > > and BIOS turbo disable to get the current max performance.
> > > > >
> > > > > This issue is side effect of fixing the issue of scaling
> > > > > frequency
> > > > > limits by the
> > > > > 'commit eacc9c5a927e ("cpufreq: intel_pstate:
> > > > > Fix intel_pstate_get_hwp_max() for turbo disabled")'
> > > > > The fix resulted in correct setting of reduced scaling
> > > > > frequencies,
> > > > > but this resulted in capping HWP.REQ to HWP_CAP.GUARANTEED in
> > > > > this
> > > > > case.
> > > > >
> > > > > Cc: 5.8+ # 5.8+
> > > > > Signed-off-by: Srinivas Pandruvada <
> > > > > srinivas.pandruv...@linux.intel.com>
> > > > > ---
> > > > > drivers/cpufreq/intel_pstate.c | 6 ++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > > > b/drivers/cpufreq/intel_pstate.c
> > > > > index 2a4db856222f..7081d1edb22b 100644
> > > > > --- a/drivers/cpufreq/intel_pstate.c
> > > > > +++ b/drivers/cpufreq/intel_pstate.c
> > > > > @@ -2199,6 +2199,12 @@ static void
> > > > > intel_pstate_clear_update_util_hook(unsigned int cpu)
> > > > >
> > > > > static int intel_pstate_get_max_freq(struct cpudata *cpu)
> > > > > {
> > > > > + if (hwp_active) {
> > > > > + int turbo_max, max_state;
> > > > > +
> > > > > + intel_pstate_get_hwp_max(cpu->cpu,
> > > > > _max,
> > > > > _state);
> > > >
> > > > This would cause intel_pstate_get_hwp_max() to be called twice
> > > > in
> > > > intel_pstate_update_perf_limits() which is not perfect.
> > >
> > > We can optimize by using cached value.
> > >
> > >
> > > diff --git a/drivers/cpufre
Re: [PATCH] cpufreq: intel_pstate: Use the latest guaranteed freq during verify
On Thu, 2020-12-17 at 06:19 -0800, Srinivas Pandruvada wrote:
> On Thu, 2020-12-17 at 14:58 +0100, Rafael J. Wysocki wrote:
> > On Thu, Dec 17, 2020 at 11:44 AM Srinivas Pandruvada
> > wrote:
> > >
> > > This change tries to address an issue, when BIOS disabled turbo
> > > but HWP_CAP guaranteed is changed later and user space wants to
> > > take
> > > advantage of this increased guaranteed performance.
> > >
> > > The HWP_CAP.GUARANTEED value is not a static value. It can be
> > > changed
> > > by some out of band agent or during Intel Speed Select
> > > performance
> > > level change. The HWP_CAP.MAX still shows max possible
> > > performance
> > > when
> > > BIOS disabled turbo. So guaranteed can still change as long as
> > > this
> > > is
> > > same or below HWP_CAP.MAX.
> > >
> > > When guaranteed is changed, the sysfs base_frequency attributes
> > > shows
> > > the latest guaranteed frequency. This attribute can be used by
> > > user
> > > space software to update scaling min/max frequency.
> > >
> > > Currently the setpolicy callback already uses the latest HWP_CAP
> > > values when setting HWP_REQ. But the verify callback will still
> > > restrict
> > > the user settings to the to old guaranteed value. So if the
> > > guaranteed
> > > is increased, user space can't take advantage of it.
> > >
> > > To solve this similar to setpolicy callback, read the latest
> > > HWP_CAP
> > > values and use it to restrict the maximum setting. This is done
> > > by
> > > calling intel_pstate_get_hwp_max(), which already accounts for
> > > user
> > > and BIOS turbo disable to get the current max performance.
> > >
> > > This issue is side effect of fixing the issue of scaling
> > > frequency
> > > limits by the
> > > 'commit eacc9c5a927e ("cpufreq: intel_pstate:
> > > Fix intel_pstate_get_hwp_max() for turbo disabled")'
> > > The fix resulted in correct setting of reduced scaling
> > > frequencies,
> > > but this resulted in capping HWP.REQ to HWP_CAP.GUARANTEED in
> > > this
> > > case.
> > >
> > > Cc: 5.8+ # 5.8+
> > > Signed-off-by: Srinivas Pandruvada <
> > > srinivas.pandruv...@linux.intel.com>
> > > ---
> > > drivers/cpufreq/intel_pstate.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > b/drivers/cpufreq/intel_pstate.c
> > > index 2a4db856222f..7081d1edb22b 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -2199,6 +2199,12 @@ static void
> > > intel_pstate_clear_update_util_hook(unsigned int cpu)
> > >
> > > static int intel_pstate_get_max_freq(struct cpudata *cpu)
> > > {
> > > + if (hwp_active) {
> > > + int turbo_max, max_state;
> > > +
> > > + intel_pstate_get_hwp_max(cpu->cpu, _max,
> > > _state);
> >
> > This would cause intel_pstate_get_hwp_max() to be called twice in
> > intel_pstate_update_perf_limits() which is not perfect.
>
> We can optimize by using cached value.
>
>
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 7081d1edb22b..d345c9ef240c 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2223,7 +2223,11 @@ static void
> intel_pstate_update_perf_limits(struct cpudata *cpu,
> * rather than pure ratios.
> */
> if (hwp_active) {
> - intel_pstate_get_hwp_max(cpu->cpu, _max,
> _state);
> + if (global.no_turbo || global.turbo_disabled)
> + max_state = HWP_GUARANTEED_PERF(cpu-
> > hwp_cap_cached);
> + else
> + max_state = HWP_HIGHEST_PERF(cpu-
> > hwp_cap_cached);
Can use ternary operator instead of if..else. to further simplify.
> + turbo_max = HWP_HIGHEST_PERF(cpu->hwp_cached);
> } else {
> max_state = global.no_turbo || global.turbo_disabled
> ?
> cpu->pstate.max_pstate : cpu-
> > pstate.turbo_pstate;
>
>
> Thanks,
> Srinivas
>
>
> >
> > > + return max_state * cpu->pstate.scaling;
> > > + }
> > > return global.turbo_disabled || global.no_turbo ?
> > > cpu->pstate.max_freq : cpu-
> > > > pstate.turbo_freq;
> > > }
> > > --
>
Re: [PATCH] cpufreq: intel_pstate: Use the latest guaranteed freq during verify
On Thu, 2020-12-17 at 14:58 +0100, Rafael J. Wysocki wrote:
> On Thu, Dec 17, 2020 at 11:44 AM Srinivas Pandruvada
> wrote:
> >
> > This change tries to address an issue, when BIOS disabled turbo
> > but HWP_CAP guaranteed is changed later and user space wants to
> > take
> > advantage of this increased guaranteed performance.
> >
> > The HWP_CAP.GUARANTEED value is not a static value. It can be
> > changed
> > by some out of band agent or during Intel Speed Select performance
> > level change. The HWP_CAP.MAX still shows max possible performance
> > when
> > BIOS disabled turbo. So guaranteed can still change as long as this
> > is
> > same or below HWP_CAP.MAX.
> >
> > When guaranteed is changed, the sysfs base_frequency attributes
> > shows
> > the latest guaranteed frequency. This attribute can be used by user
> > space software to update scaling min/max frequency.
> >
> > Currently the setpolicy callback already uses the latest HWP_CAP
> > values when setting HWP_REQ. But the verify callback will still
> > restrict
> > the user settings to the to old guaranteed value. So if the
> > guaranteed
> > is increased, user space can't take advantage of it.
> >
> > To solve this similar to setpolicy callback, read the latest
> > HWP_CAP
> > values and use it to restrict the maximum setting. This is done by
> > calling intel_pstate_get_hwp_max(), which already accounts for user
> > and BIOS turbo disable to get the current max performance.
> >
> > This issue is side effect of fixing the issue of scaling frequency
> > limits by the
> > 'commit eacc9c5a927e ("cpufreq: intel_pstate:
> > Fix intel_pstate_get_hwp_max() for turbo disabled")'
> > The fix resulted in correct setting of reduced scaling frequencies,
> > but this resulted in capping HWP.REQ to HWP_CAP.GUARANTEED in this
> > case.
> >
> > Cc: 5.8+ # 5.8+
> > Signed-off-by: Srinivas Pandruvada <
> > srinivas.pandruv...@linux.intel.com>
> > ---
> > drivers/cpufreq/intel_pstate.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c
> > index 2a4db856222f..7081d1edb22b 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2199,6 +2199,12 @@ static void
> > intel_pstate_clear_update_util_hook(unsigned int cpu)
> >
> > static int intel_pstate_get_max_freq(struct cpudata *cpu)
> > {
> > + if (hwp_active) {
> > + int turbo_max, max_state;
> > +
> > + intel_pstate_get_hwp_max(cpu->cpu, _max,
> > _state);
>
> This would cause intel_pstate_get_hwp_max() to be called twice in
> intel_pstate_update_perf_limits() which is not perfect.
We can optimize by using cached value.
diff --git a/drivers/cpufreq/intel_pstate.c
b/drivers/cpufreq/intel_pstate.c
index 7081d1edb22b..d345c9ef240c 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2223,7 +2223,11 @@ static void
intel_pstate_update_perf_limits(struct cpudata *cpu,
* rather than pure ratios.
*/
if (hwp_active) {
- intel_pstate_get_hwp_max(cpu->cpu, _max,
_state);
+ if (global.no_turbo || global.turbo_disabled)
+ max_state = HWP_GUARANTEED_PERF(cpu-
>hwp_cap_cached);
+ else
+ max_state = HWP_HIGHEST_PERF(cpu-
>hwp_cap_cached);
+ turbo_max = HWP_HIGHEST_PERF(cpu->hwp_cached);
} else {
max_state = global.no_turbo || global.turbo_disabled ?
cpu->pstate.max_pstate : cpu-
>pstate.turbo_pstate;
Thanks,
Srinivas
>
> > + return max_state * cpu->pstate.scaling;
> > + }
> > return global.turbo_disabled || global.no_turbo ?
> > cpu->pstate.max_freq : cpu-
> > >pstate.turbo_freq;
> > }
> > --
Re: [PATCH 3/5] thermal/drivers/acpi: Use hot and critical ops
On Thu, 2020-12-17 at 07:28 +0100, Daniel Lezcano wrote:
> On 10/12/2020 13:15, Daniel Lezcano wrote:
> > The acpi driver wants to do a netlink notification in case of a hot
> > or
> > critical trip point. Implement the corresponding ops to be used for
> > the thermal zone and use them instead of the notify ops.
> >
> > Signed-off-by: Daniel Lezcano
>
> Is there any comment on this patch ?
Looks good to me.
Thanks,
Srinivas
>
> > ---
> > drivers/acpi/thermal.c | 30 ++
> > 1 file changed, 14 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index 12c0ece746f0..b5e4bc9e3282 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -677,27 +677,24 @@ static int thermal_get_trend(struct
> > thermal_zone_device *thermal,
> > return 0;
> > }
> >
> > -
> > -static int thermal_notify(struct thermal_zone_device *thermal, int
> > trip,
> > - enum thermal_trip_type trip_type)
> > +static void acpi_thermal_zone_device_hot(struct
> > thermal_zone_device *thermal)
> > {
> > - u8 type = 0;
> > struct acpi_thermal *tz = thermal->devdata;
> >
> > - if (trip_type == THERMAL_TRIP_CRITICAL)
> > - type = ACPI_THERMAL_NOTIFY_CRITICAL;
> > - else if (trip_type == THERMAL_TRIP_HOT)
> > - type = ACPI_THERMAL_NOTIFY_HOT;
> > - else
> > - return 0;
> > -
> > acpi_bus_generate_netlink_event(tz->device-
> > >pnp.device_class,
> > - dev_name(>device->dev),
> > type, 1);
> > + dev_name(>device->dev),
> > + ACPI_THERMAL_NOTIFY_HOT,
> > 1);
> > +}
> >
> > - if (trip_type == THERMAL_TRIP_CRITICAL && nocrt)
> > - return 1;
> > +static void acpi_thermal_zone_device_critical(struct
> > thermal_zone_device *thermal)
> > +{
> > + struct acpi_thermal *tz = thermal->devdata;
> >
> > - return 0;
> > + acpi_bus_generate_netlink_event(tz->device-
> > >pnp.device_class,
> > + dev_name(>device->dev),
> > + ACPI_THERMAL_NOTIFY_CRITICA
> > L, 1);
> > +
> > + thermal_zone_device_critical(thermal);
> > }
> >
> > static int acpi_thermal_cooling_device_cb(struct
> > thermal_zone_device *thermal,
> > @@ -812,7 +809,8 @@ static struct thermal_zone_device_ops
> > acpi_thermal_zone_ops = {
> > .get_trip_temp = thermal_get_trip_temp,
> > .get_crit_temp = thermal_get_crit_temp,
> > .get_trend = thermal_get_trend,
> > - .notify = thermal_notify,
> > + .hot = acpi_thermal_zone_device_hot,
> > + .critical = acpi_thermal_zone_device_critical,
> > };
> >
> > static int acpi_thermal_register_thermal_zone(struct acpi_thermal
> > *tz)
> >
>
>
[PATCH] cpufreq: intel_pstate: Use the latest guaranteed freq during verify
This change tries to address an issue, when BIOS disabled turbo
but HWP_CAP guaranteed is changed later and user space wants to take
advantage of this increased guaranteed performance.
The HWP_CAP.GUARANTEED value is not a static value. It can be changed
by some out of band agent or during Intel Speed Select performance
level change. The HWP_CAP.MAX still shows max possible performance when
BIOS disabled turbo. So guaranteed can still change as long as this is
same or below HWP_CAP.MAX.
When guaranteed is changed, the sysfs base_frequency attributes shows
the latest guaranteed frequency. This attribute can be used by user
space software to update scaling min/max frequency.
Currently the setpolicy callback already uses the latest HWP_CAP
values when setting HWP_REQ. But the verify callback will still restrict
the user settings to the to old guaranteed value. So if the guaranteed
is increased, user space can't take advantage of it.
To solve this similar to setpolicy callback, read the latest HWP_CAP
values and use it to restrict the maximum setting. This is done by
calling intel_pstate_get_hwp_max(), which already accounts for user
and BIOS turbo disable to get the current max performance.
This issue is side effect of fixing the issue of scaling frequency
limits by the
'commit eacc9c5a927e ("cpufreq: intel_pstate:
Fix intel_pstate_get_hwp_max() for turbo disabled")'
The fix resulted in correct setting of reduced scaling frequencies,
but this resulted in capping HWP.REQ to HWP_CAP.GUARANTEED in this case.
Cc: 5.8+ # 5.8+
Signed-off-by: Srinivas Pandruvada
---
drivers/cpufreq/intel_pstate.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 2a4db856222f..7081d1edb22b 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2199,6 +2199,12 @@ static void intel_pstate_clear_update_util_hook(unsigned
int cpu)
static int intel_pstate_get_max_freq(struct cpudata *cpu)
{
+ if (hwp_active) {
+ int turbo_max, max_state;
+
+ intel_pstate_get_hwp_max(cpu->cpu, _max, _state);
+ return max_state * cpu->pstate.scaling;
+ }
return global.turbo_disabled || global.no_turbo ?
cpu->pstate.max_freq : cpu->pstate.turbo_freq;
}
--
2.29.2
Re: [PATCH] thermal: int340x: Support Alder Lake
On Tue, 2020-11-17 at 11:48 -0800, Srinivas Pandruvada wrote:
> Add ACPI IDs for thermal drivers for Alder Lake support.
>
This patch was not in PULL request.
It is simple patch, adding ids. Can we send as part of second PULL?
Thanks,
Srinivas
> Signed-off-by: Srinivas Pandruvada <
> srinivas.pandruv...@linux.intel.com>
> ---
> drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 1 +
> drivers/thermal/intel/int340x_thermal/int3403_thermal.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 0966551cbaaa..823354a1a91a 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -584,6 +584,7 @@ static int int3400_thermal_remove(struct
> platform_device *pdev)
> static const struct acpi_device_id int3400_thermal_match[] = {
> {"INT3400", 0},
> {"INTC1040", 0},
> + {"INTC1041", 0},
> {}
> };
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> index ec1d58c4ceaa..c3c4c4d34542 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> @@ -284,6 +284,7 @@ static int int3403_remove(struct platform_device
> *pdev)
> static const struct acpi_device_id int3403_device_ids[] = {
> {"INT3403", 0},
> {"INTC1043", 0},
> + {"INTC1046", 0},
> {"", 0},
> };
> MODULE_DEVICE_TABLE(acpi, int3403_device_ids);
Re: [PATCH v2 3/3] cpufreq: intel_pstate: Implement the ->adjust_perf() callback
On Mon, 2020-12-14 at 21:09 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> Make intel_pstate expose the ->adjust_perf() callback when it
> operates in the passive mode with HWP enabled which causes the
> schedutil governor to use that callback instead of ->fast_switch().
>
> The minimum and target performance-level values passed by the
> governor to ->adjust_perf() are converted to HWP.REQ.MIN and
> HWP.REQ.DESIRED, respectively, which allows the processor to
> adjust its configuration to maximize energy-efficiency while
> providing sufficient capacity.
>
> Signed-off-by: Rafael J. Wysocki
Acked-by: Srinivas Pandruvada
> ---
>
> v1 -> v2:
> - No changes.
>
> ---
> drivers/cpufreq/intel_pstate.c | 70
> +
> 1 file changed, 58 insertions(+), 12 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2526,20 +2526,19 @@ static void intel_cpufreq_trace(struct c
> fp_toint(cpu->iowait_boost * 100));
> }
>
> -static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32
> target_pstate,
> - bool strict, bool fast_switch)
> +static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 min,
> u32 max,
> + u32 desired, bool fast_switch)
> {
> u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev;
>
> value &= ~HWP_MIN_PERF(~0L);
> - value |= HWP_MIN_PERF(target_pstate);
> + value |= HWP_MIN_PERF(min);
>
> - /*
> - * The entire MSR needs to be updated in order to update the
> HWP min
> - * field in it, so opportunistically update the max too if
> needed.
> - */
> value &= ~HWP_MAX_PERF(~0L);
> - value |= HWP_MAX_PERF(strict ? target_pstate : cpu-
> >max_perf_ratio);
> + value |= HWP_MAX_PERF(max);
> +
> + value &= ~HWP_DESIRED_PERF(~0L);
> + value |= HWP_DESIRED_PERF(desired);
>
> if (value == prev)
> return;
> @@ -2569,11 +2568,15 @@ static int intel_cpufreq_update_pstate(s
> int old_pstate = cpu->pstate.current_pstate;
>
> target_pstate = intel_pstate_prepare_request(cpu,
> target_pstate);
> - if (hwp_active)
> - intel_cpufreq_adjust_hwp(cpu, target_pstate,
> - policy->strict_target,
> fast_switch);
> - else if (target_pstate != old_pstate)
> + if (hwp_active) {
> + int max_pstate = policy->strict_target ?
> + target_pstate : cpu-
> >max_perf_ratio;
> +
> + intel_cpufreq_adjust_hwp(cpu, target_pstate,
> max_pstate, 0,
> + fast_switch);
> + } else if (target_pstate != old_pstate) {
> intel_cpufreq_adjust_perf_ctl(cpu, target_pstate,
> fast_switch);
> + }
>
> cpu->pstate.current_pstate = target_pstate;
>
> @@ -2634,6 +2637,47 @@ static unsigned int intel_cpufreq_fast_s
> return target_pstate * cpu->pstate.scaling;
> }
>
> +static void intel_cpufreq_adjust_perf(unsigned int cpunum,
> + unsigned long min_perf,
> + unsigned long target_perf,
> + unsigned long capacity)
> +{
> + struct cpudata *cpu = all_cpu_data[cpunum];
> + int old_pstate = cpu->pstate.current_pstate;
> + int cap_pstate, min_pstate, max_pstate, target_pstate;
> +
> + update_turbo_state();
> + cap_pstate = global.turbo_disabled ? cpu->pstate.max_pstate :
> + cpu-
> >pstate.turbo_pstate;
> +
> + /* Optimization: Avoid unnecessary divisions. */
> +
> + target_pstate = cap_pstate;
> + if (target_perf < capacity)
> + target_pstate = DIV_ROUND_UP(cap_pstate *
> target_perf, capacity);
> +
> + min_pstate = cap_pstate;
> + if (min_perf < capacity)
> + min_pstate = DIV_ROUND_UP(cap_pstate * min_perf,
> capacity);
> +
> + if (min_pstate < cpu->pstate.min_pstate)
> + min_pstate = cpu->pstate.min_pstate;
> +
> + if (min_pstate < cpu->min_perf_ratio)
> + min_pstate = cpu->min_perf_ratio;
> +
> + max_pstate = min(cap_pstate, cpu->max_perf_ratio)
Re: [PATCH -next] hid/hid-sensor-custom: convert comma to semicolon
On Mon, 2020-12-14 at 21:32 +0800, Zheng Yongjun wrote:
> Replace a comma between expression statements by a semicolon.
>
> Signed-off-by: Zheng Yongjun
Acked-by: Srinivas Pandruvada
> ---
> drivers/hid/hid-sensor-custom.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index 4d25577a8573..6c47a2e7623d 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -728,7 +728,7 @@ static int hid_sensor_custom_dev_if_add(struct
> hid_sensor_custom *sensor_inst)
>
> sensor_inst->custom_dev.minor = MISC_DYNAMIC_MINOR;
> sensor_inst->custom_dev.name = dev_name(_inst->pdev-
> >dev);
> - sensor_inst->custom_dev.fops = _sensor_custom_fops,
> + sensor_inst->custom_dev.fops = _sensor_custom_fops;
> ret = misc_register(_inst->custom_dev);
> if (ret) {
> kfifo_free(_inst->data_fifo);
[PATCH] thermal: int340x: processor_thermal: Correct workload type name
Change "Burusty" to "bursty".
Reported-by: Michael Larabel
Signed-off-by: Srinivas Pandruvada
---
drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
index 2c105fed2d7b..990f51f22884 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
@@ -87,7 +87,7 @@ static const char * const workload_types[] = {
"none",
"idle",
"semi_active",
- "burusty",
+ "bursty",
"sustained",
"battery_life",
NULL
--
2.25.4
Re: [PATCH 3/4] thermal: int340x: processor_thermal: Add RFIM driver
On Wed, 2020-12-09 at 21:48 +0100, Daniel Lezcano wrote: > On 26/11/2020 18:18, Srinivas Pandruvada wrote: > > Add support for RFIM (Radio Frequency Interference Mitigation) > > support > > via processor thermal PCI device. This drivers allows adjustment of > > FIVR (Fully Integrated Voltage Regulator) and DDR (Double Data > > Rate) > > frequencies to avoid RF interference with WiFi and 5G. > > > > Switching voltage regulators (VR) generate radiated EMI or RFI at > > the > > fundamental frequency and its harmonics. Some harmonics may > > interfere > > with very sensitive wireless receivers such as Wi-Fi and cellular > > that > > are integrated into host systems like notebook PCs. One of > > mitigation > > methods is requesting SOC integrated VR (IVR) switching frequency > > to a > > small % and shift away the switching noise harmonic interference > > from > > radio channels. OEM or ODMs can use the driver to control SOC IVR > > operation within the range where it does not impact IVR > > performance. > > > > DRAM devices of DDR IO interface and their power plane can generate > > EMI > > at the data rates. Similar to IVR control mechanism, Intel offers a > > mechanism by which DDR data rates can be changed if several > > conditions > > are met: there is strong RFI interference because of DDR; CPU power > > management has no other restriction in changing DDR data rates; > > PC ODMs enable this feature (real time DDR RFI Mitigation referred > > to as > > DDR-RFIM) for Wi-Fi from BIOS. > > Thanks for the technical details, it is interesting. > > May be I missed something but how this is related to thermal Not directly offering thermal control but uses the thermal PCI device (via MMIO) as this allows optimal RF frequencies which will result in optimal TX power. Higher TX power consumption can result in non optimal die temperature. Idea is manage power before this cause issue. Thanks, Srinivas > > > > This change exports two folders under > > /sys/bus/pci/devices/:00:04.0. > > One folder "fivr" contains all attributes exposed for controling > > FIVR > > features. The other folder "dvfs" contains all attributes for DDR > > features. > > > > Changes done to implement: > > - New module for rfim interfaces > > - Two new per processor features for DDR and FIVR > > - Enable feature for Tiger Lake (FIVR only) and Alder Lake > > > > The attributes exposed and explanation: > > > > FIVR attributes > > > > vco_ref_code_lo (RW): The VCO reference code is an 11-bit field and > > controls the FIVR switching frequency. This is the 3-bit LSB field. > > > > vco_ref_code_hi (RW): The VCO reference code is an 11-bit field and > > controls the FIVR switching frequency. This is the 8-bit MSB field. > > > > spread_spectrum_pct (RW): Set the FIVR spread spectrum clocking > > percentage > > > > spread_spectrum_clk_enable (RW): Enable/disable of the FIVR spread > > spectrum clocking feature > > > > rfi_vco_ref_code (RW): This field is a read only status register > > which > > reflects the current FIVR switching frequency > > > > fivr_fffc_rev (RW): This field indicated the revision of the FIVR > > HW. > > > > DVFS attributes > > > > rfi_restriction_run_busy (RW): Request the restriction of specific > > DDR > > data rate and set this value 1. Self reset to 0 after operation. > > > > rfi_restriction_err_code (RW): Values: 0 :Request is accepted, > > 1:Feature > > disabled, 2: the request restricts more points than it is allowed > > > > rfi_restriction_data_rate_Delta (RW): Restricted DDR data rate for > > RFI > > protection: Lower Limit > > > > rfi_restriction_data_rate_Base (RW): Restricted DDR data rate for > > RFI > > protection: Upper Limit > > > > ddr_data_rate_point_0 (RO): DDR data rate selection 1st point > > > > ddr_data_rate_point_1 (RO): DDR data rate selection 2nd point > > > > ddr_data_rate_point_2 (RO): DDR data rate selection 3rd point > > > > ddr_data_rate_point_3 (RO): DDR data rate selection 4th point > > > > rfi_disable (RW): Disable DDR rate change feature > > > > Signed-off-by: Srinivas Pandruvada < > > srinivas.pandruv...@linux.intel.com> > > --- > > .../thermal/intel/int340x_thermal/Makefile| 1 + > > .../processor_thermal_device.c| 23 +- > > .../processor_thermal_device.h| 5
Re: [PATCH 1/4] thermal: int340x: processor_thermal: Refactor MMIO interface
On Thu, 2020-11-26 at 09:18 -0800, Srinivas Pandruvada wrote: > The Processor Thermal PCI device supports multiple features. > Currently > we export only RAPL. But we need more features from this device > exposed > for Tiger Lake and Alder Lake based platforms. So re-structure the > current MMIO interface, so that more features can be added cleanly. > > No functional changes are expected with this change. > Any objection for this series for 5.11? Thanks, Srinivas > Changes done in this patch: > - Using PCI_DEVICE_DATA(), hence names of defines changed > - Move RAPL MMIO code to its own module > - Move the RAPL MMIO offsets to RAPL MMIO module > - Adjust Kconfig dependency of PROC_THERMAL_MMIO_RAPL > - Per processor driver data now contains the supported features > - Moved all the common data structures and defines to a common header > file > - This new header file contains all the processor_thermal_* > interfaces > - Based on the features supported the module interface is called > - Each module atleast provides one add and one remove function > > Signed-off-by: Srinivas Pandruvada < > srinivas.pandruv...@linux.intel.com> > --- > drivers/thermal/intel/int340x_thermal/Kconfig | 6 +- > .../thermal/intel/int340x_thermal/Makefile| 1 + > .../processor_thermal_device.c| 261 -- > > .../processor_thermal_device.h| 72 + > .../int340x_thermal/processor_thermal_rapl.c | 134 + > 5 files changed, 263 insertions(+), 211 deletions(-) > create mode 100644 > drivers/thermal/intel/int340x_thermal/processor_thermal_device.h > create mode 100644 > drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c > > diff --git a/drivers/thermal/intel/int340x_thermal/Kconfig > b/drivers/thermal/intel/int340x_thermal/Kconfig > index 797907542e43..45c31f3d6054 100644 > --- a/drivers/thermal/intel/int340x_thermal/Kconfig > +++ b/drivers/thermal/intel/int340x_thermal/Kconfig > @@ -10,6 +10,7 @@ config INT340X_THERMAL > select ACPI_THERMAL_REL > select ACPI_FAN > select INTEL_SOC_DTS_IOSF_CORE > + select PROC_THERMAL_MMIO_RAPL if X86_64 && POWERCAP > help > Newer laptops and tablets that use ACPI may have thermal > sensors and > other devices with thermal control capabilities outside the > core > @@ -41,9 +42,6 @@ config INT3406_THERMAL > power consumed by display device. > > config PROC_THERMAL_MMIO_RAPL > - bool > - depends on 64BIT > - depends on POWERCAP > + tristate > select INTEL_RAPL_CORE > - default y > endif > diff --git a/drivers/thermal/intel/int340x_thermal/Makefile > b/drivers/thermal/intel/int340x_thermal/Makefile > index 287eb0a1476d..86e8d3c87df7 100644 > --- a/drivers/thermal/intel/int340x_thermal/Makefile > +++ b/drivers/thermal/intel/int340x_thermal/Makefile > @@ -4,5 +4,6 @@ obj-$(CONFIG_INT340X_THERMAL) += > int340x_thermal_zone.o > obj-$(CONFIG_INT340X_THERMAL)+= int3402_thermal.o > obj-$(CONFIG_INT340X_THERMAL)+= int3403_thermal.o > obj-$(CONFIG_INT340X_THERMAL)+= processor_thermal_device.o > +obj-$(CONFIG_PROC_THERMAL_MMIO_RAPL) += processor_thermal_rapl.o > obj-$(CONFIG_INT3406_THERMAL)+= int3406_thermal.o > obj-$(CONFIG_ACPI_THERMAL_REL) += acpi_thermal_rel.o > diff --git > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c > index 81e8b15ef405..5b8dc5e9ec86 100644 > --- > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c > +++ > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c > @@ -12,74 +12,18 @@ > #include > #include > #include > -#include > #include "int340x_thermal_zone.h" > +#include "processor_thermal_device.h" > #include "../intel_soc_dts_iosf.h" > > -/* Broadwell-U/HSB thermal reporting device */ > -#define PCI_DEVICE_ID_PROC_BDW_THERMAL 0x1603 > -#define PCI_DEVICE_ID_PROC_HSB_THERMAL 0x0A03 > - > -/* Skylake thermal reporting device */ > -#define PCI_DEVICE_ID_PROC_SKL_THERMAL 0x1903 > - > -/* CannonLake thermal reporting device */ > -#define PCI_DEVICE_ID_PROC_CNL_THERMAL 0x5a03 > -#define PCI_DEVICE_ID_PROC_CFL_THERMAL 0x3E83 > - > -/* Braswell thermal reporting device */ > -#define PCI_DEVICE_ID_PROC_BSW_THERMAL 0x22DC > - > -/* Broxton thermal reporting device */ > -#define PCI_DEVICE_ID_PROC_BXT0_THERMAL 0x0A8C > -#define PCI_DEVICE_ID_PROC_BXT1_THERMAL 0x1A8C > -#define PCI_DEVICE_ID_PROC_BXTX_THERMAL 0x4A8C > -#define PCI_DEVICE_ID
Re: [PATCH 1/3] thermal: core: Add indication for userspace usage
On Wed, 2020-12-09 at 10:30 +0100, Daniel Lezcano wrote:
> On 07/12/2020 06:36, Kai-Heng Feng wrote:
> >
> > > On Dec 1, 2020, at 02:39, Srinivas Pandruvada <
> > > srinivas.pandruv...@linux.intel.com> wrote:
> > >
> > > On Tue, 2020-12-01 at 02:22 +0800, Kai-Heng Feng wrote:
> > > > > On Dec 1, 2020, at 02:13, Srinivas Pandruvada <
> > > > > srinivas.pandruv...@linux.intel.com> wrote:
> > > >
> > > > [snipped]
> > > >
> > > > > > > What about creating an new callback
> > > > > > >
> > > > > > > enum thermal_trip_status {
> > > > > > > THERMAL_TRIP_DISABLED = 0,
> > > > > > > THERMAL_TRIP_ENABLED,
> > > > > > > };
> > > > > > >
> > > > > > > int get_trip_status(struct thermal_zone_device *, int
> > > > > > > trip,
> > > > > > > enum
> > > > > > > thermal_trip_status *state);
> > > > > > >
> > > > > > > Then in
> > > > > > > static void handle_thermal_trip(struct
> > > > > > > thermal_zone_device *tz,
> > > > > > > int
> > > > > > > trip)
> > > > > > > {
> > > > > > >
> > > > > > > /* before tz->ops->get_trip_temp(tz, trip, _temp);
> > > > > > > */
> > > > > > > if (tz->ops->get_trip_status) {
> > > > > > > enum thermal_trip_status *status;
> > > > > > >
> > > > > > > if (!tz->ops->get_trip_status(tz, trip, )) {
> > > > > > > if (status == THERMAL_TRIP_DISABLED)
> > > > > > > return;
> > > > > > > }
> > > > > > > }
> > > > > > > ...
> > > > > > > ...
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > > This callback will help the cases:
> > > > > > > - Allows drivers to selectively disable certain trips
> > > > > > > during
> > > > > > > init
> > > > > > > state
> > > > > > > or system resume where there can be spikes or always.
> > > > > > > int340x
> > > > > > > drivers
> > > > > > > can disable always.
> > > > > >
> > > > > > This sounds really great. This is indeed can happen on
> > > > > > system
> > > > > > resume,
> > > > > > before userspace process thaw.
> > > > > >
> > > > > > > - Still give options for drivers to handle critical trip
> > > > > > > even
> > > > > > > if
> > > > > > > they
> > > > > > > are bound to user space governors. User space process may
> > > > > > > be
> > > > > > > dead,
> > > > > > > so
> > > > > > > still allow kernel to process graceful shutdown
> > > > > >
> > > > > > To make the scenario happen, do we need a new sysfs to let
> > > > > > usespace
> > > > > > enable it with THERMAL_TRIP_ENABLED?
> > > > > This should be drivers call not user space.
> > > >
> > > > Understood. So after thermal_zone_device_register(), the driver
> > > > can
> > > > decide to what to return on get_trip_temp().
> > > get_trip_status()
> > >
> > > > Let me work on a new patch if there's no other concern.
> > > Better to wait for confirmation from Daniel and others.
> >
> > Daniel,
> >
> > Do you like Srinivas' proposed solution?
> >
> > I hope we can find a solution in upstream kernel soon.
>
> (just trying to figure out the full context)
>
> If the device is enumerated outside of a thermal zone, the sensor
> should
> not register in the thermal zone no ?
Other trips are fine, so sensor registry is still required for passive
and active control. We just need to ignore critical trip. These table
are tested by OEM on Windows, where critical trip doesn't result in
immediate shutdown.
Thanks,
Srinivas
>
>
>
[PATCH v2 3/3] platform/x86: ISST: Change PCI device macros
Use PCI_VDEVICE and PCI_DEVICE_DATA macros. No functional changes are
expected.
Signed-off-by: Srinivas Pandruvada
---
.../platform/x86/intel_speed_select_if/isst_if_common.h | 8
.../platform/x86/intel_speed_select_if/isst_if_mbox_pci.c | 4 ++--
drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c | 6 ++
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
b/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
index 4f6f7f0761fc..fdecdae248d7 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
@@ -10,11 +10,11 @@
#ifndef __ISST_IF_COMMON_H
#define __ISST_IF_COMMON_H
-#define INTEL_RAPL_PRIO_DEVID_00x3451
-#define INTEL_CFG_MBOX_DEVID_0 0x3459
+#define PCI_DEVICE_ID_INTEL_RAPL_PRIO_DEVID_0 0x3451
+#define PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_0 0x3459
-#define INTEL_RAPL_PRIO_DEVID_1 0x3251
-#define INTEL_CFG_MBOX_DEVID_1 0x3259
+#define PCI_DEVICE_ID_INTEL_RAPL_PRIO_DEVID_1 0x3251
+#define PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_1 0x3259
/*
* Validate maximum commands in a single request.
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
index 95f01e7a87d5..a2a2d923e60c 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
@@ -146,8 +146,8 @@ static long isst_if_mbox_proc_cmd(u8 *cmd_ptr, int
*write_only, int resume)
}
static const struct pci_device_id isst_if_mbox_ids[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_CFG_MBOX_DEVID_0)},
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_CFG_MBOX_DEVID_1)},
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_0)},
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_1)},
{ 0 },
};
MODULE_DEVICE_TABLE(pci, isst_if_mbox_ids);
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
index 4c9020249989..ff49025ec085 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
@@ -82,10 +82,8 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
}
static const struct pci_device_id isst_if_ids[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_0),
- .driver_data = (kernel_ulong_t)_range_devid_0},
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_1),
- .driver_data = (kernel_ulong_t)_range_devid_1},
+ { PCI_DEVICE_DATA(INTEL, RAPL_PRIO_DEVID_0, _range_devid_0)},
+ { PCI_DEVICE_DATA(INTEL, RAPL_PRIO_DEVID_1, _range_devid_1)},
{ 0 },
};
MODULE_DEVICE_TABLE(pci, isst_if_ids);
--
2.25.4
[PATCH v2 2/3] platform/x86: ISST: Allow configurable offset range
The mmio offset range can be different based on the PCI device id. Here
for INTEL_RAPL_PRIO_DEVID_1, the range is increased from 45 to 64. Pass
the range as the driver_data. Also account for different ranges during
save/restore via suspend/resume callbacks.
Reported-by: kernel test robot
Signed-off-by: Srinivas Pandruvada
---
LKP test robot reported sparse warning. Added "Reported-by" as
suggested by the message.
.../x86/intel_speed_select_if/isst_if_mmio.c | 50 +--
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
index e7e9808a1aed..4c9020249989 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
@@ -20,15 +20,21 @@ struct isst_mmio_range {
int end;
};
-struct isst_mmio_range mmio_range[] = {
+static struct isst_mmio_range mmio_range_devid_0[] = {
{0x04, 0x14},
{0x20, 0xD0},
};
+static struct isst_mmio_range mmio_range_devid_1[] = {
+ {0x04, 0x14},
+ {0x20, 0x11C},
+};
+
struct isst_if_device {
void __iomem *punit_mmio;
u32 range_0[5];
- u32 range_1[45];
+ u32 range_1[64];
+ struct isst_mmio_range *mmio_range;
struct mutex mutex;
};
@@ -39,8 +45,6 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
struct pci_dev *pdev;
io_reg = (struct isst_if_io_reg *)cmd_ptr;
- if (io_reg->reg < 0x04 || io_reg->reg > 0xD0)
- return -EINVAL;
if (io_reg->reg % 4)
return -EINVAL;
@@ -56,6 +60,10 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
if (!punit_dev)
return -EINVAL;
+ if (io_reg->reg < punit_dev->mmio_range[0].beg ||
+ io_reg->reg > punit_dev->mmio_range[1].end)
+ return -EINVAL;
+
/*
* Ensure that operation is complete on a PCI device to avoid read
* write race by using per PCI device mutex.
@@ -74,8 +82,10 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
}
static const struct pci_device_id isst_if_ids[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_0)},
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_1)},
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_0),
+ .driver_data = (kernel_ulong_t)_range_devid_0},
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_1),
+ .driver_data = (kernel_ulong_t)_range_devid_1},
{ 0 },
};
MODULE_DEVICE_TABLE(pci, isst_if_ids);
@@ -112,6 +122,7 @@ static int isst_if_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
mutex_init(_dev->mutex);
pci_set_drvdata(pdev, punit_dev);
+ punit_dev->mmio_range = (struct isst_mmio_range *) ent->driver_data;
memset(, 0, sizeof(cb));
cb.cmd_size = sizeof(struct isst_if_io_reg);
@@ -141,10 +152,15 @@ static int __maybe_unused isst_if_suspend(struct device
*device)
for (i = 0; i < ARRAY_SIZE(punit_dev->range_0); ++i)
punit_dev->range_0[i] = readl(punit_dev->punit_mmio +
- mmio_range[0].beg + 4 * i);
- for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i)
- punit_dev->range_1[i] = readl(punit_dev->punit_mmio +
- mmio_range[1].beg + 4 * i);
+ punit_dev->mmio_range[0].beg +
4 * i);
+ for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i) {
+ u32 addr;
+
+ addr = punit_dev->mmio_range[1].beg + 4 * i;
+ if (addr > punit_dev->mmio_range[1].end)
+ break;
+ punit_dev->range_1[i] = readl(punit_dev->punit_mmio + addr);
+ }
return 0;
}
@@ -156,10 +172,16 @@ static int __maybe_unused isst_if_resume(struct device
*device)
for (i = 0; i < ARRAY_SIZE(punit_dev->range_0); ++i)
writel(punit_dev->range_0[i], punit_dev->punit_mmio +
- mmio_range[0].beg + 4 * i);
- for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i)
- writel(punit_dev->range_1[i], punit_dev->punit_mmio +
- mmio_range[1].beg + 4 * i);
+ punit_dev->mmio_range[0].beg +
4 * i);
+ for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i) {
+ u32 addr;
+
+ addr = punit_dev->mmio_range[1].beg + 4 * i;
+ if (addr > punit_dev->mmio_range[1].end)
+ break;
+
+ writel(punit_dev->range_1[i], punit_dev->punit_mmio + addr);
+ }
return 0;
}
--
2.25.4
[PATCH v2 1/3] platform/x86: ISST: Check for unaligned mmio address
The address should be aligned to 4 byte boundary. So send an error for unaligned address. Signed-off-by: Srinivas Pandruvada --- drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c index aa17fd7817f8..e7e9808a1aed 100644 --- a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c +++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c @@ -42,6 +42,9 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only, int resume) if (io_reg->reg < 0x04 || io_reg->reg > 0xD0) return -EINVAL; + if (io_reg->reg % 4) + return -EINVAL; + if (io_reg->read_write && !capable(CAP_SYS_ADMIN)) return -EPERM; -- 2.25.4
[PATCH v2 0/3] Intel Speed Select interface changes for 5.11
v2 Fix sparse warnings reported by LKP. Srinivas Pandruvada (3): platform/x86: ISST: Check for unaligned mmio address platform/x86: ISST: Allow configurable offset range platform/x86: ISST: Change PCI device macros .../intel_speed_select_if/isst_if_common.h| 8 +-- .../intel_speed_select_if/isst_if_mbox_pci.c | 4 +- .../x86/intel_speed_select_if/isst_if_mmio.c | 49 ++- 3 files changed, 42 insertions(+), 19 deletions(-) -- 2.25.4
Re: add custom hinge sensor support
On Thu, 2020-12-03 at 11:53 +0800, Ye Xiang wrote: > Here three separate iio devices are presented which presents angle > for > hinge, keyboard and screen. You are presenting one IIO device now with three channels. > > This driver works on devices with Intel integrated sensor hub, where > hinge sensor is presented using a custom sensor usage id. > > Here the angle is presented in degrees, which is converted to > radians. > > Changes since v2: > - use 1 iio device instead of 3 for hinge sensor. > - use indexed channel instead of modified channel and added channel > labels. > - remove 2,3 patch in last version, add a document patch to > describe the > hinge channels. > - hid-sensor-custom: use meaningful return value in > get_known_custom_sensor_index and checked in call side. > - hid-sensor-ids.h: use HID_USAGE_SENSOR_DATA_FIELD_CUSTOM_VALUE(x) > to > define custom sensor value. > > Changes since v1: > - fixed errors reported by lkp > > Ye Xiang (3): > HID: hid-sensor-custom: Add custom sensor iio support > iio: hid-sensors: Add hinge sensor driver > iio:Documentation: Add documentation for hinge sensor channels > > Documentation/ABI/testing/sysfs-bus-iio | 17 + > drivers/hid/hid-sensor-custom.c | 181 > .../hid-sensors/hid-sensor-attributes.c | 2 + > drivers/iio/position/Kconfig | 16 + > drivers/iio/position/Makefile | 1 + > .../position/hid-sensor-custom-intel-hinge.c | 393 > ++ > include/linux/hid-sensor-ids.h| 14 + > 7 files changed, 624 insertions(+) > create mode 100644 drivers/iio/position/hid-sensor-custom-intel- > hinge.c >
Re: [PATCH v3 3/3] iio:Documentation: Add documentation for hinge sensor channels
On Thu, 2020-12-03 at 11:53 +0800, Ye Xiang wrote: > Add channel description for hinge sensor, including channel label > attribute and raw data description. > > Signed-off-by: Ye Xiang > --- > Documentation/ABI/testing/sysfs-bus-iio | 17 + > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio > b/Documentation/ABI/testing/sysfs-bus-iio > index df42bed09f25..82303b1bdff0 100644 > --- a/Documentation/ABI/testing/sysfs-bus-iio > +++ b/Documentation/ABI/testing/sysfs-bus-iio > @@ -1802,3 +1802,20 @@ Contact: linux-...@vger.kernel.org > Description: > Unscaled light intensity according to CIE 1931/DIN 5033 > color space. > Units after application of scale are nano nanowatts per > square meter. > + > +What:/sys/bus/iio/devices/iio:deviceX/in_anglY_raw Channel "Y" doesn't sound to be a variable. Why not just say "*" Thanks, Srinivas > +KernelVersion: 5.12 > +Contact: linux-...@vger.kernel.org > +Description: > + Angle of rotation for channel Y. Units after > application of scale > + and offset are radians. > + > +What:/sys/bus/iio/devices/iio:deviceX/in_anglY_label > +KernelVersion: 5.12 > +Contact: linux-...@vger.kernel.org > +Description: > + Optional symbolic label for channel Y. > + For Intel hid hinge sensor, the label values are: > + hinge, keyboard, screen. It means the three channels > + each correspond respectively to hinge angle, keyboard > angle, > + and screen angle.
[PATCH 3/3] platform/x86: ISST: Change PCI device macros
Use PCI_VDEVICE and PCI_DEVICE_DATA macros. No functional changes are
expected.
Signed-off-by: Srinivas Pandruvada
---
.../platform/x86/intel_speed_select_if/isst_if_common.h | 8
.../platform/x86/intel_speed_select_if/isst_if_mbox_pci.c | 4 ++--
drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c | 6 ++
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
b/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
index 4f6f7f0761fc..fdecdae248d7 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_common.h
@@ -10,11 +10,11 @@
#ifndef __ISST_IF_COMMON_H
#define __ISST_IF_COMMON_H
-#define INTEL_RAPL_PRIO_DEVID_00x3451
-#define INTEL_CFG_MBOX_DEVID_0 0x3459
+#define PCI_DEVICE_ID_INTEL_RAPL_PRIO_DEVID_0 0x3451
+#define PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_0 0x3459
-#define INTEL_RAPL_PRIO_DEVID_1 0x3251
-#define INTEL_CFG_MBOX_DEVID_1 0x3259
+#define PCI_DEVICE_ID_INTEL_RAPL_PRIO_DEVID_1 0x3251
+#define PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_1 0x3259
/*
* Validate maximum commands in a single request.
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
index 95f01e7a87d5..a2a2d923e60c 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mbox_pci.c
@@ -146,8 +146,8 @@ static long isst_if_mbox_proc_cmd(u8 *cmd_ptr, int
*write_only, int resume)
}
static const struct pci_device_id isst_if_mbox_ids[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_CFG_MBOX_DEVID_0)},
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_CFG_MBOX_DEVID_1)},
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_0)},
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CFG_MBOX_DEVID_1)},
{ 0 },
};
MODULE_DEVICE_TABLE(pci, isst_if_mbox_ids);
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
index c4bf8dea32ca..2906cfee5d9c 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
@@ -82,10 +82,8 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
}
static const struct pci_device_id isst_if_ids[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_0),
- .driver_data = (kernel_ulong_t)_range_devid_0},
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_1),
- .driver_data = (kernel_ulong_t)_range_devid_1},
+ { PCI_DEVICE_DATA(INTEL, RAPL_PRIO_DEVID_0, _range_devid_0)},
+ { PCI_DEVICE_DATA(INTEL, RAPL_PRIO_DEVID_1, _range_devid_1)},
{ 0 },
};
MODULE_DEVICE_TABLE(pci, isst_if_ids);
--
2.25.4
[PATCH 2/3] platform/x86: ISST: Allow configurable offset range
The mmio offset range can be different based on the PCI device id. Here
for INTEL_RAPL_PRIO_DEVID_1, the range is increased from 45 to 64. Pass
the range as the driver_data. Also account for different ranges during
save/restore via suspend/resume callbacks.
Signed-off-by: Srinivas Pandruvada
---
.../x86/intel_speed_select_if/isst_if_mmio.c | 50 +--
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
index e7e9808a1aed..c4bf8dea32ca 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c
@@ -20,15 +20,21 @@ struct isst_mmio_range {
int end;
};
-struct isst_mmio_range mmio_range[] = {
+struct isst_mmio_range mmio_range_devid_0[] = {
{0x04, 0x14},
{0x20, 0xD0},
};
+struct isst_mmio_range mmio_range_devid_1[] = {
+ {0x04, 0x14},
+ {0x20, 0x11C},
+};
+
struct isst_if_device {
void __iomem *punit_mmio;
u32 range_0[5];
- u32 range_1[45];
+ u32 range_1[64];
+ struct isst_mmio_range *mmio_range;
struct mutex mutex;
};
@@ -39,8 +45,6 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
struct pci_dev *pdev;
io_reg = (struct isst_if_io_reg *)cmd_ptr;
- if (io_reg->reg < 0x04 || io_reg->reg > 0xD0)
- return -EINVAL;
if (io_reg->reg % 4)
return -EINVAL;
@@ -56,6 +60,10 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
if (!punit_dev)
return -EINVAL;
+ if (io_reg->reg < punit_dev->mmio_range[0].beg ||
+ io_reg->reg > punit_dev->mmio_range[1].end)
+ return -EINVAL;
+
/*
* Ensure that operation is complete on a PCI device to avoid read
* write race by using per PCI device mutex.
@@ -74,8 +82,10 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only,
int resume)
}
static const struct pci_device_id isst_if_ids[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_0)},
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_1)},
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_0),
+ .driver_data = (kernel_ulong_t)_range_devid_0},
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, INTEL_RAPL_PRIO_DEVID_1),
+ .driver_data = (kernel_ulong_t)_range_devid_1},
{ 0 },
};
MODULE_DEVICE_TABLE(pci, isst_if_ids);
@@ -112,6 +122,7 @@ static int isst_if_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
mutex_init(_dev->mutex);
pci_set_drvdata(pdev, punit_dev);
+ punit_dev->mmio_range = (struct isst_mmio_range *) ent->driver_data;
memset(, 0, sizeof(cb));
cb.cmd_size = sizeof(struct isst_if_io_reg);
@@ -141,10 +152,15 @@ static int __maybe_unused isst_if_suspend(struct device
*device)
for (i = 0; i < ARRAY_SIZE(punit_dev->range_0); ++i)
punit_dev->range_0[i] = readl(punit_dev->punit_mmio +
- mmio_range[0].beg + 4 * i);
- for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i)
- punit_dev->range_1[i] = readl(punit_dev->punit_mmio +
- mmio_range[1].beg + 4 * i);
+ punit_dev->mmio_range[0].beg +
4 * i);
+ for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i) {
+ u32 addr;
+
+ addr = punit_dev->mmio_range[1].beg + 4 * i;
+ if (addr > punit_dev->mmio_range[1].end)
+ break;
+ punit_dev->range_1[i] = readl(punit_dev->punit_mmio + addr);
+ }
return 0;
}
@@ -156,10 +172,16 @@ static int __maybe_unused isst_if_resume(struct device
*device)
for (i = 0; i < ARRAY_SIZE(punit_dev->range_0); ++i)
writel(punit_dev->range_0[i], punit_dev->punit_mmio +
- mmio_range[0].beg + 4 * i);
- for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i)
- writel(punit_dev->range_1[i], punit_dev->punit_mmio +
- mmio_range[1].beg + 4 * i);
+ punit_dev->mmio_range[0].beg +
4 * i);
+ for (i = 0; i < ARRAY_SIZE(punit_dev->range_1); ++i) {
+ u32 addr;
+
+ addr = punit_dev->mmio_range[1].beg + 4 * i;
+ if (addr > punit_dev->mmio_range[1].end)
+ break;
+
+ writel(punit_dev->range_1[i], punit_dev->punit_mmio + addr);
+ }
return 0;
}
--
2.25.4
[PATCH 1/3] platform/x86: ISST: Check for unaligned mmio address
The address should be aligned to 4 byte boundary. So send an error for unaligned address. Signed-off-by: Srinivas Pandruvada --- drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c index aa17fd7817f8..e7e9808a1aed 100644 --- a/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c +++ b/drivers/platform/x86/intel_speed_select_if/isst_if_mmio.c @@ -42,6 +42,9 @@ static long isst_if_mmio_rd_wr(u8 *cmd_ptr, int *write_only, int resume) if (io_reg->reg < 0x04 || io_reg->reg > 0xD0) return -EINVAL; + if (io_reg->reg % 4) + return -EINVAL; + if (io_reg->read_write && !capable(CAP_SYS_ADMIN)) return -EPERM; -- 2.25.4
Re: [PATCH 1/3] thermal: core: Add indication for userspace usage
On Tue, 2020-12-01 at 02:22 +0800, Kai-Heng Feng wrote:
> > On Dec 1, 2020, at 02:13, Srinivas Pandruvada <
> > srinivas.pandruv...@linux.intel.com> wrote:
>
> [snipped]
>
> > > > What about creating an new callback
> > > >
> > > > enum thermal_trip_status {
> > > > THERMAL_TRIP_DISABLED = 0,
> > > > THERMAL_TRIP_ENABLED,
> > > > };
> > > >
> > > > int get_trip_status(struct thermal_zone_device *, int trip,
> > > > enum
> > > > thermal_trip_status *state);
> > > >
> > > > Then in
> > > > static void handle_thermal_trip(struct thermal_zone_device *tz,
> > > > int
> > > > trip)
> > > > {
> > > >
> > > > /* before tz->ops->get_trip_temp(tz, trip, _temp); */
> > > > if (tz->ops->get_trip_status) {
> > > > enum thermal_trip_status *status;
> > > >
> > > > if (!tz->ops->get_trip_status(tz, trip, )) {
> > > > if (status == THERMAL_TRIP_DISABLED)
> > > > return;
> > > > }
> > > > }
> > > > ...
> > > > ...
> > > >
> > > > }
> > > >
> > > >
> > > > This callback will help the cases:
> > > > - Allows drivers to selectively disable certain trips during
> > > > init
> > > > state
> > > > or system resume where there can be spikes or always. int340x
> > > > drivers
> > > > can disable always.
> > >
> > > This sounds really great. This is indeed can happen on system
> > > resume,
> > > before userspace process thaw.
> > >
> > > > - Still give options for drivers to handle critical trip even
> > > > if
> > > > they
> > > > are bound to user space governors. User space process may be
> > > > dead,
> > > > so
> > > > still allow kernel to process graceful shutdown
> > >
> > > To make the scenario happen, do we need a new sysfs to let
> > > usespace
> > > enable it with THERMAL_TRIP_ENABLED?
> > This should be drivers call not user space.
>
> Understood. So after thermal_zone_device_register(), the driver can
> decide to what to return on get_trip_temp().
get_trip_status()
> Let me work on a new patch if there's no other concern.
Better to wait for confirmation from Daniel and others.
Thanks,
Srinivas
>
> Kai-Heng
>
> > Thanks,
> > Srinivas
Re: [PATCH 1/3] thermal: core: Add indication for userspace usage
On Tue, 2020-12-01 at 02:04 +0800, Kai-Heng Feng wrote:
> > On Dec 1, 2020, at 00:19, Srinivas Pandruvada <
> > srinivas.pandruv...@linux.intel.com> wrote:
> >
> > On Mon, 2020-11-30 at 16:23 +0800, Kai-Heng Feng wrote:
> > > > On Nov 30, 2020, at 15:57, Daniel Lezcano <
> > > > daniel.lezc...@linaro.org> wrote:
> > > >
> > > >
> > > > [Added Srinivas]
> > > >
> > > > On 28/11/2020 18:54, Kai-Heng Feng wrote:
> > > > > We are seeing thermal shutdown on Intel based mobile
> > > > > workstations, the
> > > > > shutdown happens during the first trip handle in
> > > > > thermal_zone_device_register():
> > > > > kernel: thermal thermal_zone15: critical temperature reached
> > > > > (101
> > > > > C), shutting down
> > > > >
> > > > > However, we shouldn't do a thermal shutdown here, since
> > > > > 1) We may want to use a dedicated daemon, Intel's thermald in
> > > > > this case,
> > > > > to handle thermal shutdown.
> > > > >
> > > > > 2) For ACPI based system, _CRT doesn't mean shutdown unless
> > > > > it's
> > > > > inside
> > > > > ThermalZone. ACPI Spec, 11.4.4 _CRT (Critical Temperature):
> > > > > "... If this object it present under a device, the device’s
> > > > > driver
> > > > > evaluates this object to determine the device’s critical
> > > > > cooling
> > > > > temperature trip point. This value may then be used by the
> > > > > device’s
> > > > > driver to program an internal device temperature sensor trip
> > > > > point."
> > > > >
> > > > > So a "critical trip" here merely means we should take a more
> > > > > aggressive
> > > > > cooling method.
> > > >
> > > > Well, actually it is stated before:
> > > >
> > > > "This object, when defined under a thermal zone, returns the
> > > > critical
> > > > temperature at which OSPM must shutdown the system".
> > >
> > > This means specifically for the ACPI ThermalZone in AML, e.g.:
> > >
> > > ThermalZone (TZ0) {
> > >
> > >Method(_CRT) { ... }
> > > } // end of TZ0
> > >
> > > However the device is not under any ACPI ThermalZone.
> > >
> > > > That is what does the thermal subsystem, no ?
> > > >
> > > > > So add an indication to let thermal core know it should leave
> > > > > thermal
> > > > > device to userspace to handle.
> > > >
> > > > You may want to check the 'HOT' trip point and then use the
> > > > notification
> > > > mechanism to get notified in userspace and take action from
> > > > there
> > > > (eg.
> > > > offline some CPUs).
> > >
> > > For this particular issue we are facing, the thermal shutdown
> > > happens
> > > in thermal_zone_device_register() and userspace isn't up yet.
> >
> > What about creating an new callback
> >
> > enum thermal_trip_status {
> > THERMAL_TRIP_DISABLED = 0,
> > THERMAL_TRIP_ENABLED,
> > };
> >
> > int get_trip_status(struct thermal_zone_device *, int trip, enum
> > thermal_trip_status *state);
> >
> > Then in
> > static void handle_thermal_trip(struct thermal_zone_device *tz, int
> > trip)
> > {
> >
> > /* before tz->ops->get_trip_temp(tz, trip, _temp); */
> > if (tz->ops->get_trip_status) {
> > enum thermal_trip_status *status;
> >
> > if (!tz->ops->get_trip_status(tz, trip, )) {
> > if (status == THERMAL_TRIP_DISABLED)
> > return;
> > }
> > }
> > ...
> > ...
> >
> > }
> >
> >
> > This callback will help the cases:
> > - Allows drivers to selectively disable certain trips during init
> > state
> > or system resume where there can be spikes or always. int340x
> > drivers
> > can disable always.
>
> This sounds really great. This is indeed can happen on system resume,
> before userspace process thaw.
>
> > - Still give options for drivers to handle critical trip even if
> > they
> > are bound to user space governors. User space
Re: [PATCH 1/3] thermal: core: Add indication for userspace usage
On Mon, 2020-11-30 at 16:23 +0800, Kai-Heng Feng wrote:
> > On Nov 30, 2020, at 15:57, Daniel Lezcano <
> > daniel.lezc...@linaro.org> wrote:
> >
> >
> > [Added Srinivas]
> >
> > On 28/11/2020 18:54, Kai-Heng Feng wrote:
> > > We are seeing thermal shutdown on Intel based mobile
> > > workstations, the
> > > shutdown happens during the first trip handle in
> > > thermal_zone_device_register():
> > > kernel: thermal thermal_zone15: critical temperature reached (101
> > > C), shutting down
> > >
> > > However, we shouldn't do a thermal shutdown here, since
> > > 1) We may want to use a dedicated daemon, Intel's thermald in
> > > this case,
> > > to handle thermal shutdown.
> > >
> > > 2) For ACPI based system, _CRT doesn't mean shutdown unless it's
> > > inside
> > > ThermalZone. ACPI Spec, 11.4.4 _CRT (Critical Temperature):
> > > "... If this object it present under a device, the device’s
> > > driver
> > > evaluates this object to determine the device’s critical cooling
> > > temperature trip point. This value may then be used by the
> > > device’s
> > > driver to program an internal device temperature sensor trip
> > > point."
> > >
> > > So a "critical trip" here merely means we should take a more
> > > aggressive
> > > cooling method.
> >
> > Well, actually it is stated before:
> >
> > "This object, when defined under a thermal zone, returns the
> > critical
> > temperature at which OSPM must shutdown the system".
>
> This means specifically for the ACPI ThermalZone in AML, e.g.:
>
> ThermalZone (TZ0) {
>
> Method(_CRT) { ... }
> } // end of TZ0
>
> However the device is not under any ACPI ThermalZone.
>
> > That is what does the thermal subsystem, no ?
> >
> > > So add an indication to let thermal core know it should leave
> > > thermal
> > > device to userspace to handle.
> >
> > You may want to check the 'HOT' trip point and then use the
> > notification
> > mechanism to get notified in userspace and take action from there
> > (eg.
> > offline some CPUs).
>
> For this particular issue we are facing, the thermal shutdown happens
> in thermal_zone_device_register() and userspace isn't up yet.
What about creating an new callback
enum thermal_trip_status {
THERMAL_TRIP_DISABLED = 0,
THERMAL_TRIP_ENABLED,
};
int get_trip_status(struct thermal_zone_device *, int trip, enum
thermal_trip_status *state);
Then in
static void handle_thermal_trip(struct thermal_zone_device *tz, int
trip)
{
/* before tz->ops->get_trip_temp(tz, trip, _temp); */
if (tz->ops->get_trip_status) {
enum thermal_trip_status *status;
if (!tz->ops->get_trip_status(tz, trip, )) {
if (status == THERMAL_TRIP_DISABLED)
return;
}
}
...
...
}
This callback will help the cases:
- Allows drivers to selectively disable certain trips during init state
or system resume where there can be spikes or always. int340x drivers
can disable always.
- Still give options for drivers to handle critical trip even if they
are bound to user space governors. User space process may be dead, so
still allow kernel to process graceful shutdown
Thanks,
Srinivas
>
> Kai-Heng
>
> > > Signed-off-by: Kai-Heng Feng
> > > ---
> > > drivers/thermal/thermal_core.c | 3 +++
> > > include/linux/thermal.h| 2 ++
> > > 2 files changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c
> > > index c6d74bc1c90b..6561e3767529 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -1477,6 +1477,9 @@ thermal_zone_device_register(const char
> > > *type, int trips, int mask,
> > > goto unregister;
> > > }
> > >
> > > + if (tz->tzp && tz->tzp->userspace)
> > > + thermal_zone_device_disable(tz);
> > > +
> > > mutex_lock(_list_lock);
> > > list_add_tail(>node, _tz_list);
> > > mutex_unlock(_list_lock);
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index d07ea27e72a9..e8e8fac78fc8 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -247,6 +247,8 @@ struct thermal_zone_params {
> > >*/
> > > bool no_hwmon;
> > >
> > > + bool userspace;
> > > +
> > > int num_tbps; /* Number of tbp entries */
> > > struct thermal_bind_params *tbp;
> > >
> > >
> >
> > --
> > Facebook |
> > Twitter |
> >
Re: [PATCH 2/3] thermal: int340x: Indicate userspace usage
On Sun, 2020-11-29 at 01:54 +0800, Kai-Heng Feng wrote:
> The device isn't present under ACPI ThermalZone, and there's a
> dedicated
> userspace daemon for this thermal device.
>
> Let thermal core know it shouldn't handle trips to avoid surprising
> thermal shutdown.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 1 +
> .../thermal/intel/int340x_thermal/int340x_thermal_zone.c| 6 +---
> --
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 0966551cbaaa..2002bc96eb3c 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -439,6 +439,7 @@ static struct thermal_zone_device_ops
> int3400_thermal_ops = {
> static struct thermal_zone_params int3400_thermal_params = {
> .governor_name = "user_space",
> .no_hwmon = true,
> + .userspace = true,
I am copied on only this patch, so I don't know what is this attribute?
I think it is new.
> };
>
> static void int3400_setup_gddv(struct int3400_thermal_priv *priv)
> diff --git
> a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> index 6e479deff76b..a103eb42ef2d 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -208,6 +208,7 @@ EXPORT_SYMBOL_GPL(int340x_thermal_read_trips);
> static struct thermal_zone_params int340x_thermal_params = {
> .governor_name = "user_space",
> .no_hwmon = true,
> + .userspace = true,
> };
>
> struct int34x_thermal_zone *int340x_thermal_zone_add(struct
> acpi_device *adev,
> @@ -259,14 +260,9 @@ struct int34x_thermal_zone
> *int340x_thermal_zone_add(struct acpi_device *adev,
> ret = PTR_ERR(int34x_thermal_zone->zone);
> goto err_thermal_zone;
> }
> - ret = thermal_zone_device_enable(int34x_thermal_zone->zone);
> - if (ret)
> - goto err_enable;
What is the effect of this?
The INT340X zones don't need to poll for temperature. When HW notifies
then user space gets notified via user space governor. Not sure if the
not enabling break that path.
Thanks,
Srinivas
>
> return int34x_thermal_zone;
>
> -err_enable:
> - thermal_zone_device_unregister(int34x_thermal_zone->zone);
> err_thermal_zone:
> acpi_lpat_free_conversion_table(int34x_thermal_zone-
> >lpat_table);
> kfree(int34x_thermal_zone->aux_trips);
[PATCH 3/4] thermal: int340x: processor_thermal: Add RFIM driver
Add support for RFIM (Radio Frequency Interference Mitigation) support
via processor thermal PCI device. This drivers allows adjustment of
FIVR (Fully Integrated Voltage Regulator) and DDR (Double Data Rate)
frequencies to avoid RF interference with WiFi and 5G.
Switching voltage regulators (VR) generate radiated EMI or RFI at the
fundamental frequency and its harmonics. Some harmonics may interfere
with very sensitive wireless receivers such as Wi-Fi and cellular that
are integrated into host systems like notebook PCs. One of mitigation
methods is requesting SOC integrated VR (IVR) switching frequency to a
small % and shift away the switching noise harmonic interference from
radio channels. OEM or ODMs can use the driver to control SOC IVR
operation within the range where it does not impact IVR performance.
DRAM devices of DDR IO interface and their power plane can generate EMI
at the data rates. Similar to IVR control mechanism, Intel offers a
mechanism by which DDR data rates can be changed if several conditions
are met: there is strong RFI interference because of DDR; CPU power
management has no other restriction in changing DDR data rates;
PC ODMs enable this feature (real time DDR RFI Mitigation referred to as
DDR-RFIM) for Wi-Fi from BIOS.
This change exports two folders under /sys/bus/pci/devices/:00:04.0.
One folder "fivr" contains all attributes exposed for controling FIVR
features. The other folder "dvfs" contains all attributes for DDR
features.
Changes done to implement:
- New module for rfim interfaces
- Two new per processor features for DDR and FIVR
- Enable feature for Tiger Lake (FIVR only) and Alder Lake
The attributes exposed and explanation:
FIVR attributes
vco_ref_code_lo (RW): The VCO reference code is an 11-bit field and
controls the FIVR switching frequency. This is the 3-bit LSB field.
vco_ref_code_hi (RW): The VCO reference code is an 11-bit field and
controls the FIVR switching frequency. This is the 8-bit MSB field.
spread_spectrum_pct (RW): Set the FIVR spread spectrum clocking
percentage
spread_spectrum_clk_enable (RW): Enable/disable of the FIVR spread
spectrum clocking feature
rfi_vco_ref_code (RW): This field is a read only status register which
reflects the current FIVR switching frequency
fivr_fffc_rev (RW): This field indicated the revision of the FIVR HW.
DVFS attributes
rfi_restriction_run_busy (RW): Request the restriction of specific DDR
data rate and set this value 1. Self reset to 0 after operation.
rfi_restriction_err_code (RW): Values: 0 :Request is accepted, 1:Feature
disabled, 2: the request restricts more points than it is allowed
rfi_restriction_data_rate_Delta (RW): Restricted DDR data rate for RFI
protection: Lower Limit
rfi_restriction_data_rate_Base (RW): Restricted DDR data rate for RFI
protection: Upper Limit
ddr_data_rate_point_0 (RO): DDR data rate selection 1st point
ddr_data_rate_point_1 (RO): DDR data rate selection 2nd point
ddr_data_rate_point_2 (RO): DDR data rate selection 3rd point
ddr_data_rate_point_3 (RO): DDR data rate selection 4th point
rfi_disable (RW): Disable DDR rate change feature
Signed-off-by: Srinivas Pandruvada
---
.../thermal/intel/int340x_thermal/Makefile| 1 +
.../processor_thermal_device.c| 23 +-
.../processor_thermal_device.h| 5 +
.../int340x_thermal/processor_thermal_rfim.c | 244 ++
4 files changed, 270 insertions(+), 3 deletions(-)
create mode 100644
drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
diff --git a/drivers/thermal/intel/int340x_thermal/Makefile
b/drivers/thermal/intel/int340x_thermal/Makefile
index 86e8d3c87df7..f4e2eb7d9606 100644
--- a/drivers/thermal/intel/int340x_thermal/Makefile
+++ b/drivers/thermal/intel/int340x_thermal/Makefile
@@ -5,5 +5,6 @@ obj-$(CONFIG_INT340X_THERMAL) += int3402_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += int3403_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_device.o
obj-$(CONFIG_PROC_THERMAL_MMIO_RAPL) += processor_thermal_rapl.o
+obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_rfim.o
obj-$(CONFIG_INT3406_THERMAL) += int3406_thermal.o
obj-$(CONFIG_ACPI_THERMAL_REL) += acpi_thermal_rel.o
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index 589ac7deec02..b6a7358b989d 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -429,6 +429,8 @@ static int proc_thermal_mmio_add(struct pci_dev *pdev,
{
int ret;
+ proc_priv->mmio_feature_mask = feature_mask;
+
if (feature_mask) {
ret = proc_thermal_set_mmio_base(pdev, proc_priv);
if (ret)
@@ -443,9 +445,21 @@ static int proc_thermal_mmio_add(struct pci_dev *pdev,
}
}
- proc_priv->mmio_f
[PATCH 1/4] thermal: int340x: processor_thermal: Refactor MMIO interface
The Processor Thermal PCI device supports multiple features. Currently
we export only RAPL. But we need more features from this device exposed
for Tiger Lake and Alder Lake based platforms. So re-structure the
current MMIO interface, so that more features can be added cleanly.
No functional changes are expected with this change.
Changes done in this patch:
- Using PCI_DEVICE_DATA(), hence names of defines changed
- Move RAPL MMIO code to its own module
- Move the RAPL MMIO offsets to RAPL MMIO module
- Adjust Kconfig dependency of PROC_THERMAL_MMIO_RAPL
- Per processor driver data now contains the supported features
- Moved all the common data structures and defines to a common header
file
- This new header file contains all the processor_thermal_* interfaces
- Based on the features supported the module interface is called
- Each module atleast provides one add and one remove function
Signed-off-by: Srinivas Pandruvada
---
drivers/thermal/intel/int340x_thermal/Kconfig | 6 +-
.../thermal/intel/int340x_thermal/Makefile| 1 +
.../processor_thermal_device.c| 261 --
.../processor_thermal_device.h| 72 +
.../int340x_thermal/processor_thermal_rapl.c | 134 +
5 files changed, 263 insertions(+), 211 deletions(-)
create mode 100644
drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
create mode 100644
drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
diff --git a/drivers/thermal/intel/int340x_thermal/Kconfig
b/drivers/thermal/intel/int340x_thermal/Kconfig
index 797907542e43..45c31f3d6054 100644
--- a/drivers/thermal/intel/int340x_thermal/Kconfig
+++ b/drivers/thermal/intel/int340x_thermal/Kconfig
@@ -10,6 +10,7 @@ config INT340X_THERMAL
select ACPI_THERMAL_REL
select ACPI_FAN
select INTEL_SOC_DTS_IOSF_CORE
+ select PROC_THERMAL_MMIO_RAPL if X86_64 && POWERCAP
help
Newer laptops and tablets that use ACPI may have thermal sensors and
other devices with thermal control capabilities outside the core
@@ -41,9 +42,6 @@ config INT3406_THERMAL
power consumed by display device.
config PROC_THERMAL_MMIO_RAPL
- bool
- depends on 64BIT
- depends on POWERCAP
+ tristate
select INTEL_RAPL_CORE
- default y
endif
diff --git a/drivers/thermal/intel/int340x_thermal/Makefile
b/drivers/thermal/intel/int340x_thermal/Makefile
index 287eb0a1476d..86e8d3c87df7 100644
--- a/drivers/thermal/intel/int340x_thermal/Makefile
+++ b/drivers/thermal/intel/int340x_thermal/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_INT340X_THERMAL) += int340x_thermal_zone.o
obj-$(CONFIG_INT340X_THERMAL) += int3402_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += int3403_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_device.o
+obj-$(CONFIG_PROC_THERMAL_MMIO_RAPL) += processor_thermal_rapl.o
obj-$(CONFIG_INT3406_THERMAL) += int3406_thermal.o
obj-$(CONFIG_ACPI_THERMAL_REL) += acpi_thermal_rel.o
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index 81e8b15ef405..5b8dc5e9ec86 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -12,74 +12,18 @@
#include
#include
#include
-#include
#include "int340x_thermal_zone.h"
+#include "processor_thermal_device.h"
#include "../intel_soc_dts_iosf.h"
-/* Broadwell-U/HSB thermal reporting device */
-#define PCI_DEVICE_ID_PROC_BDW_THERMAL 0x1603
-#define PCI_DEVICE_ID_PROC_HSB_THERMAL 0x0A03
-
-/* Skylake thermal reporting device */
-#define PCI_DEVICE_ID_PROC_SKL_THERMAL 0x1903
-
-/* CannonLake thermal reporting device */
-#define PCI_DEVICE_ID_PROC_CNL_THERMAL 0x5a03
-#define PCI_DEVICE_ID_PROC_CFL_THERMAL 0x3E83
-
-/* Braswell thermal reporting device */
-#define PCI_DEVICE_ID_PROC_BSW_THERMAL 0x22DC
-
-/* Broxton thermal reporting device */
-#define PCI_DEVICE_ID_PROC_BXT0_THERMAL 0x0A8C
-#define PCI_DEVICE_ID_PROC_BXT1_THERMAL 0x1A8C
-#define PCI_DEVICE_ID_PROC_BXTX_THERMAL 0x4A8C
-#define PCI_DEVICE_ID_PROC_BXTP_THERMAL 0x5A8C
-
-/* GeminiLake thermal reporting device */
-#define PCI_DEVICE_ID_PROC_GLK_THERMAL 0x318C
-
-/* IceLake thermal reporting device */
-#define PCI_DEVICE_ID_PROC_ICL_THERMAL 0x8a03
-
-/* JasperLake thermal reporting device */
-#define PCI_DEVICE_ID_PROC_JSL_THERMAL 0x4E03
-
-/* TigerLake thermal reporting device */
-#define PCI_DEVICE_ID_PROC_TGL_THERMAL 0x9A03
-
#define DRV_NAME "proc_thermal"
-struct power_config {
- u32 index;
- u32 min_uw;
- u32 max_uw;
- u32 tmin_us;
- u32 tmax_us;
- u32 step_uw;
-};
-
-struct proc_thermal_device {
- struct device *dev;
- struct acpi_device *adev;
- struct power_config power_limits[2];
- struct i
[PATCH 2/4] thermal: int340x: processor_thermal: Add AlderLake PCI device id
Added AlderLake PCI device id to support processor thermal driver. Reuse
the feature set (just includes RAPL) from previous generations.
Signed-off-by: Srinivas Pandruvada
---
drivers/thermal/intel/int340x_thermal/processor_thermal_device.c | 1 +
drivers/thermal/intel/int340x_thermal/processor_thermal_device.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index 5b8dc5e9ec86..589ac7deec02 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -566,6 +566,7 @@ static int proc_thermal_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(proc_thermal_pm, NULL, proc_thermal_resume);
static const struct pci_device_id proc_thermal_pci_ids[] = {
+ { PCI_DEVICE_DATA(INTEL, ADL_THERMAL, PROC_THERMAL_FEATURE_RAPL) },
{ PCI_DEVICE_DATA(INTEL, BDW_THERMAL, 0) },
{ PCI_DEVICE_DATA(INTEL, BSW_THERMAL, 0) },
{ PCI_DEVICE_DATA(INTEL, BXT0_THERMAL, 0) },
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
index e20d142a55b4..45214571e00d 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
@@ -9,6 +9,7 @@
#include
+#define PCI_DEVICE_ID_INTEL_ADL_THERMAL0x461d
#define PCI_DEVICE_ID_INTEL_BDW_THERMAL0x1603
#define PCI_DEVICE_ID_INTEL_BSW_THERMAL0x22DC
--
2.25.4
[PATCH 4/4] thermal: int340x: processor_thermal: Add mailbox driver
Added processor thermal device mail box interface for workload hints
setting. These hints will give indication to hardware to better manage
power and thermals. The supported hints are:
idle
semi_active
burusty
sustained
battery_life
For example when the system is on battery, the hardware can be less
aggressive in power ramp up.
This will create an attribute group at
/sys/bus/pci/devices/:00:04.0/workload_request
This folder contains two attributes:
workload_available_types : (RO): This shows available workload types
workload_type: (RW) : Allows to set and get current workload type
setting
Signed-off-by: Srinivas Pandruvada
---
.../thermal/intel/int340x_thermal/Makefile| 1 +
.../processor_thermal_device.c| 17 +-
.../processor_thermal_device.h| 4 +
.../int340x_thermal/processor_thermal_mbox.c | 212 ++
4 files changed, 232 insertions(+), 2 deletions(-)
create mode 100644
drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
diff --git a/drivers/thermal/intel/int340x_thermal/Makefile
b/drivers/thermal/intel/int340x_thermal/Makefile
index f4e2eb7d9606..38a2731e503c 100644
--- a/drivers/thermal/intel/int340x_thermal/Makefile
+++ b/drivers/thermal/intel/int340x_thermal/Makefile
@@ -6,5 +6,6 @@ obj-$(CONFIG_INT340X_THERMAL) += int3403_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_device.o
obj-$(CONFIG_PROC_THERMAL_MMIO_RAPL) += processor_thermal_rapl.o
obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_rfim.o
+obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_mbox.o
obj-$(CONFIG_INT3406_THERMAL) += int3406_thermal.o
obj-$(CONFIG_ACPI_THERMAL_REL) += acpi_thermal_rel.o
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index b6a7358b989d..9e6f2a895a23 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -454,8 +454,18 @@ static int proc_thermal_mmio_add(struct pci_dev *pdev,
}
}
+ if (feature_mask & PROC_THERMAL_FEATURE_MBOX) {
+ ret = proc_thermal_mbox_add(pdev, proc_priv);
+ if (ret) {
+ dev_err(>dev, "failed to add MBOX interface\n");
+ goto err_rem_rfim;
+ }
+ }
+
return 0;
+err_rem_rfim:
+ proc_thermal_rfim_remove(pdev);
err_rem_rapl:
proc_thermal_rapl_remove();
@@ -472,6 +482,9 @@ static void proc_thermal_mmio_remove(struct pci_dev *pdev)
if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_FIVR ||
proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DVFS)
proc_thermal_rfim_remove(pdev);
+
+ if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_MBOX)
+ proc_thermal_mbox_remove(pdev);
}
static int proc_thermal_pci_probe(struct pci_dev *pdev,
@@ -583,7 +596,7 @@ static int proc_thermal_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(proc_thermal_pm, NULL, proc_thermal_resume);
static const struct pci_device_id proc_thermal_pci_ids[] = {
- { PCI_DEVICE_DATA(INTEL, ADL_THERMAL, PROC_THERMAL_FEATURE_RAPL |
PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS) },
+ { PCI_DEVICE_DATA(INTEL, ADL_THERMAL, PROC_THERMAL_FEATURE_RAPL |
PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS |
PROC_THERMAL_FEATURE_MBOX) },
{ PCI_DEVICE_DATA(INTEL, BDW_THERMAL, 0) },
{ PCI_DEVICE_DATA(INTEL, BSW_THERMAL, 0) },
{ PCI_DEVICE_DATA(INTEL, BXT0_THERMAL, 0) },
@@ -597,7 +610,7 @@ static const struct pci_device_id proc_thermal_pci_ids[] = {
{ PCI_DEVICE_DATA(INTEL, ICL_THERMAL, PROC_THERMAL_FEATURE_RAPL) },
{ PCI_DEVICE_DATA(INTEL, JSL_THERMAL, 0) },
{ PCI_DEVICE_DATA(INTEL, SKL_THERMAL, PROC_THERMAL_FEATURE_RAPL) },
- { PCI_DEVICE_DATA(INTEL, TGL_THERMAL, PROC_THERMAL_FEATURE_RAPL |
PROC_THERMAL_FEATURE_FIVR) },
+ { PCI_DEVICE_DATA(INTEL, TGL_THERMAL, PROC_THERMAL_FEATURE_RAPL |
PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_MBOX) },
{ },
};
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
index 4bbb88f6b4a7..b9ed64561aaf 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
@@ -56,6 +56,7 @@ struct rapl_mmio_regs {
#define PROC_THERMAL_FEATURE_RAPL 0x01
#define PROC_THERMAL_FEATURE_FIVR 0x02
#define PROC_THERMAL_FEATURE_DVFS 0x04
+#define PROC_THERMAL_FEATURE_MBOX 0x08
#if IS_ENABLED(CONFIG_PROC_THERMAL_MMIO_RAPL)
int proc_thermal_rapl_add(struct pci_dev *pdev, struct proc_thermal_device
*proc_priv);
@@ -75,4 +76,7 @@ static void __maybe_unused proc_thermal_rapl_r
[PATCH] thermal: int340x: Support Alder Lake
Add ACPI IDs for thermal drivers for Alder Lake support.
Signed-off-by: Srinivas Pandruvada
---
drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 1 +
drivers/thermal/intel/int340x_thermal/int3403_thermal.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 0966551cbaaa..823354a1a91a 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -584,6 +584,7 @@ static int int3400_thermal_remove(struct platform_device
*pdev)
static const struct acpi_device_id int3400_thermal_match[] = {
{"INT3400", 0},
{"INTC1040", 0},
+ {"INTC1041", 0},
{}
};
diff --git a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
index ec1d58c4ceaa..c3c4c4d34542 100644
--- a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
@@ -284,6 +284,7 @@ static int int3403_remove(struct platform_device *pdev)
static const struct acpi_device_id int3403_device_ids[] = {
{"INT3403", 0},
{"INTC1043", 0},
+ {"INTC1046", 0},
{"", 0},
};
MODULE_DEVICE_TABLE(acpi, int3403_device_ids);
--
2.25.4
[PATCH] ACPI: DPTF: Support Alder Lake
Add Alder Lake ACPI IDs for DPTF devices.
Signed-off-by: Srinivas Pandruvada
---
drivers/acpi/dptf/dptf_pch_fivr.c | 1 +
drivers/acpi/dptf/dptf_power.c | 2 ++
drivers/acpi/dptf/int340x_thermal.c | 6 ++
drivers/acpi/fan.c | 1 +
4 files changed, 10 insertions(+)
diff --git a/drivers/acpi/dptf/dptf_pch_fivr.c
b/drivers/acpi/dptf/dptf_pch_fivr.c
index 4c1992fce150..5fca18296bf6 100644
--- a/drivers/acpi/dptf/dptf_pch_fivr.c
+++ b/drivers/acpi/dptf/dptf_pch_fivr.c
@@ -106,6 +106,7 @@ static int pch_fivr_remove(struct platform_device *pdev)
static const struct acpi_device_id pch_fivr_device_ids[] = {
{"INTC1045", 0},
+ {"INTC1049", 0},
{"", 0},
};
MODULE_DEVICE_TABLE(acpi, pch_fivr_device_ids);
diff --git a/drivers/acpi/dptf/dptf_power.c b/drivers/acpi/dptf/dptf_power.c
index 06741305fc77..a24d5d7aa117 100644
--- a/drivers/acpi/dptf/dptf_power.c
+++ b/drivers/acpi/dptf/dptf_power.c
@@ -229,6 +229,8 @@ static const struct acpi_device_id int3407_device_ids[] = {
{"INT3532", 0},
{"INTC1047", 0},
{"INTC1050", 0},
+ {"INTC1060", 0},
+ {"INTC1061", 0},
{"", 0},
};
MODULE_DEVICE_TABLE(acpi, int3407_device_ids);
diff --git a/drivers/acpi/dptf/int340x_thermal.c
b/drivers/acpi/dptf/int340x_thermal.c
index 8d420c7e7178..d14025a85ce8 100644
--- a/drivers/acpi/dptf/int340x_thermal.c
+++ b/drivers/acpi/dptf/int340x_thermal.c
@@ -25,10 +25,16 @@ static const struct acpi_device_id
int340x_thermal_device_ids[] = {
{"INT340A"},
{"INT340B"},
{"INTC1040"},
+ {"INTC1041"},
{"INTC1043"},
{"INTC1044"},
{"INTC1045"},
+ {"INTC1046"},
{"INTC1047"},
+ {"INTC1048"},
+ {"INTC1049"},
+ {"INTC1060"},
+ {"INTC1061"},
{""},
};
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 62873388b24f..48354f82fba6 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -27,6 +27,7 @@ static const struct acpi_device_id fan_device_ids[] = {
{"PNP0C0B", 0},
{"INT3404", 0},
{"INTC1044", 0},
+ {"INTC1048", 0},
{"", 0},
};
MODULE_DEVICE_TABLE(acpi, fan_device_ids);
--
2.25.4
Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support
On Thu, 2020-11-05 at 14:53 +1100, Victor Ding wrote: > On Wed, Nov 4, 2020 at 1:17 PM Srinivas Pandruvada > wrote: > > On Wed, 2020-11-04 at 12:43 +1100, Victor Ding wrote: > > > On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada > > > wrote: > > > > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > > > > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui < > > > > > rui.zh...@intel.com> > > > > > wrote: > > > > > > On Tue, 2020-10-27 at 07:23 +, Victor Ding wrote: > > > > > > > This patch enables AMD Fam17h RAPL support for the power > > > > > > > capping > > > > > > > framework. The support is as per AMD Fam17h Model31h > > > > > > > (Zen2) > > > > > > > and > > > > > > > model 00-ffh (Zen1) PPR. > > > > > > > > > > > > > > Tested by comparing the results of following two sysfs > > > > > > > entries > > > > > > > and > > > > > > > the > > > > > > > values directly read from corresponding MSRs via > > > > > > > /dev/cpu/[x]/msr: > > > > > > > /sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj > > > > > > > /sys/class/powercap/intel-rapl/intel-rapl:0/intel- > > > > > > > rapl:0:0/energy_uj > > > > > > > > Is this for just energy reporting? No capping of power? > > > Correct, the hardware does not support capping of power. > > I wonder if there is no capping, is this the right interface? > > Do you have specific user space, which cares about this? > We have tools that previously developed to measure energy status > on Intel via the powercap interface. Powercap is the only interface > allowing reading RAPL energy counters without requiring MSR access > privileges. We want to use these tools on AMD with minimal > modifications. > I believe the powercap interface should support these counters, > regardless of the use cases, mainly for two reasons: > 1. Powercap interface already supports monitoring-only power domains, > e.g. power limit is locked by BIOS or the (Intel) CPU does not expose > an > MSR for certain power domains. The latter is the exact situation on > AMD; > 2. As AMD has partially introduced the equivalent of Intel's RAPL, we > should leverage this opportunity to reduce the divergence in the > APIs. i.e. > OS as a hardware abstraction layer should allow users to use the same > set of APIs to access RAPL features if it issupported on both Intel > and AMD. > In this specific case, if users can query for Intel's RAPL counters > via > powercap, they should be able to do so as well for AMD's. > > I think these counters are already exposed via hwmon sysf. > Yes, they were introduced early this year. However, it is not the > same as > the counters exposed via powercap interface: powercap exposes the > actual value of the energy counters while hwmon adds an accumulation > layer on top. > In addition, I don't think Intel's RAPL counters are exposed via > hwmon; > therefore: 1. existing fine grade power monitoring tools are not > based on > hwmon; 2. new tools cannot query the same set of counters via the > same > API so that they have to actively maintain two sets of logic. Fine with me. I think eventually the power capping interface will be supported. Thanks, Srinivas > > Thanks, > > Srinivas > > > > > > Thanks, > > > > Srinivas > > > > > > > > > > > > > > > Signed-off-by: Victor Ding > > > > > > > Acked-by: Kim Phillips > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > Changes in v3: > > > > > > > By Victor Ding > > > > > > > - Rebased to the latest code. > > > > > > > - Created a new rapl_defaults for AMD CPUs. > > > > > > > - Removed redundant setting to zeros. > > > > > > > - Stopped using the fake power limit domain 1. > > > > > > > > > > > > > > Changes in v2: > > > > > > > By Kim Phillips : > > > > > > > - Added Kim's Acked-by. > > > > > > > - Added Daniel Lezcano to Cc. > > > > > > > - (No code change). > > > > > > > > > > > > > > arch/x86/include/asm/msr-index.h |
Re: [PATCH] HID: hid-sensor-hub: Fix issue with devices with no report ID
On Mon, 2020-11-02 at 19:29 -0500, Pablo Ceballos wrote: > Some HID devices don't use a report ID because they only have a > single > report. In those cases, the report ID in struct hid_report will be > zero > and the data for the report will start at the first byte, so don't > skip > over the first byte. > > Signed-off-by: Pablo Ceballos Acked-by: Srinivas Pandruvada > --- > drivers/hid/hid-sensor-hub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor- > hub.c > index 94c7398b5c27..3dd7d3246737 100644 > --- a/drivers/hid/hid-sensor-hub.c > +++ b/drivers/hid/hid-sensor-hub.c > @@ -483,7 +483,8 @@ static int sensor_hub_raw_event(struct hid_device > *hdev, > return 1; > > ptr = raw_data; > - ptr++; /* Skip report id */ > + if (report->id) > + ptr++; /* Skip report id */ > > spin_lock_irqsave(>lock, flags); >
Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support
On Wed, 2020-11-04 at 12:43 +1100, Victor Ding wrote:
> On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada
> wrote:
> > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote:
> > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui
> > > wrote:
> > > > On Tue, 2020-10-27 at 07:23 +, Victor Ding wrote:
> > > > > This patch enables AMD Fam17h RAPL support for the power
> > > > > capping
> > > > > framework. The support is as per AMD Fam17h Model31h (Zen2)
> > > > > and
> > > > > model 00-ffh (Zen1) PPR.
> > > > >
> > > > > Tested by comparing the results of following two sysfs
> > > > > entries
> > > > > and
> > > > > the
> > > > > values directly read from corresponding MSRs via
> > > > > /dev/cpu/[x]/msr:
> > > > > /sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj
> > > > > /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
> > > > > rapl:0:0/energy_uj
> >
> > Is this for just energy reporting? No capping of power?
> Correct, the hardware does not support capping of power.
I wonder if there is no capping, is this the right interface?
Do you have specific user space, which cares about this?
I think these counters are already exposed via hwmon sysf.
Thanks,
Srinivas
> > Thanks,
> > Srinivas
> >
> >
> > > > > Signed-off-by: Victor Ding
> > > > > Acked-by: Kim Phillips
> > > > >
> > > > >
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > > By Victor Ding
> > > > > - Rebased to the latest code.
> > > > > - Created a new rapl_defaults for AMD CPUs.
> > > > > - Removed redundant setting to zeros.
> > > > > - Stopped using the fake power limit domain 1.
> > > > >
> > > > > Changes in v2:
> > > > > By Kim Phillips :
> > > > > - Added Kim's Acked-by.
> > > > > - Added Daniel Lezcano to Cc.
> > > > > - (No code change).
> > > > >
> > > > > arch/x86/include/asm/msr-index.h | 1 +
> > > > > drivers/powercap/intel_rapl_common.c | 6 ++
> > > > > drivers/powercap/intel_rapl_msr.c| 20
> > > > > +++-
> > > > > 3 files changed, 26 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/x86/include/asm/msr-index.h
> > > > > b/arch/x86/include/asm/msr-index.h
> > > > > index 21917e134ad4..c36a083c8ec0 100644
> > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > @@ -327,6 +327,7 @@
> > > > > #define MSR_PP1_POLICY 0x0642
> > > > >
> > > > > #define MSR_AMD_RAPL_POWER_UNIT 0xc0010299
> > > > > +#define MSR_AMD_CORE_ENERGY_STATUS 0xc001029a
> > > > > #define MSR_AMD_PKG_ENERGY_STATUS0xc001029b
> > > > >
> > > > > /* Config TDP MSRs */
> > > > > diff --git a/drivers/powercap/intel_rapl_common.c
> > > > > b/drivers/powercap/intel_rapl_common.c
> > > > > index 0b2830efc574..bedd780bed12 100644
> > > > > --- a/drivers/powercap/intel_rapl_common.c
> > > > > +++ b/drivers/powercap/intel_rapl_common.c
> > > > > @@ -1011,6 +1011,10 @@ static const struct rapl_defaults
> > > > > rapl_defaults_cht = {
> > > > > .compute_time_window = rapl_compute_time_window_atom,
> > > > > };
> > > > >
> > > > > +static const struct rapl_defaults rapl_defaults_amd = {
> > > > > + .check_unit = rapl_check_unit_core,
> > > > > +};
> > > > > +
> > > >
> > > > why do we need power_unit and time_unit if we only want to
> > > > expose
> > > > the
> > > > energy counter?
> > > AMD's Power Unit MSR provides identical information as Intel's,
> > > including
> > > time units, power units, and energy status units. By reusing the
> > > check unit
> > > method, we could avoid code duplication as well as easing future
> > > enhance-
> > > ment when AMD starts to support power limits.
> > > > Plus, in rapl_init_domains(), PL1 is enabled for every RAPL
> > > &
Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support
On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote:
> On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui
> wrote:
> > On Tue, 2020-10-27 at 07:23 +, Victor Ding wrote:
> > > This patch enables AMD Fam17h RAPL support for the power capping
> > > framework. The support is as per AMD Fam17h Model31h (Zen2) and
> > > model 00-ffh (Zen1) PPR.
> > >
> > > Tested by comparing the results of following two sysfs entries
> > > and
> > > the
> > > values directly read from corresponding MSRs via
> > > /dev/cpu/[x]/msr:
> > > /sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj
> > > /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
> > > rapl:0:0/energy_uj
Is this for just energy reporting? No capping of power?
Thanks,
Srinivas
> > >
> > > Signed-off-by: Victor Ding
> > > Acked-by: Kim Phillips
> > >
> > >
> > > ---
> > >
> > > Changes in v3:
> > > By Victor Ding
> > > - Rebased to the latest code.
> > > - Created a new rapl_defaults for AMD CPUs.
> > > - Removed redundant setting to zeros.
> > > - Stopped using the fake power limit domain 1.
> > >
> > > Changes in v2:
> > > By Kim Phillips :
> > > - Added Kim's Acked-by.
> > > - Added Daniel Lezcano to Cc.
> > > - (No code change).
> > >
> > > arch/x86/include/asm/msr-index.h | 1 +
> > > drivers/powercap/intel_rapl_common.c | 6 ++
> > > drivers/powercap/intel_rapl_msr.c| 20 +++-
> > > 3 files changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/msr-index.h
> > > b/arch/x86/include/asm/msr-index.h
> > > index 21917e134ad4..c36a083c8ec0 100644
> > > --- a/arch/x86/include/asm/msr-index.h
> > > +++ b/arch/x86/include/asm/msr-index.h
> > > @@ -327,6 +327,7 @@
> > > #define MSR_PP1_POLICY 0x0642
> > >
> > > #define MSR_AMD_RAPL_POWER_UNIT 0xc0010299
> > > +#define MSR_AMD_CORE_ENERGY_STATUS 0xc001029a
> > > #define MSR_AMD_PKG_ENERGY_STATUS0xc001029b
> > >
> > > /* Config TDP MSRs */
> > > diff --git a/drivers/powercap/intel_rapl_common.c
> > > b/drivers/powercap/intel_rapl_common.c
> > > index 0b2830efc574..bedd780bed12 100644
> > > --- a/drivers/powercap/intel_rapl_common.c
> > > +++ b/drivers/powercap/intel_rapl_common.c
> > > @@ -1011,6 +1011,10 @@ static const struct rapl_defaults
> > > rapl_defaults_cht = {
> > > .compute_time_window = rapl_compute_time_window_atom,
> > > };
> > >
> > > +static const struct rapl_defaults rapl_defaults_amd = {
> > > + .check_unit = rapl_check_unit_core,
> > > +};
> > > +
> >
> > why do we need power_unit and time_unit if we only want to expose
> > the
> > energy counter?
> AMD's Power Unit MSR provides identical information as Intel's,
> including
> time units, power units, and energy status units. By reusing the
> check unit
> method, we could avoid code duplication as well as easing future
> enhance-
> ment when AMD starts to support power limits.
> > Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain
> > blindly, I'm not sure how this is handled on the AMD CPUs.
> > Is PL1 invalidated by rapl_detect_powerlimit()? or is it still
> > registered as a valid constraint into powercap sysfs I/F?
> AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS;
> therefore, PL1 also always exists on AMD. rapl_detect_powerlimit()
> correctly
> markes the domain as monitoring-only after finding power limit MSRs
> do not
> exist.
> > Currently, the code makes the assumption that there is only on
> > power
> > limit if priv->limits[domain_id] not set, we probably need to
> > change
> > this if we want to support RAPL domains with no power limit.
> The existing code already supports RAPL domains with no power limit:
> PL1 is
> enabled when there is zero or one power limit,
> rapl_detect_powerlimit() will then
> mark if PL1 is monitoring-only if power limit MSRs do not exist. Both
> AMD's RAPL
> domains are monitoring-only and are correctly marked and handled.
> > thanks,
> > rui
> > > static const struct x86_cpu_id rapl_ids[] __initconst = {
> > > X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE, _defau
> > > lt
> > > s_core),
> > > X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, _defau
> > > lts_core),
> > > @@ -1061,6 +1065,8 @@ static const struct x86_cpu_id rapl_ids[]
> > > __initconst = {
> > >
> > > X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,_defau
> > > lts_hsw_se
> > > rver),
> > > X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM,_defau
> > > lts_hsw_se
> > > rver),
> > > +
> > > + X86_MATCH_VENDOR_FAM(AMD, 0x17, _defaults_amd),
> > > {}
> > > };
> > > MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
> > > diff --git a/drivers/powercap/intel_rapl_msr.c
> > > b/drivers/powercap/intel_rapl_msr.c
> > > index a819b3b89b2f..78213d4b5b16 100644
> > > --- a/drivers/powercap/intel_rapl_msr.c
> > > +++ b/drivers/powercap/intel_rapl_msr.c
> > > @@ -49,6 +49,14 @@ static struct rapl_if_priv rapl_msr_priv_intel
> > > = {
> > >
Re: [PATCH] x86/msr: do not warn on writes to OC_MAILBOX
On Tue, 2020-10-20 at 12:40 -0700, Dave Hansen wrote: > On 10/20/20 11:40 AM, Srinivas Pandruvada wrote: > > On Tue, 2020-10-20 at 19:47 +0200, Borislav Petkov wrote: > > > On Tue, Oct 20, 2020 at 10:21:48AM -0700, Srinivas Pandruvada > > > wrote: > > > > These command id are model specific. There is no guarantee that > > > > even > > > > meaning changes. So I don't think we should write any code in > > > > kernel > > > > which can't stick. > > > Ok, is there a common *set* of values present on all models > > Sorry, don't know. > > So, the question is: Is Intel willing to document this on a > sufficient > number of models that folks can make a sane driver out of it? > > Srinivas, that seems like a pretty sane thing for the community to > ask. > We've got random folks poking at MSRs and we don't know whether > they're > nuts or not and whether we should spew warnings of disdain. Seems > like > it would be in Intel's best interests to understand what users are > doing > with this MSR and to try to make sure they're not doing stuff which > is > too nutty, or at least give them the chance of avoiding warnings if > they're being nice. We are all for it. We have added several sysfs interfaces and adding more. > > Sounds like Borislav is willing to help give Intel's customers a > nicer > interface. Mostly we from Intel would have to go dig out the docs > for > as many models as we can, and make sure we're allowed to talk about > it > publicly. > That is the problem. There is no public document. > I dunno. Maybe we should try it for *one* model and see how it goes. > Maybe start with the one we're already poking from inside the kernel.
Re: [PATCH] x86/msr: do not warn on writes to OC_MAILBOX
On Tue, 2020-10-20 at 19:47 +0200, Borislav Petkov wrote: > On Tue, Oct 20, 2020 at 10:21:48AM -0700, Srinivas Pandruvada wrote: > > These command id are model specific. There is no guarantee that > > even > > meaning changes. So I don't think we should write any code in > > kernel > > which can't stick. > > Ok, is there a common *set* of values present on all models Sorry, don't know. > > A common set which we can abstract out from the MSR and have > userspace > write them into sysfs and the kernel does the model-specific write? > > The sysfs interface should simply provide the functionality, like, > for > example say: "we have X valid undervolt indices, choose one". > > Userspace doesn't have to deal with *how* that write happens and > which > bits need to be set in the MSR and depend on the model - that's all > abstracted away by the kernel. All userspace needs to care about is > *what* it wants done to the hw. The *how exactly* is done by the > kernel. > > And then the differences are done with x86 model tests. > > Does that make more sense? > > > May be something like this: > > - Separate mailbox stuff from intel_turbo_max_3.c > > Yah, that makes sense. > > > - Create a standalone module which creates a debugfs interface > > - This debugs interface takes one 64 bit value from user space and > > use > > protocol to avoid contention > > We can't make debugfs an API - debugfs can change at any point in > time. > If you want an API, you put it in sysfs or in a separate fs. Ok we can create a sysfs entry. > > > - Warns users on writes via new interfaces you suggested above > > > #define MSR_ADDR_TEMPERATURE 0x1a2 > > Need to check use case for undervolt. > > throttled uses it too. I asked them today to talk to us to design a > proper interface which satisfies their needs: > > https://github.com/erpalma/throttled/issues/215 > > > > #define MSR_ADDR_UNITS 0x606 > > Why not reuse powercap rapl interface. That interface will take > > care of > > units. > > Sure. > > Btw, you should have a look at those tools - they all poke at all > kinds > of MSRs and correcting that is like a whack-a-mole game... ;-\ > > Oh, and the kernel pokes at them too so imagine the surprise one > would have when > some kernel driver like > > drivers/thermal/intel/int340x_thermal/processor_thermal_device.c > > went and read some MSRs and then all of a sudden they changed because > some userspace daemon wrote them underneath it. Not good. Agree, that poking MSR from user space is not a right thing to do. Thanks, Srinivas > > Thx. >
Re: [PATCH] x86/msr: do not warn on writes to OC_MAILBOX
On Mon, 2020-10-19 at 19:15 +0200, Borislav Petkov wrote:
> On Tue, Sep 08, 2020 at 06:02:05PM -0700, Srinivas Pandruvada wrote:
> > The actual OC mailbox implementation itself is implemented in Linux
> > in
> > intel_turbo_max_3 driver. So that is public.
> > So someone can develop a driver and provide some sysfs to send
> > mailbox
> > commands, but kernel can't validate commands which can cause any
> > security or stability issues. Not sure if this is acceptable
> > standard.
> > I don't think there is any precedent of creating such blind sysfs
> > entries.
>
> So we don't need to validate those commands - we can issue a
> pr_warn_once() when something pokes at that to say that issuing those
> commands is dangerous.
>
> For example, from looking at
>
> drivers/platform/x86/intel_turbo_max_3.c::get_oc_core_priority()
>
> we should at least provide a well-defined interface to at least
> synchronize access to that MSR with the kernel. And then maybe allow
> a
> well-defined set of commands or better yet, we do them ourselves.
> Here's
> what I mean:
>
> Looking at the code in intel-undervolt:
>
> bool undervolt(struct config_t * config, bool * nl, bool write) {
> bool success = true;
> bool nll = false;
> int i;
>
> for (i = 0; config->undervolts && i < config->undervolts-
> >count; i++) {
> struct undervolt_t * undervolt = array_get(config-
> >undervolts, i);
>
> static const int mask = 0x800;
> uint64_t uvint = ((uint64_t) (mask - absf(undervolt-
> >value) * 1.024f +
> 0.5f) << 21) & 0x;
> uint64_t rdval = 0x8010 |
> ((uint64_t) undervolt->index << 40);
> uint64_t wrval = rdval | 0x1 | uvint;
>
> bool write_success = !write ||
> wr(config, MSR_ADDR_VOLTAGE, wrval);
> bool read_success = write_success &&
> wr(config, MSR_ADDR_VOLTAGE, rdval) &&
> rd(config, MSR_ADDR_VOLTAGE, rdval);
>
>
> That MSR_ADDR_VOLTAGE is 0x150, i.e., MSR_OC_MAILBOX.
>
> Trying to decipher the MSR accesses, it looks like it does the write
> with:
>
> 0x8010 | (0xf << 40) | (0x3 << 21) | 0x1
>
> and I've made the uvint 0x3 so that I can see the two 11s in the
> bitfield below.
>
> The undervolt index I made 0x for a similar reason:
>
> And the result is:
>
> Hex: 0x8f110060 Dec: 9.223.388.602.549.927.936
> 31 27 23 19 15 11 7331 27 23 19 15 11
> 73
> 1000______0001_0001___0110___
> __
> 63 59 55 51 47 43 39 35 31 27 23 19 15 11
> 73
>
> With
>
> - bit 63: MSR_OC_MAILBOX_BUSY_BIT
>
> - [47?:40]: that's some index, undervolting index, who knows. I'm
> assuming this is
> a byte, thus the 47?.
>
>
> - [39?:32]: cmd, in this case, 0x11, gonna assume that the command is
> bits [39:32]
> looking how this is a byte too:
>
> #define OC_MAILBOX_FC_CONTROL_CMD 0x1C
>
> and
>
> - [31:21]: the undervolt value
>
> The second write does:
>
> 0x8010 | (0xf << 40)
> Hex: 0x8f10 Dec: 9.223.388.598.248.669.184
> 31 27 23 19 15 11 7331 27 23 19 15 11
> 73
> 1000______0001_______
> __
> 63 59 55 51 47 43 39 35 31 27 23 19 15 11
> 73
>
> - bit 63: MSR_OC_MAILBOX_BUSY_BIT
> - [47:40] index
> - [39:32] cmd - 0x10
>
> All from only staring at this anyway - could very well be wrong.
>
These command id are model specific. There is no guarantee that even
meaning changes. So I don't think we should write any code in kernel
which can't stick.
> In any case, my point is that we could have a sysfs interface for
> those userspace-suppliable values like the undervolt value at
> [31:21],
> dunno if the index can be inferred by the kernel automatically or
> enumerated and the commands we should issue ourselves depending on
> the
> functionality, etc.
>
> And put all that in drivers/platform/x86/intel_turbo_max_3.c instead
> of
> leaving userspace to poke at it.
>
May be something like this:
- Separate mailbox stuff from intel_turbo_max_3.c
- Create a standalone module which creates a debugfs interface
- This debugs interface takes one 64 bit value from user space and use
protocol to avoid conte
Re: [PATCH 1/2] ACPI: DPTF: Fix participant driver names
On Thu, 2020-10-15 at 18:58 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> Change the names of DPTF participant drivers to adhere to the
> sysfs file naming conventions (no spaces present in the name in
> particular).
>
> Fixes: 2ce6324eadb0 ("ACPI: DPTF: Add PCH FIVR participant driver")
> Fixes: 6256ebd5daf9 ("ACPI / DPTF: Add DPTF power participant
> driver")
> Signed-off-by: Rafael J. Wysocki
Reviewed-by: Srinivas Pandruvada
> ---
> drivers/acpi/dptf/dptf_pch_fivr.c |2 +-
> drivers/acpi/dptf/dptf_power.c|2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/acpi/dptf/dptf_pch_fivr.c
> ===
> --- linux-pm.orig/drivers/acpi/dptf/dptf_pch_fivr.c
> +++ linux-pm/drivers/acpi/dptf/dptf_pch_fivr.c
> @@ -114,7 +114,7 @@ static struct platform_driver pch_fivr_d
> .probe = pch_fivr_add,
> .remove = pch_fivr_remove,
> .driver = {
> - .name = "DPTF PCH FIVR",
> + .name = "dptf_pch_fivr",
> .acpi_match_table = pch_fivr_device_ids,
> },
> };
> Index: linux-pm/drivers/acpi/dptf/dptf_power.c
> ===
> --- linux-pm.orig/drivers/acpi/dptf/dptf_power.c
> +++ linux-pm/drivers/acpi/dptf/dptf_power.c
> @@ -237,7 +237,7 @@ static struct platform_driver dptf_power
> .probe = dptf_power_add,
> .remove = dptf_power_remove,
> .driver = {
> - .name = "DPTF Platform Power",
> + .name = "dptf_power",
> .acpi_match_table = int3407_device_ids,
> },
> };
>
>
>
Re: [PATCH 2/2] ACPI: DPTF: Add ACPI_DPTF Kconfig menu
On Thu, 2020-10-15 at 18:59 +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > Add a Kconfig menu for Intel DPTF (Dynamic Platform and Thermal > Framework), put both the existing participant drivers in it and set > them to be built as modules by default. > > While at it, do a few assorted cleanups for a good measure. > > Signed-off-by: Rafael J. Wysocki Reviewed-by: Srinivas Pandruvada > --- > drivers/acpi/dptf/Kconfig | 29 - > 1 file changed, 24 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/acpi/dptf/Kconfig > === > --- linux-pm.orig/drivers/acpi/dptf/Kconfig > +++ linux-pm/drivers/acpi/dptf/Kconfig > @@ -1,8 +1,25 @@ > # SPDX-License-Identifier: GPL-2.0 > -config DPTF_POWER > - tristate "DPTF Platform Power Participant" > + > +menuconfig ACPI_DPTF > + bool "Intel DPTF (Dynamic Platform and Thermal Framework) > Support" > depends on X86 > help > + Intel Dynamic Platform and Thermal Framework (DPTF) is a > platform > + level hardware/software solution for power and thermal > management. > + > + As a container for multiple power/thermal technologies, DPTF > provides > + a coordinated approach for different policies to effect the > hardware > + state of a system. > + > + For more information see: > + < > https://01.org/intel%C2%AE-dynamic-platform-and-thermal-framework-dptf-chromium-os/overview > > > + > +if ACPI_DPTF > + > +config DPTF_POWER > + tristate "Platform Power DPTF Participant" > + default m > + help > This driver adds support for Dynamic Platform and Thermal > Framework > (DPTF) Platform Power Participant device (INT3407) support. > This participant is responsible for exposing platform > telemetry: > @@ -16,15 +33,17 @@ config DPTF_POWER > the module will be called dptf_power. > > config DPTF_PCH_FIVR > - tristate "DPTF PCH FIVR Participant" > - depends on X86 > + tristate "PCH FIVR DPTF Participant" > + default m > help > This driver adds support for Dynamic Platform and Thermal > Framework > (DPTF) PCH FIVR Participant device support. This driver > allows to > - switch PCH FIVR (Fully Integrated Voltage Regulator) > frequency. > + switch the PCH FIVR (Fully Integrated Voltage Regulator) > frequency. > This participant is responsible for exposing: > freq_mhz_low_clock > freq_mhz_high_clock > > To compile this driver as a module, choose M here: > the module will be called dptf_pch_fivr. > + > +endif > > >
Re: [RFC][PATCH] cpufreq: intel_pstate: Delete intel_pstate sysfs if failed to register the driver
On Fri, 2020-10-09 at 11:30 +0800, Chen Yu wrote:
> There is a corner case that if the intel_pstate driver failed to be
> registered(might be due to invalid MSR access)
Do you have logs why it is not loaded? On supported platforms MSRs
should be invalid.
It may be a case when we are trying to bring up pre-production systems
where some instability in MSRs on certain CPUs.
But the patch is correct. We can't have invalid folder when
intel_pstate is not used.
> and with the acpi_cpufreq
> loaded, the intel_pstate sysfs might still be created, which makes
> the
> user confusing(turbostat for example):
>
> grep . /sys/devices/system/cpu/cpu0/cpufreq/scaling_driver
> acpi-cpufreq
>
> grep . /sys/devices/system/cpu/intel_pstate/*
> /sys/devices/system/cpu/intel_pstate/max_perf_pct:0
> /sys/devices/system/cpu/intel_pstate/min_perf_pct:0
> grep: /sys/devices/system/cpu/intel_pstate/no_turbo: Resource
> temporarily unavailable
> grep: /sys/devices/system/cpu/intel_pstate/num_pstates: Resource
> temporarily unavailable
> /sys/devices/system/cpu/intel_pstate/status:off
> grep: /sys/devices/system/cpu/intel_pstate/turbo_pct: Resource
> temporarily unavailable
>
> The existing of intel_pstate sysfs does not mean that the
> intel_pstate driver
> has been successfully loaded(for example, echo off to status), but
> the
> intel_pstate sysfs should not co-exist when acpi-cpufreq is also
> present.
> Fix this issue by deleting the intel_pstate sysfs if the driver
> failed
> to be loaded during bootup.
>
> Reported-by: Wendy Wang
> Suggested-by: Zhang Rui
> Signed-off-by: Chen Yu
Acked-by: Srinivas Pandruvada --->
> drivers/cpufreq/intel_pstate.c | 24 +++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 9a515c460a00..8c5f9680de83 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1420,6 +1420,26 @@ static void __init
> intel_pstate_sysfs_expose_params(void)
> }
> }
>
> +static void __init intel_pstate_sysfs_clean(void)
> +{
> + if (!intel_pstate_kobject)
> + return;
> +
> + sysfs_remove_group(intel_pstate_kobject,
> _pstate_attr_group);
> +
> + if (per_cpu_limits)
> + goto release_kobj;
> +
> + sysfs_remove_file(intel_pstate_kobject, _perf_pct.attr);
> + sysfs_remove_file(intel_pstate_kobject, _perf_pct.attr);
> +
> + if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids))
> + sysfs_remove_file(intel_pstate_kobject,
> _efficiency.attr);
> +
> +release_kobj:
> + kobject_put(intel_pstate_kobject);
> +}
> +
> static void intel_pstate_sysfs_expose_hwp_dynamic_boost(void)
> {
> int rc;
> @@ -3063,8 +3083,10 @@ static int __init intel_pstate_init(void)
> mutex_lock(_pstate_driver_lock);
> rc = intel_pstate_register_driver(default_driver);
> mutex_unlock(_pstate_driver_lock);
> - if (rc)
> + if (rc) {
> + intel_pstate_sysfs_clean();
> return rc;
> + }
>
> if (hwp_active) {
> const struct x86_cpu_id *id;
[PATCH] ACPI / DPTF: Add PCH FIVR participant driver
This driver adds support for Dynamic Platform and Thermal Framework
(DPTF) PCH (Platform Controller Hub) FIVR (Fully Integrated Voltage
Regulator) participant support.
This participant is responsible for exposing platform telemetry and
control for:
freq_mhz_high_clock
freq_mhz_low_clock
These attributes are used to get and set PCH FIVR switching frequency
for thermal and radio frequency interference mitigation.
Refer to Documentation/ABI/testing/sysfs-platform-dptf for ABI details.
ACPI methods description used in this driver:
RFC0: This ACPI method to set PCH FIVR switching frequency when FIVR
clock is 19.2MHz or 24MHz. The ACPI method takes a DWORD value.
GFC0: This ACPI method to get PCH FIVR switching frequency when FIVR
clock is 19.2MHz or 24MHz.
RFC1: This ACPI method to set PCH FIVR switching frequency when FIVR
clock is 38.4MHz. The ACPI method takes a DWORD value.
GFC1: This ACPI method to get PCH FIVR switching frequency when FIVR
clock is 38.4MHz.
Signed-off-by: Srinivas Pandruvada
---
Documentation/ABI/testing/sysfs-platform-dptf | 16 +++
drivers/acpi/dptf/Kconfig | 14 ++
drivers/acpi/dptf/Makefile| 1 +
drivers/acpi/dptf/dptf_pch_fivr.c | 126 ++
drivers/acpi/dptf/int340x_thermal.c | 1 +
5 files changed, 158 insertions(+)
create mode 100644 drivers/acpi/dptf/dptf_pch_fivr.c
diff --git a/Documentation/ABI/testing/sysfs-platform-dptf
b/Documentation/ABI/testing/sysfs-platform-dptf
index eeed81ca6949..2cbc660d163b 100644
--- a/Documentation/ABI/testing/sysfs-platform-dptf
+++ b/Documentation/ABI/testing/sysfs-platform-dptf
@@ -92,3 +92,19 @@ Contact: linux-a...@vger.kernel.org
Description:
(RO) The battery discharge current capability obtained from
battery fuel gauge in
milli Amps.
+
+What:
/sys/bus/platform/devices/INTC1045:00/pch_fivr_switch_frequency/freq_mhz_low_clock
+Date: November, 2020
+KernelVersion: v5.10
+Contact: linux-a...@vger.kernel.org
+Description:
+ (RW) The PCH FIVR (Fully Integrated Voltage Regulator)
switching frequency in MHz,
+ when FIVR clock is 19.2MHz or 24MHz.
+
+What:
/sys/bus/platform/devices/INTC1045:00/pch_fivr_switch_frequency/freq_mhz_high_clock
+Date: November, 2020
+KernelVersion: v5.10
+Contact: linux-a...@vger.kernel.org
+Description:
+ (RW) The PCH FIVR (Fully Integrated Voltage Regulator)
switching frequency in MHz,
+ when FIVR clock is 38.4MHz.
diff --git a/drivers/acpi/dptf/Kconfig b/drivers/acpi/dptf/Kconfig
index 90a2fd979282..51f06f36cafa 100644
--- a/drivers/acpi/dptf/Kconfig
+++ b/drivers/acpi/dptf/Kconfig
@@ -14,3 +14,17 @@ config DPTF_POWER
To compile this driver as a module, choose M here:
the module will be called dptf_power.
+
+config DPTF_PCH_FIVR
+ tristate "DPTF PCH FIVR Participant"
+ depends on X86
+ help
+ This driver adds support for Dynamic Platform and Thermal Framework
+ (DPTF) PCH FIVR Participant device support. This driver allows to
+ switch PCH FIVR (Fully Integrated Voltage Regulator) frequency.
+ This participant is responsible for exposing:
+ freq_mhz_low_clock
+ freq_mhz_high_clock
+
+ To compile this driver as a module, choose M here:
+ the module will be called dptf_pch_fivr.
diff --git a/drivers/acpi/dptf/Makefile b/drivers/acpi/dptf/Makefile
index 1a9b0a2b25bf..297340682f66 100644
--- a/drivers/acpi/dptf/Makefile
+++ b/drivers/acpi/dptf/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_ACPI) += int340x_thermal.o
obj-$(CONFIG_DPTF_POWER) += dptf_power.o
+obj-$(CONFIG_DPTF_PCH_FIVR)+= dptf_pch_fivr.o
diff --git a/drivers/acpi/dptf/dptf_pch_fivr.c
b/drivers/acpi/dptf/dptf_pch_fivr.c
new file mode 100644
index ..4ab288827747
--- /dev/null
+++ b/drivers/acpi/dptf/dptf_pch_fivr.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * dptf_pch_fivr: DPTF PCH FIVR Participant driver
+ * Copyright (c) 2020, Intel Corporation.
+ */
+
+#include
+#include
+#include
+#include
+
+/*
+ * Presentation of attributes which are defined for INT1045
+ * They are:
+ * freq_mhz_low_clock : Set PCH FIVR switching freq for
+ * FIVR clock 19.2MHz and 24MHz
+ * freq_mhz_high_clock : Set PCH FIVR switching freq for
+ * FIVR clock 38.4MHz
+ */
+#define PCH_FIVR_SHOW(name, method) \
+static ssize_t name##_show(struct device *dev,\
+ struct device_attribute *attr,\
+ char *buf)\
+{\
+ struct acpi_device *acpi_dev = dev_get_drvdata(dev);\
+ unsigned long long val;\
+ acpi_status status;\
+\
+ status = acpi_evaluate_integer(acpi_dev->handle, #method,\
+
[PATCH 3/3] thermal: int340x: Add keep alive response method
When firmware requests keep alive response, send an event to user space
to confirm by using imok sysfs entry.
Create a new sysf entry called "imok". User space can write an integer,
which results in execution of IMOK ACPI method of INT3400 thermal zone
device. This results in sending response to firmware request for keep
alive.
Signed-off-by: Srinivas Pandruvada
---
.../intel/int340x_thermal/int3400_thermal.c | 29 +++
1 file changed, 29 insertions(+)
diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 2ca295e6e64d..0966551cbaaa 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -14,6 +14,7 @@
#define INT3400_THERMAL_TABLE_CHANGED 0x83
#define INT3400_ODVP_CHANGED 0x88
+#define INT3400_KEEP_ALIVE 0xA0
enum int3400_thermal_uuid {
INT3400_THERMAL_PASSIVE_1,
@@ -83,8 +84,33 @@ static struct bin_attribute *data_attributes[] = {
NULL,
};
+static ssize_t imok_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct int3400_thermal_priv *priv = dev_get_drvdata(dev);
+ acpi_status status;
+ int input, ret;
+
+ ret = kstrtouint(buf, 10, );
+ if (ret)
+ return ret;
+ status = acpi_execute_simple_method(priv->adev->handle, "IMOK", input);
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ return count;
+}
+
+static DEVICE_ATTR_WO(imok);
+
+static struct attribute *imok_attr[] = {
+ _attr_imok.attr,
+ NULL
+};
+
static const struct attribute_group data_attribute_group = {
.bin_attrs = data_attributes,
+ .attrs = imok_attr,
};
static ssize_t available_uuids_show(struct device *dev,
@@ -358,6 +384,9 @@ static void int3400_notify(acpi_handle handle,
case INT3400_THERMAL_TABLE_CHANGED:
therm_event = THERMAL_TABLE_CHANGED;
break;
+ case INT3400_KEEP_ALIVE:
+ therm_event = THERMAL_EVENT_KEEP_ALIVE;
+ break;
case INT3400_ODVP_CHANGED:
evaluate_odvp(priv);
therm_event = THERMAL_DEVICE_POWER_CAPABILITY_CHANGED;
--
2.25.4
[PATCH 1/3] thermal: int340x: Provide notification for OEM variable change
When we receive ACPI notification for OEM variable change pass the
notification to user space handler. This will avoid polling for
OEM variable change from user space.
Signed-off-by: Srinivas Pandruvada
---
.../intel/int340x_thermal/int3400_thermal.c | 22 +--
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 4f5859d4c780..2ca295e6e64d 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -349,30 +349,30 @@ static void int3400_notify(acpi_handle handle,
{
struct int3400_thermal_priv *priv = data;
char *thermal_prop[5];
+ int therm_event;
if (!priv)
return;
switch (event) {
case INT3400_THERMAL_TABLE_CHANGED:
- thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s",
- priv->thermal->type);
- thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d",
- priv->thermal->temperature);
- thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=");
- thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d",
- THERMAL_TABLE_CHANGED);
- thermal_prop[4] = NULL;
- kobject_uevent_env(>thermal->device.kobj, KOBJ_CHANGE,
- thermal_prop);
+ therm_event = THERMAL_TABLE_CHANGED;
break;
case INT3400_ODVP_CHANGED:
evaluate_odvp(priv);
+ therm_event = THERMAL_DEVICE_POWER_CAPABILITY_CHANGED;
break;
default:
/* Ignore unknown notification codes sent to INT3400 device */
- break;
+ return;
}
+
+ thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", priv->thermal->type);
+ thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d",
priv->thermal->temperature);
+ thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=");
+ thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", therm_event);
+ thermal_prop[4] = NULL;
+ kobject_uevent_env(>thermal->device.kobj, KOBJ_CHANGE,
thermal_prop);
}
static int int3400_thermal_get_temp(struct thermal_zone_device *thermal,
--
2.25.4
[PATCH 2/3] thermal: core: Add new event for sending keep alive notifications
This event is sent by the platform firmware to confirm that
user space thermal solution is alive. The response to this event
from the user space thermal solution is platform specific.
Signed-off-by: Srinivas Pandruvada
---
include/linux/thermal.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 42ef807e5d84..42b69d4072a4 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -55,6 +55,7 @@ enum thermal_notify_event {
THERMAL_DEVICE_UP, /* Thermal device is up after a down event */
THERMAL_DEVICE_POWER_CAPABILITY_CHANGED, /* power capability changed */
THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
+ THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond
*/
};
struct thermal_zone_device_ops {
--
2.25.4
thermal: int340x: New notifications for INT340X thermal
This series contains notifications to user space using currently used mechanism in these drivers. Srinivas Pandruvada (3): thermal: int340x: Provide notification for OEM variable change thermal: core: Add new event for sending keep alive notifications thermal: int340x: Add keep alive response method .../intel/int340x_thermal/int3400_thermal.c | 51 +++ include/linux/thermal.h | 1 + 2 files changed, 41 insertions(+), 11 deletions(-) -- 2.25.4
Re: [PATCH] x86/msr: do not warn on writes to OC_MAILBOX
On Tue, 2020-09-08 at 21:30 +0200, Borislav Petkov wrote: > On Tue, Sep 08, 2020 at 12:18:38PM -0700, Sultan Alsawaf wrote: > > I'd like to point out that on Intel's recent 14nm parts, > > undervolting > > is not so much for squeezing every last drop of performance out of > > the > > SoC as it is for necessity. > > > > Sounds to me that this undervolting functionality should be part of > the kernel and happen automatically. I have no clue, though, whether > people who do it, just get lucky and undervolting doesn't cause any > other hardware issues, or there's a real reason for this power > madness > and if not done, power-related failures happen only on some boxes so > they decided to do them on all. > > Or maybe BIOS is nuts, which is not a stretch. > The whole OC is based on experiments to come to correct values. This depends on whole system design not just CPUs. https://www.intel.com/content/www/us/en/gaming/resources/how-to-overclock.html It warns about system stability. > Srinivas, what's the story here? I checked and there is no public spec. There are several mailbox commands with version dependent on the processor. The actual OC mailbox implementation itself is implemented in Linux in intel_turbo_max_3 driver. So that is public. So someone can develop a driver and provide some sysfs to send mailbox commands, but kernel can't validate commands which can cause any security or stability issues. Not sure if this is acceptable standard. I don't think there is any precedent of creating such blind sysfs entries. Thanks, Srinivas
Re: [PATCH] x86/msr: do not warn on writes to OC_MAILBOX
On Mon, 2020-09-07 at 12:06 +0200, Borislav Petkov wrote:
> + Srinivas.
> + kitsunyan.
>
> On Mon, Sep 07, 2020 at 11:48:43AM +0200, Jason A. Donenfeld wrote:
> > Popular tools, like intel-undervolt, use MSR 0x150 to control the
> > CPU
> > voltage offset. In fact, evidently the intel_turbo_max_3 driver in-
> > tree
> > also uses this MSR. So, teach the kernel's MSR list about this, so
> > that
> > intel-undervolt and other such tools don't spew warnings to dmesg,
> > while
> > unifying the constant used throughout the kernel.
> >
[...]
> > - if (reg == MSR_IA32_ENERGY_PERF_BIAS)
> > + switch (reg) {
> > + case MSR_IA32_ENERGY_PERF_BIAS:
There is already sysfs interface for it.
> > + case MSR_IA32_OC_MAILBOX:
> > return 0;
> > + }
> >
[...]
> Actually, we added the filtering to catch exactly such misuses and,
> lemme check what is the proper word now... /me checks, aha, adding
> new
> MSRs to the "passlist" is the wrong thing to do.
>
> Srinivas, can you pls convert this in-tree driver to use a proper
> sysfs
> interface for that mailbox MSR and also work with the intel-undervolt
> author - I hope I have the right person CCed from the git repo on
> github
> - to come up with a proper interface so that we can drop this MSR use
> too.
Overclocking is not architectural I/F and is supported by some special
CPU skews. I can't find any public document to specify the commands
which can be used via this OC mailbox. I have to check internally to
see if there is any. To add a proper sysfs interface we have to make
sure that we are not allowing some random commands to hardware and
crash the system.
Thanks,
Srinivas
Re: [PATCH v4 0/5] cpufreq: intel_pstate: Address some HWP-related oddities
On Tue, 2020-09-01 at 19:23 +0200, Rafael J. Wysocki wrote: > Hi All, > > The last two patches in the v3 needed to be updated to take re- > enabling of HWP > after an ACPI S3 suspend/resume cycle into account > appropriately. The first > three patches are the same as before. > > The purpose of this series is to address some peculiarities related > to > taking CPUs offline/online and switching between different operation > modes with HWP enabled that have become visible after allowing the > driver to work in the passive mode with HWP enabled in 5.9-rc1 (and > one that was there earlier, but can be addressed easily after the > changes made in 5.9-rc1). > > Please refer to the patch changelogs for details. > > For easier testing/review, the series is available from the git > branch at: > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \ > intel_pstate-testing > > I've done my best to address all of the possible corner cases, but > the test > matrix is quite extensive and I may have missed something, so go > ahead > and test. Acked-by: Srinivas Pandruvada > > Thanks, > Rafael > > >
Re: [PATCH v2 2/5] cpufreq: intel_pstate: Always return last EPP value from sysfs
On Tue, 2020-08-25 at 17:14 +0200, Rafael J. Wysocki wrote: > On Tue, Aug 25, 2020 at 5:06 PM Srinivas Pandruvada > wrote: > > On Tue, 2020-08-25 at 16:51 +0200, Rafael J. Wysocki wrote: > > > On Tue, Aug 25, 2020 at 8:20 AM Artem Bityutskiy < > > > dedeki...@gmail.com > > > > wrote: > > > > On Mon, 2020-08-24 at 19:42 +0200, Rafael J. Wysocki wrote: > > > > > From: "Rafael J. Wysocki" > > > > > > > > > > Make the energy_performance_preference policy attribute in > > > > > sysfs > > > > > always return the last EPP value written to it instead of the > > > > > one > > > > > currently in the HWP Request MSR to avoid possible confusion > > > > > when > > > > > the performance scaling algorithm is used in the active mode > > > > > with > > > > > HWP enabled (in which case the EPP is forced to 0 regardless > > > > > of > > > > > what value it has been set to via sysfs). > > > > > > > > Why is this a good idea, I wonder. If there was a prior > > > > discussion, > > > > please, point to it. > > > > > > > > The general approach to changing settings via sysfs is often > > > > like > > > > this: > > > > > > > > 1. Write new value. > > > > 2. Read it back and verify that it is the same. Because there > > > > is no > > > > better way to verify that the kernel "accepted" the value. > > > > > > If the write is successful (ie. no errors returned and the value > > > returned is equal to the number of written characters), the > > > kernel > > > *has* accepted the written value, but it may not have taken > > > effect. > > > These are two different things. > > > > > > The written value may take an effect immediately or it may take > > > an > > > effect later, depending on the current configuration etc. If you > > > don't see the effect of it immediately, it doesn't matter that > > > there > > > was a failure of some sort. > > > > > > > Let's say I write 'balanced' to energy_performance_preference. > > > > I > > > > read > > > > it back, and it contains 'balanced', so I am happy, I trust the > > > > kernel > > > > changed EPP to "balanced". > > > > > > > > If the kernel, in fact, uses something else, I want to know > > > > about > > > > it > > > > and have my script fail. > > > > > > Why do you want it to fail then? > > > > > > > Why caching the value and making my script _think_ it succeeded > > > > is > > > > a good idea. > > > > > > Because when you change the scaling algorithm or the driver's > > > operation mode, the value you have written will take effect. > > > > > > In this particular case it is explained in the driver > > > documentation > > > that the performance scaling algorithm in the active mode > > > overrides > > > the sysfs value and that's the only case when it can be > > > overridden. > > > So whatever you write to this attribute will not take effect > > > immediately anyway, but it may take an effect later. > > > > In some cases without even changing active/passive this is > > happening > > when there was some error previously. For example: > > > > #cat energy_performance_preference > > 127 > > [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -p 1 0x774 > > 8000ff00 > > > > I think we should show reality. In mode change can be a special > > case > > and use the stored value to restore in new mode. > > OK, so I'll make it fail on attempts to change the EPP from 0 > (performance) in the active mode with the performance "governor". > Here the scaling governor is powersave. # cat scaling_governor powersave Thanks, Srinivas > Cheers!
Re: [PATCH v2 0/5] cpufreq: intel_pstate: Address some HWP-related oddities
Hi Doug, On Mon, 2020-08-24 at 18:00 -0700, Doug Smythies wrote: > Hi Srinivas, > > I think there is a disconnect between your written > description of what is going on and your supporting MSR reads. > I reproduced again. I see the copy paste individual at the first place swapped. I pasted the full output by direct copy - paste from the screen. But the issues are still there. [labuser@otcpl-perf-test-skx-i9 ~]$ sudo -s [root@otcpl-perf-test-skx-i9 labuser]# rdmsr -a 0x774 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002b0c 80002b0c 80002d0d [root@otcpl-perf-test-skx-i9 labuser]# cd /sys/devices/system/cpu/cpu1 [root@otcpl-perf-test-skx-i9 cpu1]# cd cpufreq/ [root@otcpl-perf-test-skx-i9 cpufreq]# echo 127 > energy_performance_preference [root@otcpl-perf-test-skx-i9 cpufreq]# cat energy_performance_preference 127 [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -a 0x774 80002b0c 7f002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002b0c 80002b0c 80002d0d [root@otcpl-perf-test-skx-i9 cpufreq]# echo 0 > /sys/devices/system/cpu/cpu1/online [root@otcpl-perf-test-skx-i9 cpufreq]# echo 1 > /sys/devices/system/cpu/cpu1/online [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -a 0x774 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002d0c 80002b0c 80002b0c 80002b0c 80002b0c 80002b0c 80002d0d 7f002b0c [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -p 1 0x774 7f002b0c [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -p 19 0x774 80002d0d [root@otcpl-perf-test-skx-i9 cpufreq]# rtcwake -m mem -s 10 rtcwake: wakeup from "mem" using /dev/rtc0 at Tue Aug 25 15:04:02 2020 [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -a 0x774 80002b0c 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 8000ff00 [root@otcpl-perf-test-skx-i9 cpufreq]# dmesg > ~/temp/dmesg.txt [root@otcpl-perf-test-skx-i9 cpufreq]# cat energy_performance_preference 127 [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -p 1 0x774 8000ff00 [root@otcpl-perf-test-skx-i9 cpufreq]# echo passive > /sys/devices/system/ clockevents/ clocksource/ container/cpu/ edac/ m achinecheck/ memory/ node/ [root@otcpl-perf-test-skx-i9 cpufreq]# echo passive > /sys/devices/system/cpu/intel_pstate/status [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -a 0x774 80002b0c 7f00 8000 80002d0c 80002b0c 8000 8000 80002b0c 8000 80002d0c 8000 80002b0c 80002b0d 80002d0c 8000 8000 8000 80002b0d 8000 8000 [root@otcpl-perf-test-skx-i9 cpufreq]# rdmsr -a 0x774 80002b0c 7f00 8000 80002d0c 80002b0c 8000 8000 80002b0c 8000 80002d0c 8000 80002b0c 80002b0d 80002d0c 8000 80002b0c 8000 80002b0d 8000 8000 > On 2020.08.24 16:56 Srinivas Pandruvada wrote: > > On Mon, 2020-08-24 at 19:39 +0200, Rafael J. Wysocki wrote: > > > Hi All, > > > > > > The v2 is here to address feedback from Doug and one issue found > > > by > > > me. > > > > > > The purpose of this series is to address some peculiarities > > > related > > > to > > > taking CPUs offline/online and switching between different > > > operation > > > modes with HWP enabled that have become visible after allowing > > > the > > > driver to work in the passive mode with HWP enabled in 5.9-rc1 > > > (and > > > one that was there earlier, but can be addressed easily after the > > > changes madein 5.9-rc1). > > > > > > Please refer to the patch changelogs for details. > > > > > > For easier testing/review, the series is available from the git > > > branch at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux- > > > pm.git \ > > > intel_pstate-testing > > > > > > > Applied these patches to 5.9-rc2 > > So did I, and the issues I reported the other day are fine now. > I did try a few of the things you were doing. > > > - After s3 limits got messed up. > > # cat /sys/power/mem_sleep > > s2idle [deep] > > > > - In the dmesg unchecked MSR for HWP register > > > > 1. > > Before test > > > > sudo rdmsr -a 0x774 > > 80002b0c > > 80002b0c > > 80002d0c > > 80002b0c > > 80002b0c > > 80002b0c > > 80002b0c > > 80002b0c > > 80002d0c > > 80002b0c > > 80002b0c > > 80002b0c > > 80002d0c > > 80002b0c >
