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

[srinivas pandruvada]

On Tue, 2023-01-24 at 15:22 +0100, Rafael J. Wysocki wrote:
> 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?
> 
I think it will be OK. But remove call will traverse 6 levels of
function taking semaphores and finally call into kernfs_find_ns(),
where it will search a hash table and fail. So much more processing
than checking one if() condition.

> 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).
If you want, I can remove and resubmit.

Thanks,
Srinivas

> 
> > +}
> > +
> >  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);
> > --