[PATCH] software node: fix wrong node passed to find nargs_prop

[PATCH] software node: fix wrong node passed to find nargs_prop

[Clément Léger]

nargs_prop refers to a property located in the reference that is found
within the nargs property. Use the correct reference node in call to
property_entry_read_int_array() to retrieve the correct nargs value.

Fixes: b06184acf751 ("software node: Add software_node_get_reference_args()")
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/swnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 4debcea4fb12..0a482212c7e8 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -529,7 +529,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 		return -ENOENT;
 
 	if (nargs_prop) {
-		error = property_entry_read_int_array(swnode->node->properties,
+		error = property_entry_read_int_array(ref->node->properties,
 						      nargs_prop, sizeof(u32),
 						      &nargs_prop_val, 1);
 		if (error)
-- 
2.34.1

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Daniel Scally]

Thanks Andy

On 20/12/2021 22:13, Andy Shevchenko wrote:
> + Sakari, Dan
> 
> On Monday, December 20, 2021, Clément Léger <clement.leger@bootlin.com
> <mailto:clement.leger@bootlin.com>> wrote:
> 
>     nargs_prop refers to a property located in the reference that is found
>     within the nargs property.

I think this is right (it's not used in the ACPI version, and the OF
version is quite convoluted so a bit hard to follow)...but also I note
that none of the users of fwnode_property_get_reference_args() pass
anything to nargs_prop anyway...do we even need this?

Use the correct reference node in call to
>     property_entry_read_int_array() to retrieve the correct nargs value.
> 
>     Fixes: b06184acf751 ("software node: Add
>     software_node_get_reference_args()")

I think this might have been introduced later...maybe 996b0830f95d1,
maybe e933bedd45099

>     Signed-off-by: Clément Léger <clement.leger@bootlin.com
>     <mailto:clement.leger@bootlin.com>>
>     ---
>      drivers/base/swnode.c | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>     index 4debcea4fb12..0a482212c7e8 100644
>     --- a/drivers/base/swnode.c
>     +++ b/drivers/base/swnode.c
>     @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct
>     fwnode_handle *fwnode,
>                     return -ENOENT;
> 
>             if (nargs_prop) {
>     -               error =
>     property_entry_read_int_array(swnode->node->properties,
>     +               error =
>     property_entry_read_int_array(ref->node->properties,
>                                                           nargs_prop,
>     sizeof(u32),
>                                                          
>     &nargs_prop_val, 1);
>                     if (error)
>     -- 
>     2.34.1
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Greg Kroah-Hartman]

On Mon, Dec 20, 2021 at 11:37:07PM +0000, Daniel Scally wrote:
> Thanks Andy
> 
> On 20/12/2021 22:13, Andy Shevchenko wrote:
> > + Sakari, Dan
> > 
> > On Monday, December 20, 2021, Clément Léger <clement.leger@bootlin.com
> > <mailto:clement.leger@bootlin.com>> wrote:
> > 
> >     nargs_prop refers to a property located in the reference that is found
> >     within the nargs property.
> 
> I think this is right (it's not used in the ACPI version, and the OF
> version is quite convoluted so a bit hard to follow)...but also I note
> that none of the users of fwnode_property_get_reference_args() pass
> anything to nargs_prop anyway...do we even need this?

Looks like it is unused, please just remove it.

thanks,

greg k-h

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Sakari Ailus]

Hi Greg, Andy,

On Tue, Dec 21, 2021 at 10:34:11AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Dec 20, 2021 at 11:37:07PM +0000, Daniel Scally wrote:
> > Thanks Andy
> > 
> > On 20/12/2021 22:13, Andy Shevchenko wrote:
> > > + Sakari, Dan
> > > 
> > > On Monday, December 20, 2021, Clément Léger <clement.leger@bootlin.com
> > > <mailto:clement.leger@bootlin.com>> wrote:
> > > 
> > >     nargs_prop refers to a property located in the reference that is found
> > >     within the nargs property.
> > 
> > I think this is right (it's not used in the ACPI version, and the OF
> > version is quite convoluted so a bit hard to follow)...but also I note
> > that none of the users of fwnode_property_get_reference_args() pass
> > anything to nargs_prop anyway...do we even need this?
> 
> Looks like it is unused, please just remove it.

If you remove nargs_prop argument, then callers will have to use OF
property API instead to parse references with property-defined number of
arguments. The goal has been to cover all functionality in a
firmware-independent way.

-- 
Regards,

Sakari Ailus

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Clément Léger]

Le Mon, 20 Dec 2021 23:37:07 +0000,
Daniel Scally <djrscally@gmail.com> a écrit :

> Thanks Andy
> 
> On 20/12/2021 22:13, Andy Shevchenko wrote:
>  [...]  
> 
> I think this is right (it's not used in the ACPI version, and the OF
> version is quite convoluted so a bit hard to follow)...but also I note
> that none of the users of fwnode_property_get_reference_args() pass
> anything to nargs_prop anyway...do we even need this?

Indeed, this is currently not used anywhere, nargs is always used
instead of nargs_prop. The usage is meant to be (almost) the same as
of_parse_phandle_with_args().

ie:

 ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
				  index, &args);

can be replaced by:

 ret = fwnode_property_get_reference_args(node, "resets",
					  "#reset-cells", 0 index,
					  &args);

I have some patches that uses that with software nodes and that will
need this support.

> 
> Use the correct reference node in call to
> >     property_entry_read_int_array() to retrieve the correct nargs value.
> > 
> >     Fixes: b06184acf751 ("software node: Add
> >     software_node_get_reference_args()")  
> 
> I think this might have been introduced later...maybe 996b0830f95d1,
> maybe e933bedd45099

From what I saw, it was already in the original commit adding this but
I can be wrong.

> 
> >     Signed-off-by: Clément Léger <clement.leger@bootlin.com
> >     <mailto:clement.leger@bootlin.com>>
> >     ---
> >      drivers/base/swnode.c | 2 +-
> >      1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> >     diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> >     index 4debcea4fb12..0a482212c7e8 100644
> >     --- a/drivers/base/swnode.c
> >     +++ b/drivers/base/swnode.c
> >     @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct
> >     fwnode_handle *fwnode,
> >                     return -ENOENT;
> > 
> >             if (nargs_prop) {
> >     -               error =
> >     property_entry_read_int_array(swnode->node->properties,
> >     +               error =
> >     property_entry_read_int_array(ref->node->properties,
> >                                                           nargs_prop,
> >     sizeof(u32),
> >                                                          
> >     &nargs_prop_val, 1);
> >                     if (error)
> >     -- 
> >     2.34.1
> > 
> > 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> >   



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Sakari Ailus]

On Mon, Dec 20, 2021 at 10:05:33PM +0100, Clément Léger wrote:
> nargs_prop refers to a property located in the reference that is found
> within the nargs property. Use the correct reference node in call to
> property_entry_read_int_array() to retrieve the correct nargs value.
> 
> Fixes: b06184acf751 ("software node: Add software_node_get_reference_args()")
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

Thank you (and thanks to Andy for cc'ing me).

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> ---
>  drivers/base/swnode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 4debcea4fb12..0a482212c7e8 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  		return -ENOENT;
>  
>  	if (nargs_prop) {
> -		error = property_entry_read_int_array(swnode->node->properties,
> +		error = property_entry_read_int_array(ref->node->properties,
>  						      nargs_prop, sizeof(u32),
>  						      &nargs_prop_val, 1);
>  		if (error)

-- 
Sakari Ailus

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Daniel Scally]

Hello

On 21/12/2021 09:46, Clément Léger wrote:
> Le Mon, 20 Dec 2021 23:37:07 +0000,
> Daniel Scally <djrscally@gmail.com> a écrit :
> 
>> Thanks Andy
>>
>> On 20/12/2021 22:13, Andy Shevchenko wrote:
>>  [...]  
>>
>> I think this is right (it's not used in the ACPI version, and the OF
>> version is quite convoluted so a bit hard to follow)...but also I note
>> that none of the users of fwnode_property_get_reference_args() pass
>> anything to nargs_prop anyway...do we even need this?
> 
> Indeed, this is currently not used anywhere, nargs is always used
> instead of nargs_prop. The usage is meant to be (almost) the same as
> of_parse_phandle_with_args().
> 
> ie:
> 
>  ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
> 				  index, &args);
> 
> can be replaced by:
> 
>  ret = fwnode_property_get_reference_args(node, "resets",
> 					  "#reset-cells", 0 index,
> 					  &args);
> 
> I have some patches that uses that with software nodes and that will
> need this support.

Ok, in that case I think this is the right thing to do and you can have my:

Reviewed-by: Daniel Scally <djrscally@gmail.com>

Might be nice to transfer the function comment from
of_parse_phandle_with_args() to fwnode_property_get_reference_args(), as
that's nice and clear.

>>
>> Use the correct reference node in call to
>>>     property_entry_read_int_array() to retrieve the correct nargs value.
>>>
>>>     Fixes: b06184acf751 ("software node: Add
>>>     software_node_get_reference_args()")  
>>
>> I think this might have been introduced later...maybe 996b0830f95d1,
>> maybe e933bedd45099
> 
> From what I saw, it was already in the original commit adding this but
> I can be wrong.

You're right of course - sorry for the noise

>>
>>>     Signed-off-by: Clément Léger <clement.leger@bootlin.com
>>>     <mailto:clement.leger@bootlin.com>>
>>>     ---
>>>      drivers/base/swnode.c | 2 +-
>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>     diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>>     index 4debcea4fb12..0a482212c7e8 100644
>>>     --- a/drivers/base/swnode.c
>>>     +++ b/drivers/base/swnode.c
>>>     @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct
>>>     fwnode_handle *fwnode,
>>>                     return -ENOENT;
>>>
>>>             if (nargs_prop) {
>>>     -               error =
>>>     property_entry_read_int_array(swnode->node->properties,
>>>     +               error =
>>>     property_entry_read_int_array(ref->node->properties,
>>>                                                           nargs_prop,
>>>     sizeof(u32),
>>>                                                          
>>>     &nargs_prop_val, 1);
>>>                     if (error)
>>>     -- 
>>>     2.34.1
>>>
>>>
>>>
>>> -- 
>>> With Best Regards,
>>> Andy Shevchenko
>>>
>>>   
> 
> 
> 

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Daniel Scally]

Hi Sakari

On 21/12/2021 09:45, Sakari Ailus wrote:
> Hi Greg, Andy,
> 
> On Tue, Dec 21, 2021 at 10:34:11AM +0100, Greg Kroah-Hartman wrote:
>> On Mon, Dec 20, 2021 at 11:37:07PM +0000, Daniel Scally wrote:
>>> Thanks Andy
>>>
>>> On 20/12/2021 22:13, Andy Shevchenko wrote:
>>>> + Sakari, Dan
>>>>
>>>> On Monday, December 20, 2021, Clément Léger <clement.leger@bootlin.com
>>>> <mailto:clement.leger@bootlin.com>> wrote:
>>>>
>>>>     nargs_prop refers to a property located in the reference that is found
>>>>     within the nargs property.
>>>
>>> I think this is right (it's not used in the ACPI version, and the OF
>>> version is quite convoluted so a bit hard to follow)...but also I note
>>> that none of the users of fwnode_property_get_reference_args() pass
>>> anything to nargs_prop anyway...do we even need this?
>>
>> Looks like it is unused, please just remove it.
> 
> If you remove nargs_prop argument, then callers will have to use OF
> property API instead to parse references with property-defined number of
> arguments. The goal has been to cover all functionality in a
> firmware-independent way.

My mistake, I missed that of_parse_phandle_with_args() has a ton of
direct users. I guess we should try to replace those with
fwnode_property_get_reference_args() where appropriate.

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Sakari Ailus]

Hi Daniel,

On Tue, Dec 21, 2021 at 10:09:45PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 21/12/2021 09:45, Sakari Ailus wrote:
> > Hi Greg, Andy,
> > 
> > On Tue, Dec 21, 2021 at 10:34:11AM +0100, Greg Kroah-Hartman wrote:
> >> On Mon, Dec 20, 2021 at 11:37:07PM +0000, Daniel Scally wrote:
> >>> Thanks Andy
> >>>
> >>> On 20/12/2021 22:13, Andy Shevchenko wrote:
> >>>> + Sakari, Dan
> >>>>
> >>>> On Monday, December 20, 2021, Clément Léger <clement.leger@bootlin.com
> >>>> <mailto:clement.leger@bootlin.com>> wrote:
> >>>>
> >>>>     nargs_prop refers to a property located in the reference that is found
> >>>>     within the nargs property.
> >>>
> >>> I think this is right (it's not used in the ACPI version, and the OF
> >>> version is quite convoluted so a bit hard to follow)...but also I note
> >>> that none of the users of fwnode_property_get_reference_args() pass
> >>> anything to nargs_prop anyway...do we even need this?
> >>
> >> Looks like it is unused, please just remove it.
> > 
> > If you remove nargs_prop argument, then callers will have to use OF
> > property API instead to parse references with property-defined number of
> > arguments. The goal has been to cover all functionality in a
> > firmware-independent way.
> 
> My mistake, I missed that of_parse_phandle_with_args() has a ton of
> direct users. I guess we should try to replace those with
> fwnode_property_get_reference_args() where appropriate.

I'd say at least when the code is otherwise using fwnode property API.

I guess most of the reference users are OF-based originally while cameras
are perhaps a bit of an exception to this. :-)

-- 
Regards,

Sakari Ailus

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Heikki Krogerus]

On Mon, Dec 20, 2021 at 10:05:33PM +0100, Clément Léger wrote:
> nargs_prop refers to a property located in the reference that is found
> within the nargs property. Use the correct reference node in call to
> property_entry_read_int_array() to retrieve the correct nargs value.
> 
> Fixes: b06184acf751 ("software node: Add software_node_get_reference_args()")
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/base/swnode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 4debcea4fb12..0a482212c7e8 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  		return -ENOENT;
>  
>  	if (nargs_prop) {
> -		error = property_entry_read_int_array(swnode->node->properties,
> +		error = property_entry_read_int_array(ref->node->properties,
>  						      nargs_prop, sizeof(u32),
>  						      &nargs_prop_val, 1);
>  		if (error)
> -- 
> 2.34.1

-- 
heikki

Re: [PATCH] software node: fix wrong node passed to find nargs_prop

[Rafael J. Wysocki]

On Wed, Dec 22, 2021 at 12:19 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Dec 20, 2021 at 10:05:33PM +0100, Clément Léger wrote:
> > nargs_prop refers to a property located in the reference that is found
> > within the nargs property. Use the correct reference node in call to
> > property_entry_read_int_array() to retrieve the correct nargs value.
> >
> > Fixes: b06184acf751 ("software node: Add software_node_get_reference_args()")
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Applied as 5.17 material, thanks!

> > ---
> >  drivers/base/swnode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 4debcea4fb12..0a482212c7e8 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> >               return -ENOENT;
> >
> >       if (nargs_prop) {
> > -             error = property_entry_read_int_array(swnode->node->properties,
> > +             error = property_entry_read_int_array(ref->node->properties,
> >                                                     nargs_prop, sizeof(u32),
> >                                                     &nargs_prop_val, 1);
> >               if (error)
> > --
> > 2.34.1
>
> --
> heikki