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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
> 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
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 <[email protected]>
> > ---
> > 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)
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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> > > ---
> > > 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)
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)
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> > ---
> > 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)
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!
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
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
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
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!