Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230524125131.2413379-1-srinivas.pandruvada@linux.intel.com>
In-Reply-To: <20230524125131.2413379-1-srinivas.pandruvada@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 13 Jun 2023 18:02:23 +0200
Message-ID: <CAJZ5v0jJK21u-W1VKOWM3KJQf7LQrHZP-ZMd=WG=AFReRCJfXg@mail.gmail.com>
Subject: Re: [PATCH v2] thermal: intel: int340x_thermal: New IOCTLs for
 thermal tables
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     rafael@kernel.org, rui.zhang@intel.com, daniel.lezcano@linaro.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Wed, May 24, 2023 at 2:51 PM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Export Passive version 2 table similar to the way _TRT and _ART tables
> via IOCTLs.
>
> This removes need for binary utility to read ACPI Passive 2 table by
> providing open source support. This table already has open source
> implementation in the user space thermald, when the table is part of
> data vault exported by the int3400 sysfs.
>
> This table is supported in some older platforms before Ice Lake
> generation.
>
> Passive 2 tables contain multiple entries. Each entry has following
> fields:
>
> Source: Named Reference (String). This is the source device for
> temperature.
> Target: Named Reference (String). This is the target device to control.
> Priority: Priority of this device compared to others.
> SamplingPeriod: Time Period in 1/10 of seconds unit.
> PassiveTemp: Passive Temperature in 1/10 of Kelvin.
> SourceDomain: Domain for the source (00:Processor, others reserved).
> ControlKnob: Type of control knob (00:Power Limit 1, others: reserved)
> Limit: The target state to set on reaching passive temperature.
> This can be a string "max", "min" or a power limit value.
> LimitStepSize: Step size during activation.
> UnLimitStepSize: Step size during deactivation.
> Reserved1: Reserved
>
> Four IOCTLs are added similar to IOCTLs for reading TRT:
>
> ACPI_THERMAL_GET_PSVT_COUNT: Number of passive 2 entries.
> ACPI_THERMAL_GET_PSVT_LEN: Total return data size (count x each
> passive 2 entry size).
> ACPI_THERMAL_GET_PSVT: Get the data as an array of objects with
> passive 2 entries.
>
> This change is based on original development done by:
> Todd Brandt <todd.e.brandt@linux.intel.com>
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v2:
> - Fix issues reported by kernel test robot <lkp@intel.com>
> - Some minor change for name of destination in sizeof
>
>  .../intel/int340x_thermal/acpi_thermal_rel.c  | 218 ++++++++++++++++++
>  .../intel/int340x_thermal/acpi_thermal_rel.h  |  57 +++++
>  2 files changed, 275 insertions(+)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> index 01b80331eab6..dc519a665c18 100644
> --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> @@ -203,6 +203,151 @@ int acpi_parse_art(acpi_handle handle, int *art_count, struct art **artp,
>  }
>  EXPORT_SYMBOL(acpi_parse_art);
>
> +/*
> + * acpi_parse_psvt - Passive Table (PSVT) for passive cooling
> + *
> + * @handle: ACPI handle of the device which contains PSVT
> + * @psvt_count: the number of valid entries resulted from parsing PSVT
> + * @psvtp: pointer to array of psvt entries
> + *
> + */
> +static int acpi_parse_psvt(acpi_handle handle, int *psvt_count, struct psvt **psvtp)
> +{
> +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +       int nr_bad_entries = 0, revision = 0;
> +       union acpi_object *p;
> +       acpi_status status;
> +       int i, result = 0;
> +       struct psvt *psvts;
> +
> +       if (!acpi_has_method(handle, "PSVT"))
> +               return -ENODEV;
> +
> +       status = acpi_evaluate_object(handle, "PSVT", NULL, &buffer);
> +       if (ACPI_FAILURE(status))
> +               return -ENODEV;
> +
> +       p = buffer.pointer;
> +       if (!p || (p->type != ACPI_TYPE_PACKAGE)) {
> +               result = -EFAULT;
> +               goto end;
> +       }
> +
> +       /* first package is the revision number */
> +       if (p->package.count > 0) {
> +               union acpi_object *prev = &(p->package.elements[0]);
> +
> +               if (prev->type == ACPI_TYPE_INTEGER)
> +                       revision = (int)prev->integer.value;
> +       } else {
> +               result = -EFAULT;
> +               goto end;
> +       }
> +
> +       /* Support only version 2 */
> +       if (revision != 2) {
> +               result = -EFAULT;
> +               goto end;
> +       }
> +
> +       *psvt_count = p->package.count - 1;
> +       if (!*psvt_count) {
> +               result = -EFAULT;
> +               goto end;
> +       }
> +
> +       psvts = kcalloc(*psvt_count, sizeof(*psvts), GFP_KERNEL);
> +       if (!psvts) {
> +               result = -ENOMEM;
> +               goto end;
> +       }
> +
> +       /* Start index is 1 because the first package is the revision number */
> +       for (i = 1; i < p->package.count; i++) {
> +               struct acpi_buffer psvt_int_format = { sizeof("RRNNNNNNNNNN"), "RRNNNNNNNNNN" };
> +               struct acpi_buffer psvt_str_format = { sizeof("RRNNNNNSNNNN"), "RRNNNNNSNNNN" };
> +               union acpi_object *package = &(p->package.elements[i]);
> +               struct psvt *psvt = &psvts[i - 1 - nr_bad_entries];
> +               struct acpi_buffer *psvt_format = &psvt_int_format;
> +               struct acpi_buffer element = { 0, NULL };
> +               union acpi_object *knob;
> +               struct acpi_device *res;
> +               struct psvt *psvt_ptr;
> +
> +               element.length = ACPI_ALLOCATE_BUFFER;
> +               element.pointer = NULL;
> +
> +               if (package->package.count >= ACPI_NR_PSVT_ELEMENTS) {
> +                       knob = &(package->package.elements[ACPI_PSVT_CONTROL_KNOB]);
> +               } else {
> +                       nr_bad_entries++;
> +                       pr_info("PSVT package %d is invalid, ignored\n", i);
> +                       continue;
> +               }
> +
> +               if (knob->type == ACPI_TYPE_STRING) {
> +                       psvt_format = &psvt_str_format;
> +                       if (knob->string.length > ACPI_LIMIT_STR_MAX_LEN - 1) {
> +                               pr_info("PSVT package %d limit string len exceeds max\n", i);
> +                               knob->string.length = ACPI_LIMIT_STR_MAX_LEN - 1;
> +                       }
> +               }
> +
> +               status = acpi_extract_package(&(p->package.elements[i]), psvt_format, &element);
> +               if (ACPI_FAILURE(status)) {
> +                       nr_bad_entries++;
> +                       pr_info("PSVT package %d is invalid, ignored\n", i);
> +                       continue;
> +               }
> +
> +               psvt_ptr = (struct psvt *)element.pointer;
> +
> +               memcpy(psvt, psvt_ptr, sizeof(*psvt));
> +
> +               /* The limit element can be string or U64 */
> +               psvt->control_knob_type = (u64)knob->type;
> +
> +               if (knob->type == ACPI_TYPE_STRING) {
> +                       memset(&psvt->limit, 0, sizeof(u64));
> +                       strncpy(psvt->limit.string, psvt_ptr->limit.str_ptr, knob->string.length);
> +               } else {
> +                       psvt->limit.integer = psvt_ptr->limit.integer;
> +               }
> +
> +               kfree(element.pointer);
> +
> +               res = acpi_fetch_acpi_dev(psvt->source);
> +               if (!res) {
> +                       nr_bad_entries++;
> +                       pr_info("Failed to get source ACPI device\n");
> +                       continue;
> +               }
> +
> +               res = acpi_fetch_acpi_dev(psvt->target);
> +               if (!res) {
> +                       nr_bad_entries++;
> +                       pr_info("Failed to get target ACPI device\n");
> +                       continue;
> +               }
> +       }
> +
> +       /* don't count bad entries */
> +       *psvt_count -= nr_bad_entries;
> +
> +       if (!*psvt_count) {
> +               result = -EFAULT;
> +               kfree(psvts);
> +               goto end;
> +       }
> +
> +       *psvtp = psvts;
> +
> +       return 0;
> +
> +end:
> +       kfree(buffer.pointer);
> +       return result;
> +}
>
>  /* get device name from acpi handle */
>  static void get_single_name(acpi_handle handle, char *name)
> @@ -289,6 +434,57 @@ static int fill_trt(char __user *ubuf)
>         return ret;
>  }
>
> +static int fill_psvt(char __user *ubuf)
> +{
> +       int i, ret, count, psvt_len;
> +       union psvt_object *psvt_user;
> +       struct psvt *psvts;
> +
> +       ret = acpi_parse_psvt(acpi_thermal_rel_handle, &count, &psvts);
> +       if (ret)
> +               return ret;
> +
> +       psvt_len = count * sizeof(*psvt_user);
> +
> +       psvt_user = kzalloc(psvt_len, GFP_KERNEL);
> +       if (!psvt_user) {
> +               ret = -ENOMEM;
> +               goto free_psvt;
> +       }
> +
> +       /* now fill in user psvt data */
> +       for (i = 0; i < count; i++) {
> +               /* userspace psvt needs device name instead of acpi reference */
> +               get_single_name(psvts[i].source, psvt_user[i].source_device);
> +               get_single_name(psvts[i].target, psvt_user[i].target_device);
> +
> +               psvt_user[i].priority = psvts[i].priority;
> +               psvt_user[i].sample_period = psvts[i].sample_period;
> +               psvt_user[i].passive_temp = psvts[i].passive_temp;
> +               psvt_user[i].source_domain = psvts[i].source_domain;
> +               psvt_user[i].control_knob = psvts[i].control_knob;
> +               psvt_user[i].step_size = psvts[i].step_size;
> +               psvt_user[i].limit_coeff = psvts[i].limit_coeff;
> +               psvt_user[i].unlimit_coeff = psvts[i].unlimit_coeff;
> +               psvt_user[i].control_knob_type = psvts[i].control_knob_type;
> +               if (psvt_user[i].control_knob_type == ACPI_TYPE_STRING)
> +                       strncpy(psvt_user[i].limit.string, psvts[i].limit.string,
> +                               ACPI_LIMIT_STR_MAX_LEN);
> +               else
> +                       psvt_user[i].limit.integer = psvts[i].limit.integer;
> +
> +       }
> +
> +       if (copy_to_user(ubuf, psvt_user, psvt_len))
> +               ret = -EFAULT;
> +
> +       kfree(psvt_user);
> +
> +free_psvt:
> +       kfree(psvts);
> +       return ret;
> +}
> +
>  static long acpi_thermal_rel_ioctl(struct file *f, unsigned int cmd,
>                                    unsigned long __arg)
>  {
> @@ -298,6 +494,7 @@ static long acpi_thermal_rel_ioctl(struct file *f, unsigned int cmd,
>         char __user *arg = (void __user *)__arg;
>         struct trt *trts = NULL;
>         struct art *arts = NULL;
> +       struct psvt *psvts;
>
>         switch (cmd) {
>         case ACPI_THERMAL_GET_TRT_COUNT:
> @@ -336,6 +533,27 @@ static long acpi_thermal_rel_ioctl(struct file *f, unsigned int cmd,
>         case ACPI_THERMAL_GET_ART:
>                 return fill_art(arg);
>
> +       case ACPI_THERMAL_GET_PSVT_COUNT:
> +               ret = acpi_parse_psvt(acpi_thermal_rel_handle, &count, &psvts);
> +               if (!ret) {
> +                       kfree(psvts);
> +                       return put_user(count, (unsigned long __user *)__arg);
> +               }
> +               return ret;
> +
> +       case ACPI_THERMAL_GET_PSVT_LEN:
> +               /* total length of the data retrieved (count * PSVT entry size) */
> +               ret = acpi_parse_psvt(acpi_thermal_rel_handle, &count, &psvts);
> +               length = count * sizeof(union psvt_object);
> +               if (!ret) {
> +                       kfree(psvts);
> +                       return put_user(length, (unsigned long __user *)__arg);
> +               }
> +               return ret;
> +
> +       case ACPI_THERMAL_GET_PSVT:
> +               return fill_psvt(arg);
> +
>         default:
>                 return -ENOTTY;
>         }
> diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> index 78d942477035..ac376d8f9ee4 100644
> --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> @@ -14,6 +14,16 @@
>  #define ACPI_THERMAL_GET_TRT   _IOR(ACPI_THERMAL_MAGIC, 5, unsigned long)
>  #define ACPI_THERMAL_GET_ART   _IOR(ACPI_THERMAL_MAGIC, 6, unsigned long)
>
> +/*
> + * ACPI_THERMAL_GET_PSVT_COUNT = Number of PSVT entries
> + * ACPI_THERMAL_GET_PSVT_LEN = Total return data size (PSVT count x each
> + * PSVT entry size)
> + * ACPI_THERMAL_GET_PSVT = Get the data as an array of psvt_objects
> + */
> +#define ACPI_THERMAL_GET_PSVT_LEN _IOR(ACPI_THERMAL_MAGIC, 7, unsigned long)
> +#define ACPI_THERMAL_GET_PSVT_COUNT _IOR(ACPI_THERMAL_MAGIC, 8, unsigned long)
> +#define ACPI_THERMAL_GET_PSVT  _IOR(ACPI_THERMAL_MAGIC, 9, unsigned long)
> +
>  struct art {
>         acpi_handle source;
>         acpi_handle target;
> @@ -43,6 +53,32 @@ struct trt {
>         u64 reserved4;
>  } __packed;
>
> +#define ACPI_NR_PSVT_ELEMENTS  12
> +#define ACPI_PSVT_CONTROL_KNOB 7
> +#define ACPI_LIMIT_STR_MAX_LEN 8
> +
> +struct psvt {
> +       acpi_handle source;
> +       acpi_handle target;
> +       u64 priority;
> +       u64 sample_period;
> +       u64 passive_temp;
> +       u64 source_domain;
> +       u64 control_knob;
> +       union {
> +               /* For limit_type = ACPI_TYPE_INTEGER */
> +               u64 integer;
> +               /* For limit_type = ACPI_TYPE_STRING */
> +               char string[ACPI_LIMIT_STR_MAX_LEN];
> +               char *str_ptr;
> +       } limit;
> +       u64 step_size;
> +       u64 limit_coeff;
> +       u64 unlimit_coeff;
> +       /* Spec calls this field reserved, so we borrow it for type info */
> +       u64 control_knob_type; /* ACPI_TYPE_STRING or ACPI_TYPE_INTEGER */
> +} __packed;
> +
>  #define ACPI_NR_ART_ELEMENTS 13
>  /* for usrspace */
>  union art_object {
> @@ -77,6 +113,27 @@ union trt_object {
>         u64 __data[8];
>  };
>
> +union psvt_object {
> +       struct {
> +               char source_device[8];
> +               char target_device[8];
> +               u64 priority;
> +               u64 sample_period;
> +               u64 passive_temp;
> +               u64 source_domain;
> +               u64 control_knob;
> +               union {
> +                       u64 integer;
> +                       char string[ACPI_LIMIT_STR_MAX_LEN];
> +               } limit;
> +               u64 step_size;
> +               u64 limit_coeff;
> +               u64 unlimit_coeff;
> +               u64 control_knob_type;
> +       };
> +       u64 __data[ACPI_NR_PSVT_ELEMENTS];
> +};
> +
>  #ifdef __KERNEL__
>  int acpi_thermal_rel_misc_device_add(acpi_handle handle);
>  int acpi_thermal_rel_misc_device_remove(acpi_handle handle);
> --

Applied as 6.5 material, thanks!