linux-kernel - Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <73faa3c8-70b9-4c95-b42a-5932249881e9@linux.dev>
Date: Tue, 8 Apr 2025 11:10:15 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
 devicetree@...r.kernel.org, Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
 Daniel Scally <djrscally@...il.com>, linux-kernel@...r.kernel.org,
 Danilo Krummrich <dakr@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH 2/2] device property: Add
 fwnode_property_get_reference_optional_args

On 4/8/25 04:39, Andy Shevchenko wrote:
> On Mon, Apr 07, 2025 at 06:37:14PM -0400, Sean Anderson wrote:
>> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
>> nargs_prop to be absent from the referenced node. This improves
>> compatibility for references where the devicetree might not always have
>> nargs_prop.
> 
> ...
> 
>> +/**
>> + * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments
>> + * @fwnode:	Firmware node where to look for the reference
>> + * @prop:	The name of the property
>> + * @nargs_prop:	The name of the property telling the number of
> 
> Use space instead of TAB as it's already too long to make it aligned with the
> rest.
> 
>> + *		arguments in the referred node.
>> + * @index:	Index of the reference, from zero onwards.
>> + * @args:	Result structure with reference and integer arguments.
>> + *		May be NULL.
>> + *
>> + * Obtain a reference based on a named property in an fwnode, with
>> + * integer arguments. If @nargs_prop is absent from the referenced node, then
>> + * number of arguments is be assumed to be 0.
>> + *
>> + * The caller is responsible for calling fwnode_handle_put() on the returned
>> + * @args->fwnode pointer.
>> + *
>> + * Return: %0 on success
>> + *	    %-ENOENT when the index is out of bounds, the index has an empty
>> + *		     reference or the property was not found
>> + *	    %-EINVAL on parse error
>> + */
>> +int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode,
>> +						const char *prop,
>> +						const char *nargs_prop,
>> +						unsigned int index,
>> +						struct fwnode_reference_args *args)
>> +{
>> +	int ret;
> 
>> +	if (IS_ERR_OR_NULL(fwnode))
>> +		return -ENOENT;
> 
> This is incorrect most likely, see below.
> 
>> +	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
>> +				 0, index, args);
>> +	if (ret == 0)
>> +		return ret;
>> +
>> +	if (IS_ERR_OR_NULL(fwnode->secondary))
>> +		return ret;
> 
> Here no such error code shadowing, and TBH I do not like the shadowing without
> real need.

I don't understand the objection. First, this logic is identical to
fwnode_property_get_reference_args. Second, the process seems clear to
me:

- If we have a primary fwnode, try it otherwise return -ENOENT
- If we have a secondary fwnode and the first failed, try it otherwise
  return the original error code

The purpose of a secondary fwnode is to allow supplying missing
properties absent from the primary fwnode. Which part of the above do
you dislike?

--Sean