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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hGPYfJ_H-+PWTaSPc3DjNDt4O5AWsGJUxjtU3jgDtktw@mail.gmail.com>
Date:   Tue, 24 Jan 2023 17:20:22 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>, rui.zhang@...el.com,
        daniel.lezcano@...aro.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3] thermal: int340x_thermal: Add production mode attribute

On Tue, Jan 24, 2023 at 5:10 PM srinivas pandruvada
<srinivas.pandruvada@...ux.intel.com> 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@...ux.intel.com> 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@...ux.intel.com>
> > > ---
> > > v3:
> > > Build warning reported by for missing static
> > > Reported-by: kernel test robot <lkp@...el.com>
> > >
> > > 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.