| Message ID | 20211220210533.3578678-1-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | software node: fix wrong node passed to find nargs_prop | expand |
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 > >
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
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.
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 > > > >
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)
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 >>> >>> > > >
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.
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. :-)
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
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
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)
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(-)