linux-kernel - Re: [PATCH net-next] net: wan: framer: Simplify API framer_provider_simple_of

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <692ac4bc-aa97-4fea-9f40-bad7339e9474@icloud.com>
Date: Fri, 13 Dec 2024 18:57:15 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH net-next] net: wan: framer: Simplify API
 framer_provider_simple_of_xlate() implementation

On 2024/12/13 00:41, Simon Horman wrote:
>>  struct framer *framer_provider_simple_of_xlate(struct device *dev,
>>  					       const struct of_phandle_args *args)
>>  {
>> -	struct class_dev_iter iter;
>> -	struct framer *framer;
>> -
>> -	class_dev_iter_init(&iter, &framer_class, NULL, NULL);
>> -	while ((dev = class_dev_iter_next(&iter))) {
>> -		framer = dev_to_framer(dev);
>> -		if (args->np != framer->dev.of_node)
>> -			continue;
>> +	struct device *target_dev;
>>  
>> -		class_dev_iter_exit(&iter);
>> -		return framer;
>> +	target_dev = class_find_device_by_of_node(&framer_class, args->np);
>> +	if (target_dev) {
>> +		put_device(target_dev);
>> +		return dev_to_framer(target_dev);
>>  	}
>>  
>> -	class_dev_iter_exit(&iter);
>>  	return ERR_PTR(-ENODEV);
> Hi Zijun Hu,
> 
> FWIIW, I think it would be more idiomatic to have the non-error path in the
> main flow of execution, something like this (completely untested!):
> 
> 	target_dev = class_find_device_by_of_node(&framer_class, args->np);
> 	if (!target_dev)
> 		return ERR_PTR(-ENODEV);
> 
> 	put_device(target_dev);
> 	return dev_to_framer(target_dev);
> 
thank you Simon for code review.
good suggestion. let me take it in v2.

> Also, is it safe to put_device(target_dev) before
> passing target_dev to dev_to_framer() ?

Successful class_find_device_by_of_node() invocation will increase the
refcount of @target_dev, so i put_device() here to keep the same logic
as original.

thank you.