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

[Guenter Roeck]

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

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

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

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

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

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

Guenter