| Message ID | 20230123163046.358879-1-srinivas.pandruvada@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 5c36cf27846a364f830e5ea86b411e05f7c8bd2b |
| Headers | show |
| Series | [v3] thermal: int340x_thermal: Add production mode attribute | expand |
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); > > --
On Tue, Jan 24, 2023 at 5:10 PM srinivas pandruvada <srinivas.pandruvada@linux.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@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. Fair enough.
On Tue, Jan 24, 2023 at 5:20 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jan 24, 2023 at 5:10 PM srinivas pandruvada > <srinivas.pandruvada@linux.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@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. > > Fair enough. So applied as 6.3 material, thanks!
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); +} + 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);
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(+)