Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] power: supply: register HWMON devices with valid names
@ 2019-08-22 15:09 Romain Izard
  2019-08-22 16:12 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Romain Izard @ 2019-08-22 15:09 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Andrey Smirnov, Guenter Roeck, linux-pm, linux-kernel, Romain Izard

With the introduction of the HWMON compatibility layer to the power
supply framework in Linux 5.3, all power supply devices' names can be
used directly to create HWMON devices with the same names.

But HWMON has rules on allowable names that are different from the power
supply framework. The dash character is forbidden, as it is used by the
libsensors library in userspace as a separator, whereas this character
is used in the device names in more than half of the existing power
supply drivers. This last case is consistent with the typical naming
usage with MFD and Device Tree.

This leads to warnings in the kernel log, with the format:

power_supply gpio-charger: hwmon: \
	'gpio-charger' is not a valid name attribute, please fix

Add a protection to power_supply_add_hwmon_sysfs() that replaces any
dash in the device name with an underscore when registering with the
HWMON framework. Other forbidden characters (star, slash, space, tab,
newline) are not replaced, as they are not in common use.

Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer")
Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/power/supply/power_supply_hwmon.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index 51fe60440d12..ebe964bd512c 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -284,6 +284,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 	struct device *dev = &psy->dev;
 	struct device *hwmon;
 	int ret, i;
+	const char *name;
 
 	if (!devres_open_group(dev, power_supply_add_hwmon_sysfs,
 			       GFP_KERNEL))
@@ -334,7 +335,19 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 		}
 	}
 
-	hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name,
+	name = psy->desc->name;
+	if (strchr(name, '-')) {
+		char *new_name;
+
+		new_name = devm_kstrdup(dev, name, GFP_KERNEL);
+		if (!new_name) {
+			ret = -ENOMEM;
+			goto error;
+		}
+		strreplace(new_name, '-', '_');
+		name = (const char *) new_name;
+	}
+	hwmon = devm_hwmon_device_register_with_info(dev, name,
 						psyhw,
 						&power_supply_hwmon_chip_info,
 						NULL);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] power: supply: register HWMON devices with valid names
  2019-08-22 15:09 [PATCH] power: supply: register HWMON devices with valid names Romain Izard
@ 2019-08-22 16:12 ` Guenter Roeck
  2019-08-23  9:03   ` Romain Izard
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-08-22 16:12 UTC (permalink / raw)
  To: Romain Izard; +Cc: Sebastian Reichel, Andrey Smirnov, linux-pm, linux-kernel

On Thu, Aug 22, 2019 at 05:09:19PM +0200, Romain Izard wrote:
> With the introduction of the HWMON compatibility layer to the power
> supply framework in Linux 5.3, all power supply devices' names can be
> used directly to create HWMON devices with the same names.
> 
> But HWMON has rules on allowable names that are different from the power
> supply framework. The dash character is forbidden, as it is used by the
> libsensors library in userspace as a separator, whereas this character
> is used in the device names in more than half of the existing power
> supply drivers. This last case is consistent with the typical naming
> usage with MFD and Device Tree.
> 
> This leads to warnings in the kernel log, with the format:
> 
> power_supply gpio-charger: hwmon: \
> 	'gpio-charger' is not a valid name attribute, please fix
> 
> Add a protection to power_supply_add_hwmon_sysfs() that replaces any
> dash in the device name with an underscore when registering with the
> HWMON framework. Other forbidden characters (star, slash, space, tab,
> newline) are not replaced, as they are not in common use.
> 
> Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer")
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  drivers/power/supply/power_supply_hwmon.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index 51fe60440d12..ebe964bd512c 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -284,6 +284,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>  	struct device *dev = &psy->dev;
>  	struct device *hwmon;
>  	int ret, i;
> +	const char *name;
>  
>  	if (!devres_open_group(dev, power_supply_add_hwmon_sysfs,
>  			       GFP_KERNEL))
> @@ -334,7 +335,19 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>  		}
>  	}
>  
> -	hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name,
> +	name = psy->desc->name;
> +	if (strchr(name, '-')) {
> +		char *new_name;
> +
> +		new_name = devm_kstrdup(dev, name, GFP_KERNEL);
> +		if (!new_name) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}
> +		strreplace(new_name, '-', '_');
> +		name = (const char *) new_name;

Is that typecast necessary ?

Other than that,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> +	}
> +	hwmon = devm_hwmon_device_register_with_info(dev, name,
>  						psyhw,
>  						&power_supply_hwmon_chip_info,
>  						NULL);
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] power: supply: register HWMON devices with valid names
  2019-08-22 16:12 ` Guenter Roeck
@ 2019-08-23  9:03   ` Romain Izard
  0 siblings, 0 replies; 3+ messages in thread
From: Romain Izard @ 2019-08-23  9:03 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Sebastian Reichel, Andrey Smirnov, linux-pm, linux-kernel

On Thu, Aug 22, 2019 at 09:12:07AM -0700, Guenter Roeck wrote:
> On Thu, Aug 22, 2019 at 05:09:19PM +0200, Romain Izard wrote:
> > With the introduction of the HWMON compatibility layer to the power
> > supply framework in Linux 5.3, all power supply devices' names can be
> > used directly to create HWMON devices with the same names.
> > 
> > But HWMON has rules on allowable names that are different from the power
> > supply framework. The dash character is forbidden, as it is used by the
> > libsensors library in userspace as a separator, whereas this character
> > is used in the device names in more than half of the existing power
> > supply drivers. This last case is consistent with the typical naming
> > usage with MFD and Device Tree.
> > 
> > This leads to warnings in the kernel log, with the format:
> > 
> > power_supply gpio-charger: hwmon: \
> > 	'gpio-charger' is not a valid name attribute, please fix
> > 
> > Add a protection to power_supply_add_hwmon_sysfs() that replaces any
> > dash in the device name with an underscore when registering with the
> > HWMON framework. Other forbidden characters (star, slash, space, tab,
> > newline) are not replaced, as they are not in common use.
> > 
> > Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer")
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > ---
> >  drivers/power/supply/power_supply_hwmon.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> > index 51fe60440d12..ebe964bd512c 100644
> > --- a/drivers/power/supply/power_supply_hwmon.c
> > +++ b/drivers/power/supply/power_supply_hwmon.c
> > @@ -284,6 +284,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
> >  	struct device *dev = &psy->dev;
> >  	struct device *hwmon;
> >  	int ret, i;
> > +	const char *name;
> >  
> >  	if (!devres_open_group(dev, power_supply_add_hwmon_sysfs,
> >  			       GFP_KERNEL))
> > @@ -334,7 +335,19 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
> >  		}
> >  	}
> >  
> > -	hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name,
> > +	name = psy->desc->name;
> > +	if (strchr(name, '-')) {
> > +		char *new_name;
> > +
> > +		new_name = devm_kstrdup(dev, name, GFP_KERNEL);
> > +		if (!new_name) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +		strreplace(new_name, '-', '_');
> > +		name = (const char *) new_name;
> 
> Is that typecast necessary ?

Indeed, it is not. I'll remove it if a V2 is necessary.
> 
> Other than that,
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> > +	}
> > +	hwmon = devm_hwmon_device_register_with_info(dev, name,
> >  						psyhw,
> >  						&power_supply_hwmon_chip_info,
> >  						NULL);

Thanks for your review.

Best regards,
-- 
Romain Izard

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-22 15:09 [PATCH] power: supply: register HWMON devices with valid names Romain Izard
2019-08-22 16:12 ` Guenter Roeck
2019-08-23  9:03   ` Romain Izard

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox