On Thu, Jan 09, 2025 at 07:24:32PM +0100, Heiner Kallweit wrote: > On 09.01.2025 19:00, Guenter Roeck wrote: > > On Thu, Jan 09, 2025 at 06:03:17PM +0100, Heiner Kallweit wrote: > >> A number of network PHY drivers use the following code: > >> > >> name = devm_hwmon_sanitize_name(dev, dev_name(dev)); > >> if (IS_ERR(name)) > >> return PTR_ERR(name); > >> devm_hwmon_device_register_with_info(dev, name, ..); > >> > >> Make this a generic fallback option and use the device name if no name > >> is provided to devm_hwmon_device_register_with_info(). This would allow > >> to simplify the affected drivers. > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> > >> --- > >> drivers/hwmon/hwmon.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > >> index 685d4ce8d..1fd3d94e1 100644 > >> --- a/drivers/hwmon/hwmon.c > >> +++ b/drivers/hwmon/hwmon.c > >> @@ -1170,6 +1170,11 @@ devm_hwmon_device_register_with_info(struct device *dev, const char *name, > >> if (!dev) > >> return ERR_PTR(-EINVAL); > >> > >> + if (!name) > >> + name = devm_hwmon_sanitize_name(dev, dev_name(dev)); > >> + if (IS_ERR(name)) > >> + return ERR_CAST(name); > >> + > > > > That introduces an undiscussed change: It handles an ERR_PTR() > > passed as name parameter, not just NULL. Why would that be warranted ? > > It may not look like much, but it introduces an inconsistency: Other > > pointers (dev, chip) are not checked against ERR_PTR(). It is also not > > immediately obvious to me why the driver would want to check if the > > passed name is an ER_PTR(). > > > If a caller would pass an ERRPTR as name, we'd be in trouble in > __hwmon_device_register(), when the following code is executed: > if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) > Just like we are in trouble if the caller passes an ERR_PTR() as any of the other pointer arguments. I see that as a bug in the calling code. > By doing the IS_ERR() check outside the if(!name) clause we get > a little bit more of argument checking for free, at least with > no code overhead. > > If that's not wanted I can also move the check into the > if(!name) clause. Please do. Guenter
