Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 4 Apr 2022 16:31:08 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v3 1/2] hwmon: introduce hwmon_sanitize_name() |
| |
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@walle.cc> >> --- >> 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
| ![\](/images/icornerr.gif) |