linux-kernel - Re: [PATCH v1 6/6] ACPI: thermal: processor: Use the generic cpufreq infrastructure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 2 Jun 2014 13:36:02 -0400
From:	Eduardo Valentin <edubezval@...il.com>
To:	Javi Merino <javi.merino@....com>
Cc:	Amit Kachhap <amit.kachhap@...il.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	Zhang Rui <rui.zhang@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rjw@...ysocki.net" <rjw@...ysocki.net>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"lenb@...nel.org" <lenb@...nel.org>
Subject: Re: [PATCH v1 6/6] ACPI: thermal: processor: Use the generic cpufreq
 infrastructure

Hello Amit, Javi,

On Mon, Jun 02, 2014 at 11:20:48AM +0100, Javi Merino wrote:
> On Mon, Jun 02, 2014 at 10:21:48AM +0100, Amit Kachhap wrote:
> > Hi Javi,
> > 
> > On 5/29/14, Javi Merino <javi.merino@....com> wrote:
> > > Hi Amit,
> > >
> > > On Thu, May 29, 2014 at 09:15:34AM +0100, Amit Daniel Kachhap wrote:
> > >> This patch upgrades the ACPI cpufreq cooling portions to use the generic
> > >> cpufreq cooling infrastructure. There should not be any functionality
> > >> related changes as the same behaviour is provided by the generic
> > >> cpufreq APIs with the notifier mechanism.
> > >>
> > >> Signed-off-by: Amit Daniel Kachhap <amit.daniel@...sung.com>
> > >> ---
> > >>  drivers/acpi/processor_driver.c  |    6 +-
> > >>  drivers/acpi/processor_thermal.c |  235
> > >> ++++++++++++++++++--------------------
> > >>  include/acpi/processor.h         |    3 +-
> > >>  3 files changed, 115 insertions(+), 129 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/processor_driver.c
> > >> b/drivers/acpi/processor_driver.c
> > >> index 7f70f31..10aba4a 100644
> > >> --- a/drivers/acpi/processor_driver.c
> > >> +++ b/drivers/acpi/processor_driver.c
> > >> @@ -36,6 +36,7 @@
> > >>  #include <linux/cpuidle.h>
> > >>  #include <linux/slab.h>
> > >>  #include <linux/acpi.h>
> > >> +#include <linux/cpu_cooling.h>
> > >>
> > >>  #include <acpi/processor.h>
> > >>
> > >> @@ -178,8 +179,7 @@ static int __acpi_processor_start(struct acpi_device
> > >> *device)
> > >>         if (!cpuidle_get_driver() || cpuidle_get_driver() ==
> > >> &acpi_idle_driver)
> > >>                 acpi_processor_power_init(pr);
> > >>
> > >> -       pr->cdev = thermal_cooling_device_register("Processor", device,
> > >> -
> > >> &processor_cooling_ops);
> > >> +       pr->cdev = acpi_processor_cooling_register(device);
> > >
> > > With this you have removed the only cooling device whose type was
> > > "Processor".  There's special code for dealing with this cooling
> > > device in drivers/thermal/thermal_core.c:passive_store():
> > >
> > > 		list_for_each_entry(cdev, &thermal_cdev_list, node) {
> > > 			if (!strncmp("Processor", cdev->type,
> > > 				     sizeof("Processor")))
> > > 				thermal_zone_bind_cooling_device(tz,
> > > 						THERMAL_TRIPS_NONE, cdev,
> > > 						THERMAL_NO_LIMIT,
> > > 						THERMAL_NO_LIMIT);
> > > 		}
> > > 		mutex_unlock(&thermal_list_lock);
> > > 		if (!tz->passive_delay)
> > >
> > > With your change, that code is now "dead" as it can't do anything.  No
> > > I don't know what should you do with it, either remove it or make it
> > > match the cpufreq cooling device.  But this patch should deal with
> > > that code as well.
> > nice catch. I somehow missed modifying this section.
> > I think the following changes should fix this,
> > -                       if (!strncmp("Processor", cdev->type,
> > -                                    sizeof("Processor")))
> > +                       if (!strncmp("thermal-cpufreq", cdev->type,
> > +                                    sizeof("thermal-cpufreq")))
> >                                 thermal_zone_bind_cooling_device(tz,
> > 
> 
> That should do it.  I don't really understand why this code is
> specifically looking for ACPI processor cooling devices but I guess
> that's the least disrupting change you can make.

Well, I suggest we move slightly carefuly here. The problem is that this
change actually breaks ABI. If so, we need to follow the kernel ABI
change rules. We should never break userspace.

Rui, Do you recall what users are aware of this sysfs entry?

Cheers,

> 
> Cheers,
> Javi
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/