From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751632AbcEQRS5 (ORCPT ); Tue, 17 May 2016 13:18:57 -0400 Received: from mga02.intel.com ([134.134.136.20]:18110 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751557AbcEQRSz (ORCPT ); Tue, 17 May 2016 13:18:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,324,1459839600"; d="scan'208";a="808787351" Message-ID: <1463505584.28729.198.camel@linux.intel.com> Subject: Re: [RFC PATCH 1/1] acpi: processor_driver: register cooling device per package From: Srinivas Pandruvada To: Eduardo Valentin , Rui Zhang Cc: Linux PM , LKML , rjw@rjwysocki.net Date: Tue, 17 May 2016 10:19:44 -0700 In-Reply-To: <1463120370-2553-1-git-send-email-edubezval@gmail.com> References: <1463120370-2553-1-git-send-email-edubezval@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2 (3.18.5.2-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-05-12 at 23:19 -0700, Eduardo Valentin wrote: > The current throttling strategy is to apply cooling levels > on all processors in a package. Therefore, if one cooling > device is requested to cap the frequency, the same request > is applied to all sibling processors in the same package. > > For this reason, this patch removes the redundant cooling > devices, registering only one cooling device per package. > > Signed-off-by: Eduardo Valentin > --- > Srinivas, > > This is the outcome of our discussion on the Processor cooling > device. > As per your suggestion, I am now attempting to register a single > cooling device per package. > > The only thing is about the sysfs links. Now the Processor cooling > device would look like: > $ ls /sys/class/thermal/cooling_device0/ > cur_state  device.0  device.1  device.2  device.3  max_state  power   > subsystem type  uevent > > Instead of: > > $ ls /sys/class/thermal/cooling_device0/ > cur_state  device  max_state  power  subsystem type  uevent > > This is obviously to keep a link between the cooling device and > its affected devices. Would this break userspace? > Conceptually it is fine and should work. I need to run some tests before I confirm. Hopefully I can do in next 2 weeks. Thanks, Srinivas > BR, > > Eduardo Valentin > >  drivers/acpi/processor_driver.c | 40 > +++++++++++++++++++++++++++++++++++++--- >  1 file changed, 37 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/processor_driver.c > b/drivers/acpi/processor_driver.c > index 11154a3..e1e17de 100644 > --- a/drivers/acpi/processor_driver.c > +++ b/drivers/acpi/processor_driver.c > @@ -160,9 +160,29 @@ static struct notifier_block acpi_cpu_notifier = > { >  }; >   >  #ifdef CONFIG_ACPI_CPU_FREQ_PSS > +static struct thermal_cooling_device * > +acpi_pss_register_cooling(struct acpi_processor *pr, struct > acpi_device *device) > +{ > + int i, cpu = pr->id; > + > + for_each_online_cpu(i) { > + if (topology_physical_package_id(i) == > +     topology_physical_package_id(cpu)) { > + struct acpi_processor *pre = > per_cpu(processors, i); > + > + if (pre->cdev && !IS_ERR(pre->cdev)) > + return pre->cdev; > + } > + } > + > + return thermal_cooling_device_register("Processor", device, > +        &processor_cooling_op > s); > +} > + >  static int acpi_pss_perf_init(struct acpi_processor *pr, >   struct acpi_device *device) >  { > + char cpu_id[15]; >   int result = 0; >   >   acpi_processor_ppc_has_changed(pr, 0); > @@ -172,8 +192,7 @@ 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, > -    &processor_coolin > g_ops); > + pr->cdev = acpi_pss_register_cooling(pr, device); >   if (IS_ERR(pr->cdev)) { >   result = PTR_ERR(pr->cdev); >   return result; > @@ -191,9 +210,10 @@ static int acpi_pss_perf_init(struct > acpi_processor *pr, >   goto err_thermal_unregister; >   } >   > + snprintf(cpu_id, sizeof(cpu_id), "device.%d", pr->id); >   result = sysfs_create_link(&pr->cdev->device.kobj, >      &device->dev.kobj, > -    "device"); > +    cpu_id); >   if (result) { >   dev_err(&pr->cdev->device, >   "Failed to create sysfs link 'device'\n"); > @@ -213,11 +233,25 @@ static int acpi_pss_perf_init(struct > acpi_processor *pr, >  static void acpi_pss_perf_exit(struct acpi_processor *pr, >   struct acpi_device *device) >  { > + char cpu_id[15]; > + int i; > + >   if (pr->cdev) { >   sysfs_remove_link(&device->dev.kobj, > "thermal_cooling"); > + snprintf(cpu_id, sizeof(cpu_id), "device.%d", pr- > >id); >   sysfs_remove_link(&pr->cdev->device.kobj, "device"); >   thermal_cooling_device_unregister(pr->cdev); >   pr->cdev = NULL; > + > + for_each_online_cpu(i) { > + if (topology_physical_package_id(i) == > +     topology_physical_package_id(pr->id)) { > + struct acpi_processor *pre; > + > + pre = per_cpu(processors, i); > + pre->cdev = NULL; > + } > + } >   } >  } >  #else