Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3] thermal: int340x_thermal: Add production mode attribute | From | srinivas pandruvada <> | Date | Tue, 24 Jan 2023 08:10:26 -0800 |
| |
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@linux.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@linux.intel.com> > > --- > > v3: > > Build warning reported by for missing static > > Reported-by: kernel test robot <lkp@intel.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); > > --
| |