| 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
| ||
|
Message-ID: <1b9aa29a3c217eed97e2a732ba94bfc03f95f320.camel@linux.intel.com>
Date: Tue, 24 Jan 2023 08:10:26 -0800
From: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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, 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.
> 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);
> > --
Powered by blists - more mailing lists