Re: [PATCH] hwmon: (core) Use device name as a fallback in devm_hwmon_device_register_with_info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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")))

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.

> Guenter
> 

Heiner




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux