[PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[Eduardo Valentin]

Currently, in an ACPI based system, the processor driver registers
one cooling device per processor. However, the cooling device type
is the same for each processor. For example, on a system with four
processors, the sysfs reading of each cooling device would look like:
ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
Processor
Processor
Processor
Processor

which turns out to fine. But, some parts of the thermal code may use
type to identify participating devices in a thermal zone. Besides,
adding notifications to user space may cause the production of messages
that may confuse the listener.

For this reason, this patch adds the processor ID cooling device type.
After this change, the cooling device listing in the same previous example
would look like this:
ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
Processor.0
Processor.1
Processor.2
Processor.3

allowing an easier identification of cooling device target.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/acpi/processor_driver.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index d2fa8cb..6e982c1 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -163,6 +163,7 @@ static struct notifier_block acpi_cpu_notifier = {
 static int acpi_pss_perf_init(struct acpi_processor *pr,
 		struct acpi_device *device)
 {
+	char cdev_name[THERMAL_NAME_LENGTH];
 	int result = 0;
 
 	acpi_processor_ppc_has_changed(pr, 0);
@@ -172,7 +173,8 @@ static int acpi_pss_perf_init(struct acpi_processor *pr,
 	if (pr->flags.throttling)
 		pr->flags.limit = 1;
 
-	pr->cdev = thermal_cooling_device_register("Processor", device,
+	snprintf(cdev_name, sizeof(cdev_name), "Processor.%d", pr->id);
+	pr->cdev = thermal_cooling_device_register(cdev_name, device,
 						   &processor_cooling_ops);
 	if (IS_ERR(pr->cdev)) {
 		result = PTR_ERR(pr->cdev);
-- 
2.1.4

Re: [PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[Rafael J. Wysocki]

On Tue, May 3, 2016 at 7:04 AM, Eduardo Valentin <edubezval@gmail.com> wrote:
> Currently, in an ACPI based system, the processor driver registers
> one cooling device per processor. However, the cooling device type
> is the same for each processor. For example, on a system with four
> processors, the sysfs reading of each cooling device would look like:
> ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> Processor
> Processor
> Processor
> Processor
>
> which turns out to fine. But, some parts of the thermal code may use
> type to identify participating devices in a thermal zone. Besides,
> adding notifications to user space may cause the production of messages
> that may confuse the listener.
>
> For this reason, this patch adds the processor ID cooling device type.
> After this change, the cooling device listing in the same previous example
> would look like this:
> ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> Processor.0
> Processor.1
> Processor.2
> Processor.3
>
> allowing an easier identification of cooling device target.

Is it not going to confuse any user space scripts or similar?

Re: [PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[Srinivas Pandruvada]

On Wed, 2016-05-04 at 23:49 +0200, Rafael J. Wysocki wrote:
> On Tue, May 3, 2016 at 7:04 AM, Eduardo Valentin <edubezval@gmail.com
> > wrote:
> > 
> > Currently, in an ACPI based system, the processor driver registers
> > one cooling device per processor. However, the cooling device type
> > is the same for each processor. For example, on a system with four
> > processors, the sysfs reading of each cooling device would look
> > like:
> > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > Processor
> > Processor
> > Processor
> > Processor
> > 
> > which turns out to fine. But, some parts of the thermal code may
> > use
> > type to identify participating devices in a thermal zone. Besides,
> > adding notifications to user space may cause the production of
> > messages
> > that may confuse the listener.
> > 
> > For this reason, this patch adds the processor ID cooling device
> > type.
> > After this change, the cooling device listing in the same previous
> > example
> > would look like this:
> > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > Processor.0
> > Processor.1
> > Processor.2
> > Processor.3
> > 
> > allowing an easier identification of cooling device target.
> Is it not going to confuse any user space scripts or similar?
Yes, it will.

Thanks,
Srinivas

Re: [PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[Rafael J. Wysocki]

On Wednesday, May 04, 2016 02:54:32 PM Srinivas Pandruvada wrote:
> On Wed, 2016-05-04 at 23:49 +0200, Rafael J. Wysocki wrote:
> > On Tue, May 3, 2016 at 7:04 AM, Eduardo Valentin <edubezval@gmail.com
> > > wrote:
> > > 
> > > Currently, in an ACPI based system, the processor driver registers
> > > one cooling device per processor. However, the cooling device type
> > > is the same for each processor. For example, on a system with four
> > > processors, the sysfs reading of each cooling device would look
> > > like:
> > > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > > Processor
> > > Processor
> > > Processor
> > > Processor
> > > 
> > > which turns out to fine. But, some parts of the thermal code may
> > > use
> > > type to identify participating devices in a thermal zone. Besides,
> > > adding notifications to user space may cause the production of
> > > messages
> > > that may confuse the listener.
> > > 
> > > For this reason, this patch adds the processor ID cooling device
> > > type.
> > > After this change, the cooling device listing in the same previous
> > > example
> > > would look like this:
> > > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > > Processor.0
> > > Processor.1
> > > Processor.2
> > > Processor.3
> > > 
> > > allowing an easier identification of cooling device target.
> > 
> > Is it not going to confuse any user space scripts or similar?
>
> Yes, it will.

In that case the patch cannot be applied.

Thanks,
Rafael

Re: [PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[Eduardo Valentin]

On Thu, May 05, 2016 at 12:00:57AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, May 04, 2016 02:54:32 PM Srinivas Pandruvada wrote:
> > On Wed, 2016-05-04 at 23:49 +0200, Rafael J. Wysocki wrote:
> > > On Tue, May 3, 2016 at 7:04 AM, Eduardo Valentin <edubezval@gmail.com
> > > > wrote:
> > > > 
> > > > Currently, in an ACPI based system, the processor driver registers
> > > > one cooling device per processor. However, the cooling device type
> > > > is the same for each processor. For example, on a system with four
> > > > processors, the sysfs reading of each cooling device would look
> > > > like:
> > > > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > > > Processor
> > > > Processor
> > > > Processor
> > > > Processor
> > > > 
> > > > which turns out to fine. But, some parts of the thermal code may
> > > > use
> > > > type to identify participating devices in a thermal zone. Besides,
> > > > adding notifications to user space may cause the production of
> > > > messages
> > > > that may confuse the listener.
> > > > 
> > > > For this reason, this patch adds the processor ID cooling device
> > > > type.
> > > > After this change, the cooling device listing in the same previous
> > > > example
> > > > would look like this:
> > > > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > > > Processor.0
> > > > Processor.1
> > > > Processor.2
> > > > Processor.3
> > > > 
> > > > allowing an easier identification of cooling device target.
> > > 
> > > Is it not going to confuse any user space scripts or similar?
> >
> > Yes, it will.
> 
> In that case the patch cannot be applied.

In fact, we shall never brake userspace. 

Srinivas, could you please elaborate a bit more on how this would break
userspace? How different would it be having an extra id? Are you
expecting "Processor" string in daemon?

BR,

> 
> Thanks,
> Rafael
> 

Re: [PATCH 1/1] drivers: acpi: add CPU id to cooling device type of processor driver

[Srinivas Pandruvada]

Hi Eduardo,

On Wed, 2016-05-04 at 20:06 -0700, Eduardo Valentin wrote:
> On Thu, May 05, 2016 at 12:00:57AM +0200, Rafael J. Wysocki wrote:
> > 
> > On Wednesday, May 04, 2016 02:54:32 PM Srinivas Pandruvada wrote:
> > > 
> > > On Wed, 2016-05-04 at 23:49 +0200, Rafael J. Wysocki wrote:
> > > > 
> > > > On Tue, May 3, 2016 at 7:04 AM, Eduardo Valentin <edubezval@gma
> > > > il.com
> > > > > 
> > > > > wrote:
> > > > > 
> > > > > Currently, in an ACPI based system, the processor driver
> > > > > registers
> > > > > one cooling device per processor. However, the cooling device
> > > > > type
> > > > > is the same for each processor. For example, on a system with
> > > > > four
> > > > > processors, the sysfs reading of each cooling device would
> > > > > look
> > > > > like:
> > > > > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > > > > Processor
> > > > > Processor
> > > > > Processor
> > > > > Processor
> > > > > 
> > > > > which turns out to fine. But, some parts of the thermal code
> > > > > may
> > > > > use
> > > > > type to identify participating devices in a thermal zone.
> > > > > Besides,
> > > > > adding notifications to user space may cause the production
> > > > > of
> > > > > messages
> > > > > that may confuse the listener.
> > > > > 
> > > > > For this reason, this patch adds the processor ID cooling
> > > > > device
> > > > > type.
> > > > > After this change, the cooling device listing in the same
> > > > > previous
> > > > > example
> > > > > would look like this:
> > > > > ebv@besouro ~ $ cat /sys/class/thermal/cooling_device*/type
> > > > > Processor.0
> > > > > Processor.1
> > > > > Processor.2
> > > > > Processor.3
> > > > > 
> > > > > allowing an easier identification of cooling device target.
> > > > Is it not going to confuse any user space scripts or similar?
> > > Yes, it will.
> > In that case the patch cannot be applied.
> In fact, we shall never brake userspace. 
> 
> Srinivas, could you please elaborate a bit more on how this would
> break
> userspace? How different would it be having an extra id? Are you
> expecting "Processor" string in daemon?
Yes.
Also Processor is a special type of cdev on client systems. So you set
a state in one Processor all Processors cooling device gets changed as
they share voltage and frequency. So they all are same. So if some user
space tries to reduce the cur_state by 1 for all the processor cooling
devices they will effectively reduce cur_state to much lower value.

So instead of this, we should have only one processor cooling device
per CPU package, so on most of the client systems, we will see only one
processor cooling device.

Thanks,
Srinivas

> 
> BR,
> 
> > 
> > 
> > Thanks,
> > Rafael
> >