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

[PATCH v1 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.

I'm not sure how to handle this correctly, as this touches both the
network tree and the hwmon tree. Also, the GPY PHY temperature senors
driver would use it.

Michael Walle (2):
  hwmon: introduce hwmon_sanitize_name()
  net: phy: use hwmon_sanitize_name()

 drivers/hwmon/intel-m10-bmc-hwmon.c |  5 +----
 drivers/net/phy/nxp-tja11xx.c       |  5 +----
 drivers/net/phy/sfp.c               |  6 ++----
 include/linux/hwmon.h               | 16 ++++++++++++++++
 4 files changed, 20 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH v1 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>
---
 drivers/hwmon/intel-m10-bmc-hwmon.c |  5 +----
 include/linux/hwmon.h               | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
index 7a08e4c44a4b..e6e55fc30153 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)
@@ -532,9 +531,7 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
 	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_sanitize_name(hw->hw_name);
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, hw->hw_name,
 							 hw, &hw->chip, NULL);
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index eba380b76d15..210b8c0b2827 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -484,4 +484,20 @@ static inline bool hwmon_is_bad_char(const char ch)
 	}
 }
 
+/**
+ * hwmon_sanitize_name - Replaces invalid characters in a hwmon name
+ * @name: NUL-terminated name
+ *
+ * Invalid characters in the name will be overwritten in-place by an
+ * underscore.
+ */
+static inline void hwmon_sanitize_name(char *name)
+{
+	while (*name) {
+		if (hwmon_is_bad_char(*name))
+			*name = '_';
+		name++;
+	};
+}
+
 #endif
-- 
2.30.2

[PATCH v1 2/2] net: phy: use hwmon_sanitize_name()

[Michael Walle]

Instead of open-coding it, use the new hwmon_sanitize_name() in th
nxp-tja11xx and sfp driver.

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

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index 9944cc501806..3973aa1b90dc 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -444,15 +444,12 @@ 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);
 	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] = '_';
+	hwmon_sanitize_name(priv->hwmon_name);
 
 	priv->hwmon_dev =
 		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4dfb79807823..35a4641303eb 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
@@ -1323,9 +1323,7 @@ static void sfp_hwmon_probe(struct work_struct *work)
 		return;
 	}
 
-	for (i = 0; sfp->hwmon_name[i]; i++)
-		if (hwmon_is_bad_char(sfp->hwmon_name[i]))
-			sfp->hwmon_name[i] = '_';
+	hwmon_sanitize_name(sfp->hwmon_name);
 
 	sfp->hwmon_dev = hwmon_device_register_with_info(sfp->dev,
 							 sfp->hwmon_name, sfp,
-- 
2.30.2

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

[Tom Rix]

On 3/28/22 4:52 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>
> ---
>   drivers/hwmon/intel-m10-bmc-hwmon.c |  5 +----
>   include/linux/hwmon.h               | 16 ++++++++++++++++
>   2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
> index 7a08e4c44a4b..e6e55fc30153 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)
> @@ -532,9 +531,7 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
>   	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_sanitize_name(hw->hw_name);
>   
>   	hwmon_dev = devm_hwmon_device_register_with_info(dev, hw->hw_name,
>   							 hw, &hw->chip, NULL);
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index eba380b76d15..210b8c0b2827 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -484,4 +484,20 @@ static inline bool hwmon_is_bad_char(const char ch)
>   	}
>   }

hwmon_is_bad_char is now only used by hwmon_sanitize_name.

as patch 3, consolidate into only hwmon_sanitize_name.

Tom

>   
> +/**
> + * hwmon_sanitize_name - Replaces invalid characters in a hwmon name
> + * @name: NUL-terminated name
> + *
> + * Invalid characters in the name will be overwritten in-place by an
> + * underscore.
> + */
> +static inline void hwmon_sanitize_name(char *name)
> +{
> +	while (*name) {
> +		if (hwmon_is_bad_char(*name))
> +			*name = '_';
> +		name++;
> +	};
> +}
> +
>   #endif

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

[Andrew Lunn]

> I'm not sure how to handle this correctly, as this touches both the
> network tree and the hwmon tree. Also, the GPY PHY temperature senors
> driver would use it.

There are a few options:

1) Get the hwmon_sanitize_name() merged into hwmon, ask for a stable
branch, and get it merged into netdev net-next.

2) Have the hwmon maintainers ACK the change and agree that it can be
merged via netdev.

Probably the second option is easiest, and since it is not touching
the core of hwmon, it is unlikely to cause merge conflicts.

    Andrew

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

[Guenter Roeck]

On 3/28/22 05:56, Andrew Lunn wrote:
>> I'm not sure how to handle this correctly, as this touches both the
>> network tree and the hwmon tree. Also, the GPY PHY temperature senors
>> driver would use it.
> 
> There are a few options:
> 
> 1) Get the hwmon_sanitize_name() merged into hwmon, ask for a stable
> branch, and get it merged into netdev net-next.
> 
> 2) Have the hwmon maintainers ACK the change and agree that it can be
> merged via netdev.
> 
> Probably the second option is easiest, and since it is not touching
> the core of hwmon, it is unlikely to cause merge conflicts.
> 

No, it isn't the easiest solution because it also modifies a hwmon
driver to use it.

Guenter

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

[Guenter Roeck]

On 3/28/22 04:52, 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>
> ---
>   drivers/hwmon/intel-m10-bmc-hwmon.c |  5 +----
>   include/linux/hwmon.h               | 16 ++++++++++++++++

Separate patches please, and the new function needs to be documented
in Documentation/hwmon/hwmon-kernel-api.rst.

Thanks,
Guenter

>   2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
> index 7a08e4c44a4b..e6e55fc30153 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)
> @@ -532,9 +531,7 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
>   	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_sanitize_name(hw->hw_name);
>   
>   	hwmon_dev = devm_hwmon_device_register_with_info(dev, hw->hw_name,
>   							 hw, &hw->chip, NULL);
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index eba380b76d15..210b8c0b2827 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -484,4 +484,20 @@ static inline bool hwmon_is_bad_char(const char ch)
>   	}
>   }
>   
> +/**
> + * hwmon_sanitize_name - Replaces invalid characters in a hwmon name
> + * @name: NUL-terminated name
> + *
> + * Invalid characters in the name will be overwritten in-place by an
> + * underscore.
> + */
> +static inline void hwmon_sanitize_name(char *name)
> +{
> +	while (*name) {
> +		if (hwmon_is_bad_char(*name))
> +			*name = '_';
> +		name++;
> +	};
> +}
> +
>   #endif

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

[Guenter Roeck]

On 3/28/22 05:25, Tom Rix wrote:
> 
> On 3/28/22 4:52 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>
>> ---
>>   drivers/hwmon/intel-m10-bmc-hwmon.c |  5 +----
>>   include/linux/hwmon.h               | 16 ++++++++++++++++
>>   2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
>> index 7a08e4c44a4b..e6e55fc30153 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)
>> @@ -532,9 +531,7 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
>>       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_sanitize_name(hw->hw_name);
>>       hwmon_dev = devm_hwmon_device_register_with_info(dev, hw->hw_name,
>>                                hw, &hw->chip, NULL);
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index eba380b76d15..210b8c0b2827 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -484,4 +484,20 @@ static inline bool hwmon_is_bad_char(const char ch)
>>       }
>>   }
> 
> hwmon_is_bad_char is now only used by hwmon_sanitize_name.
> 
> as patch 3, consolidate into only hwmon_sanitize_name.
> 

That would make the code look messy.

The function isn't execution-time critical, and neither is hwmon_sanitize_name().
I would suggest to implement hwmon_sanitize_name() in the hwmon core.
The existing static inline can then be removed from the include file
after all callers have been converted.

Thanks,
Guenter

Re: [PATCH v1 2/2] net: phy: use hwmon_sanitize_name()

[Michael Walle]

Am 2022-03-28 13:52, schrieb Michael Walle:
> Instead of open-coding it, use the new hwmon_sanitize_name() in th

s/th/the/ btw, will be fixed in the next version.

> nxp-tja11xx and sfp driver.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/phy/nxp-tja11xx.c | 5 +----
>  drivers/net/phy/sfp.c         | 6 ++----

Andrew, do you also prefer two seperate patches?

-michael

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

[Michael Walle]

Am 2022-03-28 18:27, schrieb Guenter Roeck:
> On 3/28/22 05:56, Andrew Lunn wrote:
>>> I'm not sure how to handle this correctly, as this touches both the
>>> network tree and the hwmon tree. Also, the GPY PHY temperature senors
>>> driver would use it.
>> 
>> There are a few options:
>> 
>> 1) Get the hwmon_sanitize_name() merged into hwmon, ask for a stable
>> branch, and get it merged into netdev net-next.
>> 
>> 2) Have the hwmon maintainers ACK the change and agree that it can be
>> merged via netdev.
>> 
>> Probably the second option is easiest, and since it is not touching
>> the core of hwmon, it is unlikely to cause merge conflicts.
>> 
> 
> No, it isn't the easiest solution because it also modifies a hwmon
> driver to use it.

So that leaves us with option 1? The next version will contain the
additional patch which moves the hwmon_is_bad_char() from the include
to the core and make it private. That will then need an immutable
branch from netdev to get merged back into hwmon before that patch
can be applied, right?

-michael

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

[Jakub Kicinski]

On Tue, 29 Mar 2022 00:50:28 +0200 Michael Walle wrote:
> > No, it isn't the easiest solution because it also modifies a hwmon
> > driver to use it.  
> 
> So that leaves us with option 1? The next version will contain the
> additional patch which moves the hwmon_is_bad_char() from the include
> to the core and make it private. That will then need an immutable
> branch from netdev to get merged back into hwmon before that patch
> can be applied, right?

If anything immutable branch from hwmon that we can pull, because hwmon
is the home of the API, and netdev is just _a_ consumer.

Either way I think you can post the patch that adds the new helper
for review.

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

[Guenter Roeck]

On 3/28/22 15:50, Michael Walle wrote:
> Am 2022-03-28 18:27, schrieb Guenter Roeck:
>> On 3/28/22 05:56, Andrew Lunn wrote:
>>>> I'm not sure how to handle this correctly, as this touches both the
>>>> network tree and the hwmon tree. Also, the GPY PHY temperature senors
>>>> driver would use it.
>>>
>>> There are a few options:
>>>
>>> 1) Get the hwmon_sanitize_name() merged into hwmon, ask for a stable
>>> branch, and get it merged into netdev net-next.
>>>
>>> 2) Have the hwmon maintainers ACK the change and agree that it can be
>>> merged via netdev.
>>>
>>> Probably the second option is easiest, and since it is not touching
>>> the core of hwmon, it is unlikely to cause merge conflicts.
>>>
>>
>> No, it isn't the easiest solution because it also modifies a hwmon
>> driver to use it.
> 
> So that leaves us with option 1? The next version will contain the
> additional patch which moves the hwmon_is_bad_char() from the include
> to the core and make it private. That will then need an immutable
> branch from netdev to get merged back into hwmon before that patch
> can be applied, right?

We can not control if someone else starts using the function before
it is removed. As pointed out, the immutable branch needs to be from hwmon,
and the patch to make hwmon_is_bad_char private can only be applied
after all of its users are gone from the mainline kernel.

I would actually suggest to allocate the new string as part of the
function and have it return a pointer to a new string. Something like
	char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
and
         char *hwmon_sanitize_name(const char *name);

because the string duplication is also part of all calling code.

Guenter