[PATCH v3 0/2] hwmon: introduce hwmon_sanitize()

[PATCH v3 0/2] hwmon: introduce hwmon_sanitize()

[Michael Walle]

During development of the support for the temperature sensor on the GPY
PHY, I've noticed that there is ususually a loop over the name to
replace any invalid characters. Instead of open coding it in the drivers
provide a convenience function.

The last patch is marked as RFC, it should probably be reposted/applied
to the kernel release after next (?).

changes since v2:
 - doc update
 - dropped last three patches, the net patches will be submitted
   seperately

changes since v1:
 - split patches
 - add hwmon-kernel-api.rst documentation
 - move the strdup into the hwmon core
 - also provide a resource managed variant

Michael Walle (2):
  hwmon: introduce hwmon_sanitize_name()
  hwmon: intel-m10-bmc-hwmon: use devm_hwmon_sanitize_name()

 Documentation/hwmon/hwmon-kernel-api.rst | 16 ++++++++
 drivers/hwmon/hwmon.c                    | 50 ++++++++++++++++++++++++
 drivers/hwmon/intel-m10-bmc-hwmon.c      |  7 +---
 include/linux/hwmon.h                    |  3 ++
 4 files changed, 70 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v3 1/2] hwmon: introduce hwmon_sanitize_name()

[Michael Walle]

More and more drivers will check for bad characters in the hwmon name
and all are using the same code snippet. Consolidate that code by adding
a new hwmon_sanitize_name() function.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 Documentation/hwmon/hwmon-kernel-api.rst | 16 ++++++++
 drivers/hwmon/hwmon.c                    | 50 ++++++++++++++++++++++++
 include/linux/hwmon.h                    |  3 ++
 3 files changed, 69 insertions(+)

diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
index c41eb6108103..e2975d5caf34 100644
--- a/Documentation/hwmon/hwmon-kernel-api.rst
+++ b/Documentation/hwmon/hwmon-kernel-api.rst
@@ -50,6 +50,10 @@ register/unregister functions::
 
   void devm_hwmon_device_unregister(struct device *dev);
 
+  char *hwmon_sanitize_name(const char *name);
+
+  char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
+
 hwmon_device_register_with_groups registers a hardware monitoring device.
 The first parameter of this function is a pointer to the parent device.
 The name parameter is a pointer to the hwmon device name. The registration
@@ -95,6 +99,18 @@ All supported hwmon device registration functions only accept valid device
 names. Device names including invalid characters (whitespace, '*', or '-')
 will be rejected. The 'name' parameter is mandatory.
 
+If the driver doesn't use a static device name (for example it uses
+dev_name()), and therefore cannot make sure the name only contains valid
+characters, hwmon_sanitize_name can be used. This convenience function
+will duplicate the string and replace any invalid characters with an
+underscore. It will allocate memory for the new string and it is the
+responsibility of the caller to release the memory when the device is
+removed.
+
+devm_hwmon_sanitize_name is the resource managed version of
+hwmon_sanitize_name; the memory will be freed automatically on device
+removal.
+
 Using devm_hwmon_device_register_with_info()
 --------------------------------------------
 
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 989e2c8496dd..cc4a16a466a0 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -1057,6 +1057,56 @@ void devm_hwmon_device_unregister(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister);
 
+static char *__hwmon_sanitize_name(struct device *dev, const char *old_name)
+{
+	char *name, *p;
+
+	if (dev)
+		name = devm_kstrdup(dev, old_name, GFP_KERNEL);
+	else
+		name = kstrdup(old_name, GFP_KERNEL);
+	if (!name)
+		return NULL;
+
+	for (p = name; *p; p++)
+		if (hwmon_is_bad_char(*p))
+			*p = '_';
+
+	return name;
+}
+
+/**
+ * hwmon_sanitize_name - Replaces invalid characters in a hwmon name
+ * @name: NUL-terminated name
+ *
+ * Allocates a new string where any invalid characters will be replaced
+ * by an underscore. It is the responsibility of the caller to release
+ * the memory.
+ *
+ * Returns newly allocated name or %NULL in case of error.
+ */
+char *hwmon_sanitize_name(const char *name)
+{
+	return __hwmon_sanitize_name(NULL, name);
+}
+EXPORT_SYMBOL_GPL(hwmon_sanitize_name);
+
+/**
+ * devm_hwmon_sanitize_name - resource managed hwmon_sanitize_name()
+ * @dev: device to allocate memory for
+ * @name: NUL-terminated name
+ *
+ * Allocates a new string where any invalid characters will be replaced
+ * by an underscore.
+ *
+ * Returns newly allocated name or %NULL in case of error.
+ */
+char *devm_hwmon_sanitize_name(struct device *dev, const char *name)
+{
+	return __hwmon_sanitize_name(dev, name);
+}
+EXPORT_SYMBOL_GPL(devm_hwmon_sanitize_name);
+
 static void __init hwmon_pci_quirks(void)
 {
 #if defined CONFIG_X86 && defined CONFIG_PCI
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index eba380b76d15..4efaf06fd2b8 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -461,6 +461,9 @@ void devm_hwmon_device_unregister(struct device *dev);
 int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel);
 
+char *hwmon_sanitize_name(const char *name);
+char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
+
 /**
  * hwmon_is_bad_char - Is the char invalid in a hwmon name
  * @ch: the char to be considered
-- 
2.30.2

Re: [PATCH v3 1/2] hwmon: introduce hwmon_sanitize_name()

[Tom Rix]

On 4/4/22 11:43 AM, Michael Walle wrote:
> More and more drivers will check for bad characters in the hwmon name
> and all are using the same code snippet. Consolidate that code by adding
> a new hwmon_sanitize_name() function.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>   Documentation/hwmon/hwmon-kernel-api.rst | 16 ++++++++
>   drivers/hwmon/hwmon.c                    | 50 ++++++++++++++++++++++++
>   include/linux/hwmon.h                    |  3 ++
>   3 files changed, 69 insertions(+)
>
> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> index c41eb6108103..e2975d5caf34 100644
> --- a/Documentation/hwmon/hwmon-kernel-api.rst
> +++ b/Documentation/hwmon/hwmon-kernel-api.rst
> @@ -50,6 +50,10 @@ register/unregister functions::
>   
>     void devm_hwmon_device_unregister(struct device *dev);
>   
> +  char *hwmon_sanitize_name(const char *name);
> +
> +  char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
> +
>   hwmon_device_register_with_groups registers a hardware monitoring device.
>   The first parameter of this function is a pointer to the parent device.
>   The name parameter is a pointer to the hwmon device name. The registration
> @@ -95,6 +99,18 @@ All supported hwmon device registration functions only accept valid device
>   names. Device names including invalid characters (whitespace, '*', or '-')
>   will be rejected. The 'name' parameter is mandatory.
>   
> +If the driver doesn't use a static device name (for example it uses
> +dev_name()), and therefore cannot make sure the name only contains valid
> +characters, hwmon_sanitize_name can be used. This convenience function
> +will duplicate the string and replace any invalid characters with an
> +underscore. It will allocate memory for the new string and it is the
> +responsibility of the caller to release the memory when the device is
> +removed.
> +
> +devm_hwmon_sanitize_name is the resource managed version of
> +hwmon_sanitize_name; the memory will be freed automatically on device
> +removal.
> +
>   Using devm_hwmon_device_register_with_info()
>   --------------------------------------------
>   
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 989e2c8496dd..cc4a16a466a0 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -1057,6 +1057,56 @@ void devm_hwmon_device_unregister(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister);
>   
> +static char *__hwmon_sanitize_name(struct device *dev, const char *old_name)
> +{
> +	char *name, *p;
> +
> +	if (dev)
> +		name = devm_kstrdup(dev, old_name, GFP_KERNEL);
> +	else
> +		name = kstrdup(old_name, GFP_KERNEL);
> +	if (!name)
> +		return NULL;
should return ERR_PTR(-ENOMEM)
> +
> +	for (p = name; *p; p++)
> +		if (hwmon_is_bad_char(*p))
> +			*p = '_';
> +
> +	return name;
> +}
> +
> +/**
> + * hwmon_sanitize_name - Replaces invalid characters in a hwmon name
> + * @name: NUL-terminated name
> + *
> + * Allocates a new string where any invalid characters will be replaced
> + * by an underscore. It is the responsibility of the caller to release
> + * the memory.
> + *
> + * Returns newly allocated name or %NULL in case of error.
> + */
> +char *hwmon_sanitize_name(const char *name)
> +{
> +	return __hwmon_sanitize_name(NULL, name);
> +}
> +EXPORT_SYMBOL_GPL(hwmon_sanitize_name);
> +
> +/**
> + * devm_hwmon_sanitize_name - resource managed hwmon_sanitize_name()
> + * @dev: device to allocate memory for
> + * @name: NUL-terminated name
> + *
> + * Allocates a new string where any invalid characters will be replaced
> + * by an underscore.
> + *
> + * Returns newly allocated name or %NULL in case of error.
> + */
> +char *devm_hwmon_sanitize_name(struct device *dev, const char *name)
> +{
> +	return __hwmon_sanitize_name(dev, name);
Should have a (!dev) check.
> +}
> +EXPORT_SYMBOL_GPL(devm_hwmon_sanitize_name);
> +
>   static void __init hwmon_pci_quirks(void)
>   {
>   #if defined CONFIG_X86 && defined CONFIG_PCI
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index eba380b76d15..4efaf06fd2b8 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -461,6 +461,9 @@ void devm_hwmon_device_unregister(struct device *dev);
>   int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
>   		       u32 attr, int channel);
>   
> +char *hwmon_sanitize_name(const char *name);
> +char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
> +
>   /**
>    * hwmon_is_bad_char - Is the char invalid in a hwmon name

This still needed in *.h ?

Tom

>    * @ch: the char to be considered

Re: [PATCH v3 1/2] hwmon: introduce hwmon_sanitize_name()

[Guenter Roeck]

On 4/4/22 16:14, Tom Rix wrote:
> 
> On 4/4/22 11:43 AM, Michael Walle wrote:
>> More and more drivers will check for bad characters in the hwmon name
>> and all are using the same code snippet. Consolidate that code by adding
>> a new hwmon_sanitize_name() function.
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>   Documentation/hwmon/hwmon-kernel-api.rst | 16 ++++++++
>>   drivers/hwmon/hwmon.c                    | 50 ++++++++++++++++++++++++
>>   include/linux/hwmon.h                    |  3 ++
>>   3 files changed, 69 insertions(+)
>>
>> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
>> index c41eb6108103..e2975d5caf34 100644
>> --- a/Documentation/hwmon/hwmon-kernel-api.rst
>> +++ b/Documentation/hwmon/hwmon-kernel-api.rst
>> @@ -50,6 +50,10 @@ register/unregister functions::
>>     void devm_hwmon_device_unregister(struct device *dev);
>> +  char *hwmon_sanitize_name(const char *name);
>> +
>> +  char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
>> +
>>   hwmon_device_register_with_groups registers a hardware monitoring device.
>>   The first parameter of this function is a pointer to the parent device.
>>   The name parameter is a pointer to the hwmon device name. The registration
>> @@ -95,6 +99,18 @@ All supported hwmon device registration functions only accept valid device
>>   names. Device names including invalid characters (whitespace, '*', or '-')
>>   will be rejected. The 'name' parameter is mandatory.
>> +If the driver doesn't use a static device name (for example it uses
>> +dev_name()), and therefore cannot make sure the name only contains valid
>> +characters, hwmon_sanitize_name can be used. This convenience function
>> +will duplicate the string and replace any invalid characters with an
>> +underscore. It will allocate memory for the new string and it is the
>> +responsibility of the caller to release the memory when the device is
>> +removed.
>> +
>> +devm_hwmon_sanitize_name is the resource managed version of
>> +hwmon_sanitize_name; the memory will be freed automatically on device
>> +removal.
>> +
>>   Using devm_hwmon_device_register_with_info()
>>   --------------------------------------------
>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>> index 989e2c8496dd..cc4a16a466a0 100644
>> --- a/drivers/hwmon/hwmon.c
>> +++ b/drivers/hwmon/hwmon.c
>> @@ -1057,6 +1057,56 @@ void devm_hwmon_device_unregister(struct device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister);
>> +static char *__hwmon_sanitize_name(struct device *dev, const char *old_name)
>> +{
>> +    char *name, *p;
>> +
>> +    if (dev)
>> +        name = devm_kstrdup(dev, old_name, GFP_KERNEL);
>> +    else
>> +        name = kstrdup(old_name, GFP_KERNEL);
>> +    if (!name)
>> +        return NULL;
> should return ERR_PTR(-ENOMEM)

In general this is a matter of opinion, but other hwmon functions
return  ERR_PTR(-ENOMEM), so I agree here.

>> +
>> +    for (p = name; *p; p++)
>> +        if (hwmon_is_bad_char(*p))
>> +            *p = '_';
>> +
>> +    return name;
>> +}
>> +
>> +/**
>> + * hwmon_sanitize_name - Replaces invalid characters in a hwmon name
>> + * @name: NUL-terminated name
>> + *
>> + * Allocates a new string where any invalid characters will be replaced
>> + * by an underscore. It is the responsibility of the caller to release
>> + * the memory.
>> + *
>> + * Returns newly allocated name or %NULL in case of error.
>> + */
>> +char *hwmon_sanitize_name(const char *name)
>> +{
>> +    return __hwmon_sanitize_name(NULL, name);
>> +}
>> +EXPORT_SYMBOL_GPL(hwmon_sanitize_name);
>> +
>> +/**
>> + * devm_hwmon_sanitize_name - resource managed hwmon_sanitize_name()
>> + * @dev: device to allocate memory for
>> + * @name: NUL-terminated name
>> + *
>> + * Allocates a new string where any invalid characters will be replaced
>> + * by an underscore.
>> + *
>> + * Returns newly allocated name or %NULL in case of error.
>> + */
>> +char *devm_hwmon_sanitize_name(struct device *dev, const char *name)
>> +{
>> +    return __hwmon_sanitize_name(dev, name);
> Should have a (!dev) check.

... and return ERR_PTR(-EINVAL) if it is NULL, to match
hwmon registration functions.

>> +}
>> +EXPORT_SYMBOL_GPL(devm_hwmon_sanitize_name);
>> +
>>   static void __init hwmon_pci_quirks(void)
>>   {
>>   #if defined CONFIG_X86 && defined CONFIG_PCI
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index eba380b76d15..4efaf06fd2b8 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -461,6 +461,9 @@ void devm_hwmon_device_unregister(struct device *dev);
>>   int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
>>                  u32 attr, int channel);
>> +char *hwmon_sanitize_name(const char *name);
>> +char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
>> +
>>   /**
>>    * hwmon_is_bad_char - Is the char invalid in a hwmon name
> 
> This still needed in *.h ?
> 

Yes, because it is used by out-of-tree drivers. We can only move
that into hwmon.c after all users are gone.

Guenter

RE: [PATCH v3 1/2] hwmon: introduce hwmon_sanitize_name()

[David Laight]

From: Guenter Roeck
> Sent: 05 April 2022 00:31
...
> >>   /**
> >>    * hwmon_is_bad_char - Is the char invalid in a hwmon name
> >
> > This still needed in *.h ?
> >
> 
> Yes, because it is used by out-of-tree drivers. We can only move
> that into hwmon.c after all users are gone.

And drivers might want to 'sanitize' a copy of the name without
reallocating it.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH v3 2/2] hwmon: intel-m10-bmc-hwmon: use devm_hwmon_sanitize_name()

[Michael Walle]

Instead of open-coding the bad characters replacement in the hwmon name,
use the new devm_hwmon_sanitize_name().

Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/hwmon/intel-m10-bmc-hwmon.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
index 7a08e4c44a4b..29370108fa1c 100644
--- a/drivers/hwmon/intel-m10-bmc-hwmon.c
+++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
@@ -515,7 +515,6 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
 	struct intel_m10bmc *m10bmc = dev_get_drvdata(pdev->dev.parent);
 	struct device *hwmon_dev, *dev = &pdev->dev;
 	struct m10bmc_hwmon *hw;
-	int i;
 
 	hw = devm_kzalloc(dev, sizeof(*hw), GFP_KERNEL);
 	if (!hw)
@@ -528,14 +527,10 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
 	hw->chip.info = hw->bdata->hinfo;
 	hw->chip.ops = &m10bmc_hwmon_ops;
 
-	hw->hw_name = devm_kstrdup(dev, id->name, GFP_KERNEL);
+	hw->hw_name = devm_hwmon_sanitize_name(dev, id->name);
 	if (!hw->hw_name)
 		return -ENOMEM;
 
-	for (i = 0; hw->hw_name[i]; i++)
-		if (hwmon_is_bad_char(hw->hw_name[i]))
-			hw->hw_name[i] = '_';
-
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, hw->hw_name,
 							 hw, &hw->chip, NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
-- 
2.30.2