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

[Rafael J. Wysocki]

On Tue, Jan 24, 2023 at 5:20 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Tue, Jan 24, 2023 at 5:10 PM srinivas pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> >
> > 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.
>
> Fair enough.

So applied as 6.3 material, thanks!