[PATCH 0/3] hwmon: Introduce and use hwmon_device_register_for_thermal

[PATCH 0/3] hwmon: Introduce and use hwmon_device_register_for_thermal

[Guenter Roeck]

The thermal subsystem registers a hwmon driver without providing
chip information or sysfs group information. This is for legacy reasons
and would be difficult to change.

At the same time, several attempts have been made to convert hwmon
drivers using the deprecated hwmon_device_register() to use
hwmon_device_register_with_info() by just providing NULL parameters.
This is an abuse of the hwmon API. To prevent this abuse, we want to
enforce that a parent device pointer as well as chip information is
provided when registering a hwmon device using
hwmon_device_register_with_info().

To be able to do this, introduce and use a special API for use only by
the thermal subsystem (patches 1 and 2). Patch 3 makes the 'dev' and 'chip'
parameters of hwmon_device_register_with_info() mandatory.

----------------------------------------------------------------
Guenter Roeck (3):
      hwmon: Introduce hwmon_device_register_for_thermal
      thermal/drivers/thermal_hwmon: Use hwmon_device_register_for_thermal()
      hwmon: Make chip parameter for with_info API mandatory

 Documentation/hwmon/hwmon-kernel-api.rst |  2 +-
 drivers/hwmon/hwmon.c                    | 41 ++++++++++++++++++++++++++++++++---------
 drivers/thermal/thermal_hwmon.c          |  6 ++++--
 include/linux/hwmon.h                    |  3 +++
 4 files changed, 40 insertions(+), 12 deletions(-)

[PATCH 1/3] hwmon: Introduce hwmon_device_register_for_thermal

[Guenter Roeck]

The thermal subsystem registers a hwmon driver without providing
chip or sysfs group information. This is for legacy reasons and
would be difficult to change. At the same time, we want to enforce
that chip information is provided when registering a hwmon device
using hwmon_device_register_with_info(). To enable this, introduce
a special API for use only by the thermal subsystem.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 25 +++++++++++++++++++++++++
 include/linux/hwmon.h |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 5915ccfdb7d9..13053a4edc9e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -916,6 +916,31 @@ hwmon_device_register_with_info(struct device *dev, const char *name,
 }
 EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
 
+/**
+ * hwmon_device_register_for_thermal - register hwmon device for thermal subsystem
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ *
+ * The use of this function is restricted. It is provided for legacy reasons
+ * and must only be called from the thermal subsystem.
+ *
+ * hwmon_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *
+hwmon_device_register_for_thermal(struct device *dev, const char *name,
+				  void *drvdata)
+{
+	if (!name || !dev)
+		return ERR_PTR(-EINVAL);
+
+	return __hwmon_device_register(dev, name, drvdata, NULL, NULL);
+}
+EXPORT_SYMBOL_NS_GPL(hwmon_device_register_for_thermal, HWMON_THERMAL);
+
 /**
  * hwmon_device_register - register w/ hwmon
  * @dev: the device to register
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 4efaf06fd2b8..14325f93c6b2 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -450,6 +450,9 @@ hwmon_device_register_with_info(struct device *dev,
 				const struct hwmon_chip_info *info,
 				const struct attribute_group **extra_groups);
 struct device *
+hwmon_device_register_for_thermal(struct device *dev, const char *name,
+				  void *drvdata);
+struct device *
 devm_hwmon_device_register_with_info(struct device *dev,
 				const char *name, void *drvdata,
 				const struct hwmon_chip_info *info,
-- 
2.35.1

[PATCH 2/3] thermal/drivers/thermal_hwmon: Use hwmon_device_register_for_thermal()

[Guenter Roeck]

The thermal subsystem registers a hwmon device without providing chip
information or sysfs attribute groups. While undesirable, it would be
difficult to change. On the other side, it abuses the
hwmon_device_register_with_info API by not providing that information.
Use new API specifically created for the thermal subsystem instead to
let us enforce the 'chip' parameter for other callers of
hwmon_device_register_with_info().

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/thermal/thermal_hwmon.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
index ad03262cca56..09e49ec8b6f4 100644
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -149,8 +149,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 	INIT_LIST_HEAD(&hwmon->tz_list);
 	strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
 	strreplace(hwmon->type, '-', '_');
-	hwmon->device = hwmon_device_register_with_info(&tz->device, hwmon->type,
-							hwmon, NULL, NULL);
+	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
+							  hwmon->type, hwmon);
 	if (IS_ERR(hwmon->device)) {
 		result = PTR_ERR(hwmon->device);
 		goto free_mem;
@@ -277,3 +277,5 @@ int devm_thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(devm_thermal_add_hwmon_sysfs);
+
+MODULE_IMPORT_NS(HWMON_THERMAL);
-- 
2.35.1

[PATCH 3/3] hwmon: Make chip parameter for with_info API mandatory

[Guenter Roeck]

Various attempts were made recently to "convert" the old
hwmon_device_register() API to devm_hwmon_device_register_with_info()
by just changing the function name without actually converting the
driver. Prevent this from happening by making the 'chip' parameter of
devm_hwmon_device_register_with_info() mandatory.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/hwmon/hwmon-kernel-api.rst |  2 +-
 drivers/hwmon/hwmon.c                    | 16 +++++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
index e2975d5caf34..f3276b3a381a 100644
--- a/Documentation/hwmon/hwmon-kernel-api.rst
+++ b/Documentation/hwmon/hwmon-kernel-api.rst
@@ -76,7 +76,7 @@ hwmon_device_register_with_info is the most comprehensive and preferred means
 to register a hardware monitoring device. It creates the standard sysfs
 attributes in the hardware monitoring core, letting the driver focus on reading
 from and writing to the chip instead of having to bother with sysfs attributes.
-The parent device parameter cannot be NULL with non-NULL chip info. Its
+The parent device parameter as well as the chip parameter must not be NULL. Its
 parameters are described in more detail below.
 
 devm_hwmon_device_register_with_info is similar to
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 13053a4edc9e..22de7a9e7ba7 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -886,11 +886,12 @@ EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
 
 /**
  * hwmon_device_register_with_info - register w/ hwmon
- * @dev: the parent device
- * @name: hwmon name attribute
- * @drvdata: driver data to attach to created device
- * @chip: pointer to hwmon chip information
+ * @dev: the parent device (mandatory)
+ * @name: hwmon name attribute (mandatory)
+ * @drvdata: driver data to attach to created device (optional)
+ * @chip: pointer to hwmon chip information (mandatory)
  * @extra_groups: pointer to list of additional non-standard attribute groups
+ *	(optional)
  *
  * hwmon_device_unregister() must be called when the device is no
  * longer needed.
@@ -903,13 +904,10 @@ hwmon_device_register_with_info(struct device *dev, const char *name,
 				const struct hwmon_chip_info *chip,
 				const struct attribute_group **extra_groups)
 {
-	if (!name)
-		return ERR_PTR(-EINVAL);
-
-	if (chip && (!chip->ops || !chip->ops->is_visible || !chip->info))
+	if (!dev || !name || !chip)
 		return ERR_PTR(-EINVAL);
 
-	if (chip && !dev)
+	if (!chip->ops || !chip->ops->is_visible || !chip->info)
 		return ERR_PTR(-EINVAL);
 
 	return __hwmon_device_register(dev, name, drvdata, chip, extra_groups);
-- 
2.35.1

Re: [PATCH 0/3] hwmon: Introduce and use hwmon_device_register_for_thermal

[Rafael J. Wysocki]

On Wed, May 11, 2022 at 5:12 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> The thermal subsystem registers a hwmon driver without providing
> chip information or sysfs group information. This is for legacy reasons
> and would be difficult to change.
>
> At the same time, several attempts have been made to convert hwmon
> drivers using the deprecated hwmon_device_register() to use
> hwmon_device_register_with_info() by just providing NULL parameters.
> This is an abuse of the hwmon API. To prevent this abuse, we want to
> enforce that a parent device pointer as well as chip information is
> provided when registering a hwmon device using
> hwmon_device_register_with_info().
>
> To be able to do this, introduce and use a special API for use only by
> the thermal subsystem (patches 1 and 2). Patch 3 makes the 'dev' and 'chip'
> parameters of hwmon_device_register_with_info() mandatory.
>
> ----------------------------------------------------------------
> Guenter Roeck (3):
>       hwmon: Introduce hwmon_device_register_for_thermal
>       thermal/drivers/thermal_hwmon: Use hwmon_device_register_for_thermal()
>       hwmon: Make chip parameter for with_info API mandatory
>
>  Documentation/hwmon/hwmon-kernel-api.rst |  2 +-
>  drivers/hwmon/hwmon.c                    | 41 ++++++++++++++++++++++++++++++++---------
>  drivers/thermal/thermal_hwmon.c          |  6 ++++--
>  include/linux/hwmon.h                    |  3 +++
>  4 files changed, 40 insertions(+), 12 deletions(-)

This looks good to me from the thermal perspective, so please feel
free to add my ACKs to the first two patches.

Thanks!

Re: [PATCH 0/3] hwmon: Introduce and use hwmon_device_register_for_thermal

[Guenter Roeck]

On 5/11/22 11:21, Rafael J. Wysocki wrote:
> On Wed, May 11, 2022 at 5:12 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> The thermal subsystem registers a hwmon driver without providing
>> chip information or sysfs group information. This is for legacy reasons
>> and would be difficult to change.
>>
>> At the same time, several attempts have been made to convert hwmon
>> drivers using the deprecated hwmon_device_register() to use
>> hwmon_device_register_with_info() by just providing NULL parameters.
>> This is an abuse of the hwmon API. To prevent this abuse, we want to
>> enforce that a parent device pointer as well as chip information is
>> provided when registering a hwmon device using
>> hwmon_device_register_with_info().
>>
>> To be able to do this, introduce and use a special API for use only by
>> the thermal subsystem (patches 1 and 2). Patch 3 makes the 'dev' and 'chip'
>> parameters of hwmon_device_register_with_info() mandatory.
>>
>> ----------------------------------------------------------------
>> Guenter Roeck (3):
>>        hwmon: Introduce hwmon_device_register_for_thermal
>>        thermal/drivers/thermal_hwmon: Use hwmon_device_register_for_thermal()
>>        hwmon: Make chip parameter for with_info API mandatory
>>
>>   Documentation/hwmon/hwmon-kernel-api.rst |  2 +-
>>   drivers/hwmon/hwmon.c                    | 41 ++++++++++++++++++++++++++++++++---------
>>   drivers/thermal/thermal_hwmon.c          |  6 ++++--
>>   include/linux/hwmon.h                    |  3 +++
>>   4 files changed, 40 insertions(+), 12 deletions(-)
> 
> This looks good to me from the thermal perspective, so please feel
> free to add my ACKs to the first two patches.
> 

Thanks!

Guenter