[PATCH v2 0/5] hwmon: introduce hwmon_sanitize()

[PATCH v2 0/5] 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 v1:
 - split patches
 - add hwmon-kernel-api.rst documentation
 - move the strdup into the hwmon core
 - also provide a resource managed variant

Michael Walle (5):
  hwmon: introduce hwmon_sanitize_name()
  hwmon: intel-m10-bmc-hwmon: use devm_hwmon_sanitize_name()
  net: sfp: use hwmon_sanitize_name()
  net: phy: nxp-tja11xx: use devm_hwmon_sanitize_name()
  hwmon: move hwmon_is_bad_char() into core

 Documentation/hwmon/hwmon-kernel-api.rst |  9 +++-
 drivers/hwmon/hwmon.c                    | 69 ++++++++++++++++++++++++
 drivers/hwmon/intel-m10-bmc-hwmon.c      |  7 +--
 drivers/net/phy/nxp-tja11xx.c            |  7 +--
 drivers/net/phy/sfp.c                    |  8 +--
 include/linux/hwmon.h                    | 24 +--------
 6 files changed, 83 insertions(+), 41 deletions(-)

-- 
2.30.2

[PATCH v2 1/5] 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 |  9 ++++-
 drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
 include/linux/hwmon.h                    |  3 ++
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
index c41eb6108103..12f4a9bcef04 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
@@ -93,7 +97,10 @@ removal would be too late.
 
 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.
+will be rejected. The 'name' parameter is mandatory. Before calling a
+register function you should either use hwmon_sanitize_name or
+devm_hwmon_sanitize_name to replace any invalid characters with an
+underscore.
 
 Using devm_hwmon_device_register_with_info()
 --------------------------------------------
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 989e2c8496dd..619ef9f9a16e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -1057,6 +1057,55 @@ 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.
+ *
+ * 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

[PATCH v2 2/5] 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>
---
 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

[PATCH v2 3/5] net: sfp: use hwmon_sanitize_name()

[Michael Walle]

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

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/sfp.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4dfb79807823..0d5dba30444d 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1289,7 +1289,7 @@ static const struct hwmon_chip_info sfp_hwmon_chip_info = {
 static void sfp_hwmon_probe(struct work_struct *work)
 {
 	struct sfp *sfp = container_of(work, struct sfp, hwmon_probe.work);
-	int err, i;
+	int err;
 
 	/* hwmon interface needs to access 16bit registers in atomic way to
 	 * guarantee coherency of the diagnostic monitoring data. If it is not
@@ -1317,16 +1317,12 @@ static void sfp_hwmon_probe(struct work_struct *work)
 		return;
 	}
 
-	sfp->hwmon_name = kstrdup(dev_name(sfp->dev), GFP_KERNEL);
+	sfp->hwmon_name = hwmon_sanitize_name(dev_name(sfp->dev));
 	if (!sfp->hwmon_name) {
 		dev_err(sfp->dev, "out of memory for hwmon name\n");
 		return;
 	}
 
-	for (i = 0; sfp->hwmon_name[i]; i++)
-		if (hwmon_is_bad_char(sfp->hwmon_name[i]))
-			sfp->hwmon_name[i] = '_';
-
 	sfp->hwmon_dev = hwmon_device_register_with_info(sfp->dev,
 							 sfp->hwmon_name, sfp,
 							 &sfp_hwmon_chip_info,
-- 
2.30.2

[PATCH v2 4/5] net: phy: nxp-tja11xx: 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>
---
 drivers/net/phy/nxp-tja11xx.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index 9944cc501806..8088c2a4a000 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -444,16 +444,11 @@ static int tja11xx_hwmon_register(struct phy_device *phydev,
 				  struct tja11xx_priv *priv)
 {
 	struct device *dev = &phydev->mdio.dev;
-	int i;
 
-	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	priv->hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
 	if (!priv->hwmon_name)
 		return -ENOMEM;
 
-	for (i = 0; priv->hwmon_name[i]; i++)
-		if (hwmon_is_bad_char(priv->hwmon_name[i]))
-			priv->hwmon_name[i] = '_';
-
 	priv->hwmon_dev =
 		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
 						     phydev,
-- 
2.30.2

[PATCH RFC v2 5/5] hwmon: move hwmon_is_bad_char() into core

[Michael Walle]

With the last user of this function converted to hwmon_sanitize_name(),
move the function into the core itself and make it private.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/hwmon/hwmon.c | 20 ++++++++++++++++++++
 include/linux/hwmon.h | 23 -----------------------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 619ef9f9a16e..f19b69b066ef 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -1057,6 +1057,26 @@ void devm_hwmon_device_unregister(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister);
 
+/**
+ * hwmon_is_bad_char - Is the char invalid in a hwmon name
+ * @ch: the char to be considered
+ *
+ * Returns true if the char is invalid, false otherwise.
+ */
+static bool hwmon_is_bad_char(const char ch)
+{
+	switch (ch) {
+	case '-':
+	case '*':
+	case ' ':
+	case '\t':
+	case '\n':
+		return true;
+	default:
+		return false;
+	}
+}
+
 static char *__hwmon_sanitize_name(struct device *dev, const char *old_name)
 {
 	char *name, *p;
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 4efaf06fd2b8..6a60e3a4acc0 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -464,27 +464,4 @@ int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 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
- *
- * hwmon_is_bad_char() can be used to determine if the given character
- * may not be used in a hwmon name.
- *
- * Returns true if the char is invalid, false otherwise.
- */
-static inline bool hwmon_is_bad_char(const char ch)
-{
-	switch (ch) {
-	case '-':
-	case '*':
-	case ' ':
-	case '\t':
-	case '\n':
-		return true;
-	default:
-		return false;
-	}
-}
-
 #endif
-- 
2.30.2

RE: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[David Laight]

From: Michael Walle
> Sent: 29 March 2022 17:07
> 
> 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.

I'm assuming these 'bad' hwmon names come from userspace?
Like ethernet interface names??

Is silently changing the name of the hwmon entries the right
thing to do at all?

What happens if the user tries to create both "foo_bar" and "foo-bar"?
I'm sure that is going to go horribly wrong somewhere.

It would certainly make sense to have a function to verify the name
is actually valid.
Then bad names can be rejected earlier on.

I'm also intrigued about the list of invalid characters:

+static bool hwmon_is_bad_char(const char ch)
+{
+	switch (ch) {
+	case '-':
+	case '*':
+	case ' ':
+	case '\t':
+	case '\n':
+		return true;
+	default:
+		return false;
+	}
+}

If '\t' and '\n' are invalid why are all the other control characters
allowed?
I'm guessing '*' is disallowed because it is the shell wildcard?
So what about '?'.
Then I'd expect '/' to be invalid - but that isn't checked.
Never mind all the values 0x80 to 0xff - they are probably worse
than whitespace.

OTOH why are any characters invalid at all - except '/'?

	David

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

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Guenter Roeck]

On 3/29/22 19:57, David Laight wrote:
> From: Michael Walle
>> Sent: 29 March 2022 17:07
>>
>> 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.
> 
> I'm assuming these 'bad' hwmon names come from userspace?
> Like ethernet interface names??
> 
> Is silently changing the name of the hwmon entries the right
> thing to do at all?
> 
> What happens if the user tries to create both "foo_bar" and "foo-bar"?
> I'm sure that is going to go horribly wrong somewhere.
> 
> It would certainly make sense to have a function to verify the name
> is actually valid.
> Then bad names can be rejected earlier on.
> 
> I'm also intrigued about the list of invalid characters:
> 
> +static bool hwmon_is_bad_char(const char ch)
> +{
> +	switch (ch) {
> +	case '-':
> +	case '*':
> +	case ' ':
> +	case '\t':
> +	case '\n':
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> 
> If '\t' and '\n' are invalid why are all the other control characters
> allowed?
> I'm guessing '*' is disallowed because it is the shell wildcard?
> So what about '?'.
> Then I'd expect '/' to be invalid - but that isn't checked.
> Never mind all the values 0x80 to 0xff - they are probably worse
> than whitespace.
> 
> OTOH why are any characters invalid at all - except '/'?
> 

The name is supposed to reflect a driver name. Usually driver names
are not defined by userspace but by driver authors. The name is used
by libsensors to distinguish a driver from its instantiation.
libsensors uses wildcards in /etc/sensors3.conf. Duplicate names
are expected; there can be many instances of the same driver in
the system. For example, on the system I am typing this on, I have:

/sys/class/hwmon/hwmon0/name:nvme
/sys/class/hwmon/hwmon1/name:nvme
/sys/class/hwmon/hwmon2/name:nouveau
/sys/class/hwmon/hwmon3/name:nct6797
/sys/class/hwmon/hwmon4/name:jc42
/sys/class/hwmon/hwmon5/name:jc42
/sys/class/hwmon/hwmon6/name:jc42
/sys/class/hwmon/hwmon7/name:jc42
/sys/class/hwmon/hwmon8/name:k10temp

hwmon_is_bad_char() filters out characters which interfere with
libsensor's view of driver instances and the configuration data
in /etc/sensors3.conf. For example, again on my system, the
"sensors" command reports the following jc42 and nvme sensors.

jc42-i2c-0-1a
jc42-i2c-0-18
jc42-i2c-0-1b
jc42-i2c-0-19
nvme-pci-0100
nvme-pci-2500

In /etc/sensors3.conf, there might be entries for "jc42-*" or "nvme-*".
I don't think libsensors cares if a driver is named "this/is/my/driver".
That driver would then, assuming it is an i2c driver, show up
with the sensors command as "this/is/my/driver-i2c-0-25" or similar.
If it is named "this%is%my%driver", it would be something like
"this%is%my%driver-i2c-0-25". And so on. We can not permit "jc-42"
because libsensors would not be able to parse something like
"jc-42-*" or "jc-42-i2c-*".

Taking your example, if driver authors implement two drivers, one
named foo-bar and the other foo_bar, it would be the driver authors'
responsibility to provide valid driver names to the hwmon subsystem,
whatever those names might be. If both end up named "foo_bar" and can
as result not be distinguished from each other by libsensors,
or a user of the "sensors" command, that would be entirely the
responsibility of the driver authors. The only involvement of the
hwmon subsystem - and that is optional - would be to provide means
to the drivers to help them ensure that the names are valid, but
not that they are unique.

If there is ever a driver with a driver name that interferes with
libsensors' ability to distinguish the driver name from interface/port
information, we'll be happy to add the offending character(s)
to hwmon_is_bad_char(). Until then, being picky doesn't really
add any value and appears pointless.

Thanks,
Guenter

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Xu Yilun]

On Tue, Mar 29, 2022 at 06:07:26PM +0200, 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 |  9 ++++-
>  drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
>  include/linux/hwmon.h                    |  3 ++
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> index c41eb6108103..12f4a9bcef04 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
> @@ -93,7 +97,10 @@ removal would be too late.
>  
>  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.
> +will be rejected. The 'name' parameter is mandatory. Before calling a
> +register function you should either use hwmon_sanitize_name or
> +devm_hwmon_sanitize_name to replace any invalid characters with an

I suggest                   to duplicate the name and replace ...

Thanks,
Yilun

> +underscore.
>  
>  Using devm_hwmon_device_register_with_info()
>  --------------------------------------------
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 989e2c8496dd..619ef9f9a16e 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -1057,6 +1057,55 @@ 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.
> + *
> + * 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 v2 2/5] hwmon: intel-m10-bmc-hwmon: use devm_hwmon_sanitize_name()

[Xu Yilun]

On Tue, Mar 29, 2022 at 06:07:27PM +0200, Michael Walle wrote:
> 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

RE: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[David Laight]

From: Guenter Roeck
> Sent: 30 March 2022 04:47
> 
> On 3/29/22 19:57, David Laight wrote:
> > From: Michael Walle
> >> Sent: 29 March 2022 17:07
> >>
> >> 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.
> >
> > I'm assuming these 'bad' hwmon names come from userspace?
> > Like ethernet interface names??
> >
> > Is silently changing the name of the hwmon entries the right
> > thing to do at all?
> >
> > What happens if the user tries to create both "foo_bar" and "foo-bar"?
> > I'm sure that is going to go horribly wrong somewhere.
> >
> > It would certainly make sense to have a function to verify the name
> > is actually valid.
> > Then bad names can be rejected earlier on.
> >
> > I'm also intrigued about the list of invalid characters:
> >
> > +static bool hwmon_is_bad_char(const char ch)
> > +{
> > +	switch (ch) {
> > +	case '-':
> > +	case '*':
> > +	case ' ':
> > +	case '\t':
> > +	case '\n':
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> >
> > If '\t' and '\n' are invalid why are all the other control characters
> > allowed?
> > I'm guessing '*' is disallowed because it is the shell wildcard?
> > So what about '?'.
> > Then I'd expect '/' to be invalid - but that isn't checked.
> > Never mind all the values 0x80 to 0xff - they are probably worse
> > than whitespace.
> >
> > OTOH why are any characters invalid at all - except '/'?
> >
> 
> The name is supposed to reflect a driver name. Usually driver names
> are not defined by userspace but by driver authors. The name is used
> by libsensors to distinguish a driver from its instantiation.
> libsensors uses wildcards in /etc/sensors3.conf. Duplicate names
> are expected; there can be many instances of the same driver in
> the system. For example, on the system I am typing this on, I have:
> 
> /sys/class/hwmon/hwmon0/name:nvme
> /sys/class/hwmon/hwmon1/name:nvme
> /sys/class/hwmon/hwmon2/name:nouveau
> /sys/class/hwmon/hwmon3/name:nct6797
> /sys/class/hwmon/hwmon4/name:jc42
> /sys/class/hwmon/hwmon5/name:jc42
> /sys/class/hwmon/hwmon6/name:jc42
> /sys/class/hwmon/hwmon7/name:jc42
> /sys/class/hwmon/hwmon8/name:k10temp
> 
> hwmon_is_bad_char() filters out characters which interfere with
> libsensor's view of driver instances and the configuration data
> in /etc/sensors3.conf. For example, again on my system, the
> "sensors" command reports the following jc42 and nvme sensors.
> 
> jc42-i2c-0-1a
> jc42-i2c-0-18
> jc42-i2c-0-1b
> jc42-i2c-0-19
> nvme-pci-0100
> nvme-pci-2500
> 
> In /etc/sensors3.conf, there might be entries for "jc42-*" or "nvme-*".
> I don't think libsensors cares if a driver is named "this/is/my/driver".
> That driver would then, assuming it is an i2c driver, show up
> with the sensors command as "this/is/my/driver-i2c-0-25" or similar.
> If it is named "this%is%my%driver", it would be something like
> "this%is%my%driver-i2c-0-25". And so on. We can not permit "jc-42"
> because libsensors would not be able to parse something like
> "jc-42-*" or "jc-42-i2c-*".
> 
> Taking your example, if driver authors implement two drivers, one
> named foo-bar and the other foo_bar, it would be the driver authors'
> responsibility to provide valid driver names to the hwmon subsystem,
> whatever those names might be. If both end up named "foo_bar" and can
> as result not be distinguished from each other by libsensors,
> or a user of the "sensors" command, that would be entirely the
> responsibility of the driver authors. The only involvement of the
> hwmon subsystem - and that is optional - would be to provide means
> to the drivers to help them ensure that the names are valid, but
> not that they are unique.
> 
> If there is ever a driver with a driver name that interferes with
> libsensors' ability to distinguish the driver name from interface/port
> information, we'll be happy to add the offending character(s)
> to hwmon_is_bad_char(). Until then, being picky doesn't really
> add any value and appears pointless.

So actually, the only one of the characters that is actually
likely at all is '-'.
And even that can be deemed to be an error in the caller?
Or a 'bug' in the libsensors code - which could itself treat '-' as '_'.

So why not error the request to created the hwmon device with
an invalid name.
The name supplied will soon get fixed - since it is a literal
string in the calling driver.

	David

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

RE: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[David Laight]

From: Xu Yilun
> Sent: 30 March 2022 07:51
> 
> On Tue, Mar 29, 2022 at 06:07:26PM +0200, 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 |  9 ++++-
> >  drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
> >  include/linux/hwmon.h                    |  3 ++
> >  3 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> > index c41eb6108103..12f4a9bcef04 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
> > @@ -93,7 +97,10 @@ removal would be too late.
> >
> >  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.
> > +will be rejected. The 'name' parameter is mandatory. Before calling a
> > +register function you should either use hwmon_sanitize_name or
> > +devm_hwmon_sanitize_name to replace any invalid characters with an
> 
> I suggest                   to duplicate the name and replace ...

You are now going to get code that passed in NULL when the kmalloc() fails.
If 'sanitizing' the name is the correct thing to do then sanitize it
when the copy is made into the allocated structure.
(I'm assuming that the 'const char *name' parameter doesn't have to
be persistent - that would be another bug just waiting to happen.)

Seems really pointless to be do a kmalloc() just to pass a string
into a function.

	David

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

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Andrew Lunn]

> So why not error the request to created the hwmon device with
> an invalid name.
> The name supplied will soon get fixed - since it is a literal
> string in the calling driver.

It is often not a literal string in the driver, but something based on
the DT description of the hardware.

    Andrew

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Guenter Roeck]

On 3/29/22 09:07, 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 |  9 ++++-
>   drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
>   include/linux/hwmon.h                    |  3 ++
>   3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> index c41eb6108103..12f4a9bcef04 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
> @@ -93,7 +97,10 @@ removal would be too late.
>   
>   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.
> +will be rejected. The 'name' parameter is mandatory. Before calling a
> +register function you should either use hwmon_sanitize_name or
> +devm_hwmon_sanitize_name to replace any invalid characters with an
> +underscore.

That needs more details and deserves its own paragraph. Calling one of
the functions is only necessary if the original name does or can include
unsupported characters; an unconditional "should" is therefore a bit
strong. Also, it is important to mention that the function duplicates
the name, and that it is the responsibility of the caller to release
the name if hwmon_sanitize_name() was called and the device is removed.

Thanks,
Guenter

RE: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[David Laight]

From: Guenter Roeck
> Sent: 30 March 2022 15:23
> On 3/29/22 09:07, 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 |  9 ++++-
> >   drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
> >   include/linux/hwmon.h                    |  3 ++
> >   3 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> > index c41eb6108103..12f4a9bcef04 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
> > @@ -93,7 +97,10 @@ removal would be too late.
> >
> >   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.
> > +will be rejected. The 'name' parameter is mandatory. Before calling a
> > +register function you should either use hwmon_sanitize_name or
> > +devm_hwmon_sanitize_name to replace any invalid characters with an
> > +underscore.
> 
> That needs more details and deserves its own paragraph. Calling one of
> the functions is only necessary if the original name does or can include
> unsupported characters; an unconditional "should" is therefore a bit
> strong. Also, it is important to mention that the function duplicates
> the name, and that it is the responsibility of the caller to release
> the name if hwmon_sanitize_name() was called and the device is removed.

More worrying, and not documented, is that the buffer 'name' points
to must persist.

ISTM that the kmalloc() in __hwmon_device_register() should include
space for a copy of the name.
It can then do what it will with whatever is passed in.

Oh yes, it has my 'favourite construct':  if (!strlen(name)) ...
(well str[strlen(str)] = 0 also happens!)

	David

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

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Xu Yilun]

On Wed, Mar 30, 2022 at 10:11:39AM +0000, David Laight wrote:
> From: Xu Yilun
> > Sent: 30 March 2022 07:51
> > 
> > On Tue, Mar 29, 2022 at 06:07:26PM +0200, 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 |  9 ++++-
> > >  drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
> > >  include/linux/hwmon.h                    |  3 ++
> > >  3 files changed, 60 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> > > index c41eb6108103..12f4a9bcef04 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
> > > @@ -93,7 +97,10 @@ removal would be too late.
> > >
> > >  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.
> > > +will be rejected. The 'name' parameter is mandatory. Before calling a
> > > +register function you should either use hwmon_sanitize_name or
> > > +devm_hwmon_sanitize_name to replace any invalid characters with an
> > 
> > I suggest                   to duplicate the name and replace ...
> 
> You are now going to get code that passed in NULL when the kmalloc() fails.
> If 'sanitizing' the name is the correct thing to do then sanitize it
> when the copy is made into the allocated structure.

Then the driver is unaware of the name change, which makes more
confusing.

> (I'm assuming that the 'const char *name' parameter doesn't have to
> be persistent - that would be another bug just waiting to happen.)

The hwmon core does require a persistent "name" parameter now. No name
copy is made when hwmon dev register.

> 
> Seems really pointless to be do a kmalloc() just to pass a string
> into a function.

Maybe we should not force a kmalloc() when the sanitizing is needed, let
the driver decide whether to duplicate the string or not.

Thanks,
Yilun

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

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Guenter Roeck]

On 3/30/22 07:50, David Laight wrote:
> From: Guenter Roeck
>> Sent: 30 March 2022 15:23
>> On 3/29/22 09:07, 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 |  9 ++++-
>>>    drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
>>>    include/linux/hwmon.h                    |  3 ++
>>>    3 files changed, 60 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
>>> index c41eb6108103..12f4a9bcef04 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
>>> @@ -93,7 +97,10 @@ removal would be too late.
>>>
>>>    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.
>>> +will be rejected. The 'name' parameter is mandatory. Before calling a
>>> +register function you should either use hwmon_sanitize_name or
>>> +devm_hwmon_sanitize_name to replace any invalid characters with an
>>> +underscore.
>>
>> That needs more details and deserves its own paragraph. Calling one of
>> the functions is only necessary if the original name does or can include
>> unsupported characters; an unconditional "should" is therefore a bit
>> strong. Also, it is important to mention that the function duplicates
>> the name, and that it is the responsibility of the caller to release
>> the name if hwmon_sanitize_name() was called and the device is removed.
> 
> More worrying, and not documented, is that the buffer 'name' points
> to must persist.
> 

You mean the name argument passed to the hwmon registration functions ?
That has been the case forever, and I don't recall a single problem
with it. If it disturbs you, please feel free to submit a patch adding
more details to the documentation.

I would not want to change the code and always copy the name because in
almost all cases it _is_ a fixed string, and duplicating it would have
no value.

> ISTM that the kmalloc() in __hwmon_device_register() should include
> space for a copy of the name.
> It can then do what it will with whatever is passed in.
> 

Whatever is passed in is what the user wants. Registration functions
don't change the name. Providing a valid name is the responsibility
of the caller.

> Oh yes, it has my 'favourite construct':  if (!strlen(name)) ...
> (well str[strlen(str)] = 0 also happens!)
> 

Sorry, I don't understand what the problem is here.

Thanks,
Guenter

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Guenter Roeck]

On 3/30/22 07:51, Xu Yilun wrote:
> On Wed, Mar 30, 2022 at 10:11:39AM +0000, David Laight wrote:
>> From: Xu Yilun
>>> Sent: 30 March 2022 07:51
>>>
>>> On Tue, Mar 29, 2022 at 06:07:26PM +0200, 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 |  9 ++++-
>>>>   drivers/hwmon/hwmon.c                    | 49 ++++++++++++++++++++++++
>>>>   include/linux/hwmon.h                    |  3 ++
>>>>   3 files changed, 60 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
>>>> index c41eb6108103..12f4a9bcef04 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
>>>> @@ -93,7 +97,10 @@ removal would be too late.
>>>>
>>>>   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.
>>>> +will be rejected. The 'name' parameter is mandatory. Before calling a
>>>> +register function you should either use hwmon_sanitize_name or
>>>> +devm_hwmon_sanitize_name to replace any invalid characters with an
>>>
>>> I suggest                   to duplicate the name and replace ...
>>
>> You are now going to get code that passed in NULL when the kmalloc() fails.
>> If 'sanitizing' the name is the correct thing to do then sanitize it
>> when the copy is made into the allocated structure.
> 
> Then the driver is unaware of the name change, which makes more
> confusing.
> 
>> (I'm assuming that the 'const char *name' parameter doesn't have to
>> be persistent - that would be another bug just waiting to happen.)
> 
> The hwmon core does require a persistent "name" parameter now. No name
> copy is made when hwmon dev register.
> 
>>
>> Seems really pointless to be do a kmalloc() just to pass a string
>> into a function.
> 
> Maybe we should not force a kmalloc() when the sanitizing is needed, let
> the driver decide whether to duplicate the string or not.
> 

Drivers can do that today, and in all existing cases they do so
(which is why I had suggested to handle the duplication in the
convenience function in the first place). Drivers don't _have_
to use the provided convenience functions. At the same time,
convenience functions should cover the most common use cases.

Michael, let's just drop the changes outside drivers/hwmon from
the series, and let's keep hwmon_is_bad_char() in the include file.
Let's just document it, explaining its use case.

Code outside drivers/hwmon can then be independently modified or not
at a later time, based on driver author and/or maintainer preference.

Thanks,
Guenter

Re: [PATCH v2 3/5] net: sfp: use hwmon_sanitize_name()

[Russell King (Oracle)]

On Tue, Mar 29, 2022 at 06:07:28PM +0200, Michael Walle wrote:
> Instead of open-coding the bad characters replacement in the hwmon name,
> use the new hwmon_sanitize_name().
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Assuming hwmon_sanitize_name() gets settled, then:

Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

> ---
>  drivers/net/phy/sfp.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 4dfb79807823..0d5dba30444d 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -1289,7 +1289,7 @@ static const struct hwmon_chip_info sfp_hwmon_chip_info = {
>  static void sfp_hwmon_probe(struct work_struct *work)
>  {
>  	struct sfp *sfp = container_of(work, struct sfp, hwmon_probe.work);
> -	int err, i;
> +	int err;
>  
>  	/* hwmon interface needs to access 16bit registers in atomic way to
>  	 * guarantee coherency of the diagnostic monitoring data. If it is not
> @@ -1317,16 +1317,12 @@ static void sfp_hwmon_probe(struct work_struct *work)
>  		return;
>  	}
>  
> -	sfp->hwmon_name = kstrdup(dev_name(sfp->dev), GFP_KERNEL);
> +	sfp->hwmon_name = hwmon_sanitize_name(dev_name(sfp->dev));
>  	if (!sfp->hwmon_name) {
>  		dev_err(sfp->dev, "out of memory for hwmon name\n");
>  		return;
>  	}
>  
> -	for (i = 0; sfp->hwmon_name[i]; i++)
> -		if (hwmon_is_bad_char(sfp->hwmon_name[i]))
> -			sfp->hwmon_name[i] = '_';
> -
>  	sfp->hwmon_dev = hwmon_device_register_with_info(sfp->dev,
>  							 sfp->hwmon_name, sfp,
>  							 &sfp_hwmon_chip_info,
> -- 
> 2.30.2
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Russell King (Oracle)]

On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote:
> Michael, let's just drop the changes outside drivers/hwmon from
> the series, and let's keep hwmon_is_bad_char() in the include file.
> Let's just document it, explaining its use case.

Why? There hasn't been any objection to the change. All the discussion
seems to be around the new function (this patch) rather than the actual
conversions in drivers.

I'm entirely in favour of cleaning this up - it irks me that we're doing
exactly the same cleanup everywhere we have a hwmon.

At the very least, I would be completely in favour of keeping the
changes in the sfp and phy code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 3/5] net: sfp: use hwmon_sanitize_name()

[Russell King (Oracle)]

On Tue, Mar 29, 2022 at 06:07:28PM +0200, Michael Walle wrote:
> Instead of open-coding the bad characters replacement in the hwmon name,
> use the new hwmon_sanitize_name().
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Michael Walle]

Am 2022-03-31 16:45, schrieb Russell King (Oracle):
> On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote:
>> Michael, let's just drop the changes outside drivers/hwmon from
>> the series, and let's keep hwmon_is_bad_char() in the include file.
>> Let's just document it, explaining its use case.
> 
> Why? There hasn't been any objection to the change. All the discussion
> seems to be around the new function (this patch) rather than the actual
> conversions in drivers.
> 
> I'm entirely in favour of cleaning this up - it irks me that we're 
> doing
> exactly the same cleanup everywhere we have a hwmon.
> 
> At the very least, I would be completely in favour of keeping the
> changes in the sfp and phy code.

FWIW, my plan was to send the hwmon patches first, by then my other
series (the polynomial_calc() one) will also be ready to be picked.
Then I'd ask Guenter for a stable branch with these two series which
hopefully get merged into net-next. Then I can repost the missing
patches on net-next along with the new sensors support for the GPY
and LAN8814 PHYs.

For the last patch, if it should be applied or not, or when, that
will be up to Guenter then.

-michael

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Russell King (Oracle)]

On Thu, Mar 31, 2022 at 04:51:47PM +0200, Michael Walle wrote:
> Am 2022-03-31 16:45, schrieb Russell King (Oracle):
> > On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote:
> > > Michael, let's just drop the changes outside drivers/hwmon from
> > > the series, and let's keep hwmon_is_bad_char() in the include file.
> > > Let's just document it, explaining its use case.
> > 
> > Why? There hasn't been any objection to the change. All the discussion
> > seems to be around the new function (this patch) rather than the actual
> > conversions in drivers.
> > 
> > I'm entirely in favour of cleaning this up - it irks me that we're doing
> > exactly the same cleanup everywhere we have a hwmon.
> > 
> > At the very least, I would be completely in favour of keeping the
> > changes in the sfp and phy code.
> 
> FWIW, my plan was to send the hwmon patches first, by then my other
> series (the polynomial_calc() one) will also be ready to be picked.
> Then I'd ask Guenter for a stable branch with these two series which
> hopefully get merged into net-next. Then I can repost the missing
> patches on net-next along with the new sensors support for the GPY
> and LAN8814 PHYs.

Okay, that's fine. It just sounded like the conversion of other drivers
outside drivers/hwmon was being dropped.

Note that there's another "sanitisation" of hwmon names in
drivers/net/phy/marvell.c - that converts any non-alnum character to
an underscore. Not sure why the different approach was chosen there.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Michael Walle]

Am 2022-03-31 16:58, schrieb Russell King (Oracle):

> Note that there's another "sanitisation" of hwmon names in
> drivers/net/phy/marvell.c - that converts any non-alnum character to
> an underscore. Not sure why the different approach was chosen there.

I saw that, but I didn't touch it because it would change the
name. I guess that will be an incompatible userspace change.

-michael

Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

[Guenter Roeck]

On 3/31/22 07:58, Russell King (Oracle) wrote:
> On Thu, Mar 31, 2022 at 04:51:47PM +0200, Michael Walle wrote:
>> Am 2022-03-31 16:45, schrieb Russell King (Oracle):
>>> On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote:
>>>> Michael, let's just drop the changes outside drivers/hwmon from
>>>> the series, and let's keep hwmon_is_bad_char() in the include file.
>>>> Let's just document it, explaining its use case.
>>>
>>> Why? There hasn't been any objection to the change. All the discussion
>>> seems to be around the new function (this patch) rather than the actual
>>> conversions in drivers.
>>>
>>> I'm entirely in favour of cleaning this up - it irks me that we're doing
>>> exactly the same cleanup everywhere we have a hwmon.
>>>
>>> At the very least, I would be completely in favour of keeping the
>>> changes in the sfp and phy code.
>>
>> FWIW, my plan was to send the hwmon patches first, by then my other
>> series (the polynomial_calc() one) will also be ready to be picked.
>> Then I'd ask Guenter for a stable branch with these two series which
>> hopefully get merged into net-next. Then I can repost the missing
>> patches on net-next along with the new sensors support for the GPY
>> and LAN8814 PHYs.
> 
> Okay, that's fine. It just sounded like the conversion of other drivers
> outside drivers/hwmon was being dropped.
> 

Not dropped, just disconnected. From hwmon perspective, we want a certain
set of characters to be dropped or replaced. Also, from hwmon perspective,
it makes sense to have the helper function allocate the replacement data.
There was disagreement about which characters should be replaced, and if
the helper function should allocate the replacement string or not.
I have no intention to change the set of characters without good reason,
and I feel quite strongly about allocating the replacement in the helper
function. Since potential callers don't _have_ to use the helper and don't
_have_ to provide valid names (and are responsible for the consequences),
I would like that discussion to be separate from hwmon changes.

> Note that there's another "sanitisation" of hwmon names in
> drivers/net/phy/marvell.c - that converts any non-alnum character to
> an underscore. Not sure why the different approach was chosen there.
> 

It actually drops non-alphanumeric characters. The name is derived
from the phy device name, which I think is derived from the name field
in struct phy_driver. That includes spaces and '(', ')'. I honestly
have no idea what libsensors would do with '(' and ')'. Either case,
even if that would create a hiccup in libsensors and we would add
'(' and ')' to the 'forbidden' list of characters, the fact that the
code doesn't replace but drop non-alphanumeric characters means
it won't be able to use a helper anyway since that would result
in a hwmon 'name' attribute change and thus not be backward
compatible. Besides, "Marvell_88E1111__Finisar_" would look a bit
odd anyway, and "Marvell88E1111Finisar" may be at least slightly
better.

Guenter