Re: [PATCH v3] thermal: int340x_thermal: Add production mode attribute

[Rafael J. Wysocki]

On Mon, Jan 23, 2023 at 5:31 PM Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> It is possible that the system manufacturer locks down thermal tuning
> beyond what is usually done on the given platform. In that case user
> space calibration tools should not try to adjust the thermal
> configuration of the system.
>
> To allow user space to check if that is the case, add a new sysfs
> attribute "production_mode" that will be present when the ACPI DCFG
> method is present under the INT3400 device object in the ACPI Namespace.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
> v3:
> Build warning reported by for missing static
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>
> v2
> Addressed comments from Rafael:
> - Updated commit excatly same as Rafael wrote
> - Removed production_mode_support bool
> - Use sysfs_emit
> - Update documentation
>
>  .../driver-api/thermal/intel_dptf.rst         |  3 ++
>  .../intel/int340x_thermal/int3400_thermal.c   | 48 +++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/Documentation/driver-api/thermal/intel_dptf.rst b/Documentation/driver-api/thermal/intel_dptf.rst
> index 372bdb4d04c6..f5c193cccbda 100644
> --- a/Documentation/driver-api/thermal/intel_dptf.rst
> +++ b/Documentation/driver-api/thermal/intel_dptf.rst
> @@ -84,6 +84,9 @@ DPTF ACPI Drivers interface
>         https:/github.com/intel/thermal_daemon for decoding
>         thermal table.
>
> +``production_mode`` (RO)
> +       When different from zero, manufacturer locked thermal configuration
> +       from further changes.
>
>  ACPI Thermal Relationship table interface
>  ------------------------------------------
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index db8a6f63657d..23ea21238bbd 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -60,6 +60,7 @@ struct int3400_thermal_priv {
>         int odvp_count;
>         int *odvp;
>         u32 os_uuid_mask;
> +       int production_mode;
>         struct odvp_attr *odvp_attrs;
>  };
>
> @@ -315,6 +316,44 @@ static int int3400_thermal_get_uuids(struct int3400_thermal_priv *priv)
>         return result;
>  }
>
> +static ssize_t production_mode_show(struct device *dev, struct device_attribute *attr,
> +                                    char *buf)
> +{
> +       struct int3400_thermal_priv *priv = dev_get_drvdata(dev);
> +
> +       return sysfs_emit(buf, "%d\n", priv->production_mode);
> +}
> +
> +static DEVICE_ATTR_RO(production_mode);
> +
> +static int production_mode_init(struct int3400_thermal_priv *priv)
> +{
> +       unsigned long long mode;
> +       acpi_status status;
> +       int ret;
> +
> +       priv->production_mode = -1;
> +
> +       status = acpi_evaluate_integer(priv->adev->handle, "DCFG", NULL, &mode);
> +       /* If the method is not present, this is not an error */
> +       if (ACPI_FAILURE(status))
> +               return 0;
> +
> +       ret = sysfs_create_file(&priv->pdev->dev.kobj, &dev_attr_production_mode.attr);
> +       if (ret)
> +               return ret;
> +
> +       priv->production_mode = mode;
> +
> +       return 0;
> +}
> +
> +static void production_mode_exit(struct int3400_thermal_priv *priv)
> +{
> +       if (priv->production_mode >= 0)
> +               sysfs_remove_file(&priv->pdev->dev.kobj, &dev_attr_production_mode.attr);

Isn't it OK to call sysfs_remove_file() if the given attribute is not there?

If so, the above check is unnecessary and the assignment to -1 above
too (as this is the only place where the value is tested).

> +}
> +
>  static ssize_t odvp_show(struct device *dev, struct device_attribute *attr,
>                          char *buf)
>  {
> @@ -610,8 +649,15 @@ static int int3400_thermal_probe(struct platform_device *pdev)
>         if (result)
>                 goto free_sysfs;
>
> +       result = production_mode_init(priv);
> +       if (result)
> +               goto free_notify;
> +
>         return 0;
>
> +free_notify:
> +       acpi_remove_notify_handler(priv->adev->handle, ACPI_DEVICE_NOTIFY,
> +                                  int3400_notify);
>  free_sysfs:
>         cleanup_odvp(priv);
>         if (!ZERO_OR_NULL_PTR(priv->data_vault)) {
> @@ -638,6 +684,8 @@ static int int3400_thermal_remove(struct platform_device *pdev)
>  {
>         struct int3400_thermal_priv *priv = platform_get_drvdata(pdev);
>
> +       production_mode_exit(priv);
> +
>         acpi_remove_notify_handler(
>                         priv->adev->handle, ACPI_DEVICE_NOTIFY,
>                         int3400_notify);
> --