[v2,06/12] software_node: Add support for fwnode_graph*() family of functions

Commit Message

Daniel Scally Dec. 17, 2020, 11:43 p.m. UTC

From: Heikki Krogerus <[e-mail]>

This implements the remaining .graph_* callbacks in the
fwnode operations structure for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.

The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port@" (for example "port@0", "port@1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.

The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint".

Signed-off-by: Heikki Krogerus <[e-mail]>
Co-developed-by: Daniel Scally <[e-mail]>
Signed-off-by: Daniel Scally <[e-mail]>
---
Changes in v2:

	- Changed commit to specify port name prefix as port@
	- Accounted for that rename in *parse_endpoint()

 drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Dec. 18, 2020, 4:22 p.m. UTC | #1

Hi Daniel,

Thank you for the patch.

On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <[e-mail]>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".
> 
> Signed-off-by: Heikki Krogerus <[e-mail]>
> Co-developed-by: Daniel Scally <[e-mail]>
> Signed-off-by: Daniel Scally <[e-mail]>
> ---
> Changes in v2:
> 
> 	- Changed commit to specify port name prefix as port@
> 	- Accounted for that rename in *parse_endpoint()
> 
>  drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2b90d380039b..0d14d5ebe441 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +			    struct fwnode_handle *port)
> +{
> +	struct fwnode_handle *old = port;
> +
> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))

Maybe we'll need to limit this to matching on "port" or "port@[0-9]+" to
avoid false positives, but that can be done later, if needed.

> +			return port;
> +		old = port;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +				      struct fwnode_handle *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *old = endpoint;
> +	struct fwnode_handle *parent;
> +	struct fwnode_handle *port;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	if (endpoint) {
> +		port = software_node_get_parent(endpoint);

Here the reference count to port is incremented.

> +		parent = software_node_get_parent(port);
> +	} else {
> +		parent = software_node_get_named_child_node(fwnode, "ports");
> +		if (!parent)
> +			parent = software_node_get(&swnode->fwnode);
> +
> +		port = swnode_graph_find_next_port(parent, NULL);

But here it isn't, software_node_get_next_child() doesn't deal with
reference counts.

> +	}
> +
> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {

So if the loop terminates normally, the reference acquired in the first
branch of the if will be leaked.

> +		endpoint = software_node_get_next_child(port, old);
> +		if (endpoint) {
> +			fwnode_handle_put(port);

While in this case the reference not acquired in the second branch of
the if will be released incorrectly.

I think it's software_node_get_next_child() that needs to be fixed if
I'm not mistaken.

> +			break;
> +		}
> +
> +		/* No more endpoints for that port, so stop passing old */
> +		old = NULL;

I wonder if you could drop the 'old' variable and use 'enpoint' in the
call to software_node_get_next_child(). You could then drop these two
lines.

> +	}
> +
> +	fwnode_handle_put(parent);
> +
> +	return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct software_node_ref_args *ref;
> +	const struct property_entry *prop;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +		return NULL;
> +
> +	ref = prop->pointer;
> +
> +	return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;
> +
> +	return software_node_get(parent);
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	int ret;
> +
> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
> +	if (ret)
> +		return ret;
> +
> +	endpoint->id = swnode->id;
> +	endpoint->local_fwnode = fwnode;
> +
> +	return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>  	.get = software_node_get,
>  	.put = software_node_put,
> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>  	.get_parent = software_node_get_parent,
>  	.get_next_child_node = software_node_get_next_child,
>  	.get_named_child_node = software_node_get_named_child_node,
> -	.get_reference_args = software_node_get_reference_args
> +	.get_reference_args = software_node_get_reference_args,
> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +	.graph_get_port_parent = software_node_graph_get_port_parent,
> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>  
>  /* -------------------------------------------------------------------------- */

Andy Shevchenko Dec. 18, 2020, 8:37 p.m. UTC | #2

On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <[e-mail]>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".

...

> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> +			return port;
> +		old = port;
> +	}

Dunno if we need defines for port and its length here.

...

> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);

But here at least comment is needed what 5 means ('port@' I suppose).

> +	if (ret)
> +		return ret;

Daniel Scally Dec. 18, 2020, 10:13 p.m. UTC | #3

Hi Laurent - thanks for comments as always

On 18/12/2020 16:22, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>> From: Heikki Krogerus <[e-mail]>
>>
>> This implements the remaining .graph_* callbacks in the
>> fwnode operations structure for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port@" (for example "port@0", "port@1",
>> ...) and endpoints will use the index of the software node
>> that is given to them during creation. The port nodes can
>> also be grouped under a specially named "ports" subnode,
>> just like in DT, if necessary.
>>
>> The remote-endpoints are reference properties under the
>> endpoint nodes that are named "remote-endpoint".
>>
>> Signed-off-by: Heikki Krogerus <[e-mail]>
>> Co-developed-by: Daniel Scally <[e-mail]>
>> Signed-off-by: Daniel Scally <[e-mail]>
>> ---
>> Changes in v2:
>>
>> 	- Changed commit to specify port name prefix as port@
>> 	- Accounted for that rename in *parse_endpoint()
>>
>>  drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 2b90d380039b..0d14d5ebe441 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>>  	return 0;
>>  }
>>  
>> +static struct fwnode_handle *
>> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
>> +			    struct fwnode_handle *port)
>> +{
>> +	struct fwnode_handle *old = port;
>> +
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> 
> Maybe we'll need to limit this to matching on "port" or "port@[0-9]+" to
> avoid false positives, but that can be done later, if needed.

Hmm yeah I guess that's a danger - ok, I'll stick it on the list.


>> +			return port;
>> +		old = port;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>> +				      struct fwnode_handle *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	struct fwnode_handle *old = endpoint;
>> +	struct fwnode_handle *parent;
>> +	struct fwnode_handle *port;
>> +
>> +	if (!swnode)
>> +		return NULL;
>> +
>> +	if (endpoint) {
>> +		port = software_node_get_parent(endpoint);
> 
> Here the reference count to port is incremented.
> 
>> +		parent = software_node_get_parent(port);
>> +	} else {
>> +		parent = software_node_get_named_child_node(fwnode, "ports");
>> +		if (!parent)
>> +			parent = software_node_get(&swnode->fwnode);
>> +
>> +		port = swnode_graph_find_next_port(parent, NULL);
> 
> But here it isn't, software_node_get_next_child() doesn't deal with
> reference counts.

Not as in the kernel right now, but after patch one of this series, it does:

[PATCH v2 01/12] software_node: Fix refcounts in
software_node_get_next_child()

I'm not sure that one linked to the thread correctly, but it's here if
you haven't seen it:

https://lore.kernel.org/linux-media/[e-mail]/T/#u

The tl;dr of the change is that it will now get() the next node (if
found) and **always** put() if one is passed.


>> +	}
>> +
>> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
> 
> So if the loop terminates normally, the reference acquired in the first
> branch of the if will be leaked.
> 
>> +		endpoint = software_node_get_next_child(port, old);
>> +		if (endpoint) {
>> +			fwnode_handle_put(port);
> 
> While in this case the reference not acquired in the second branch of
> the if will be released incorrectly.
> 
> I think it's software_node_get_next_child() that needs to be fixed if
> I'm not mistaken.

I think that's all handled in software_node_get_next_child() as amended
by 01/12. The net effect of get_next_endpoint() should be one refcount
increased for any endpoint returned, and 0 change to parent and any ports.


>> +			break;
>> +		}
>> +
>> +		/* No more endpoints for that port, so stop passing old */
>> +		old = NULL;
> 
> I wonder if you could drop the 'old' variable and use 'enpoint' in the
> call to software_node_get_next_child(). You could then drop these two
> lines.

That won't work, because endpoint would at that point not be a child of
the port we're passing, and the function relies on it being one:

	if (!p || list_empty(&p->children) ||
	    (c && list_is_last(&c->entry, &p->children))) {
		fwnode_handle_put(child);
		return NULL;
	}

>> +	}
>> +
>> +	fwnode_handle_put(parent);
>> +
>> +	return endpoint;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	const struct software_node_ref_args *ref;
>> +	const struct property_entry *prop;
>> +
>> +	if (!swnode)
>> +		return NULL;
>> +
>> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
>> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
>> +		return NULL;
>> +
>> +	ref = prop->pointer;
>> +
>> +	return software_node_get(software_node_fwnode(ref[0].node));
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	struct fwnode_handle *parent;
>> +
>> +	if (!strcmp(swnode->parent->node->name, "ports"))
>> +		parent = &swnode->parent->parent->fwnode;
>> +	else
>> +		parent = &swnode->parent->fwnode;
>> +
>> +	return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	endpoint->id = swnode->id;
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct fwnode_operations software_node_ops = {
>>  	.get = software_node_get,
>>  	.put = software_node_put,
>> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>>  	.get_parent = software_node_get_parent,
>>  	.get_next_child_node = software_node_get_next_child,
>>  	.get_named_child_node = software_node_get_named_child_node,
>> -	.get_reference_args = software_node_get_reference_args
>> +	.get_reference_args = software_node_get_reference_args,
>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>  };
>>  
>>  /* -------------------------------------------------------------------------- */
>

Daniel Scally Dec. 18, 2020, 10:26 p.m. UTC | #4

On 18/12/2020 20:37, Andy Shevchenko wrote:
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>> From: Heikki Krogerus <[e-mail]>
>>
>> This implements the remaining .graph_* callbacks in the
>> fwnode operations structure for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port@" (for example "port@0", "port@1",
>> ...) and endpoints will use the index of the software node
>> that is given to them during creation. The port nodes can
>> also be grouped under a specially named "ports" subnode,
>> just like in DT, if necessary.
>>
>> The remote-endpoints are reference properties under the
>> endpoint nodes that are named "remote-endpoint".
> 
> ...
> 
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
>> +			return port;
>> +		old = port;
>> +	}
> 
> Dunno if we need defines for port and its length here.

Mmm, maybe a comment?

> ...
> 
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
> 
> But here at least comment is needed what 5 means ('port@' I suppose).

Ack - I'll add an explanatory comment (and yep, it's 'port@')

>> +	if (ret)
>> +		return ret;
> 
>

Daniel Scally Dec. 18, 2020, 11:46 p.m. UTC | #5

On 18/12/2020 22:13, Daniel Scally wrote:

>>> +			break;
>>> +		}
>>> +
>>> +		/* No more endpoints for that port, so stop passing old */
>>> +		old = NULL;
>>
>> I wonder if you could drop the 'old' variable and use 'enpoint' in the
>> call to software_node_get_next_child(). You could then drop these two
>> lines.
> 
> That won't work, because endpoint would at that point not be a child of
> the port we're passing, and the function relies on it being one:
> 
> 	if (!p || list_empty(&p->children) ||
> 	    (c && list_is_last(&c->entry, &p->children))) {
> 		fwnode_handle_put(child);
> 		return NULL;
> 	}
> 

Wait, that's nonsense of course, because endpoint gets set to NULL when
software_node_get_next_child() finds nothing - I'll double check but
pretty sure you're right.

Sakari Ailus Dec. 21, 2020, 9:34 a.m. UTC | #6

Hi Daniel and Heikki,

On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <[e-mail]>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".
> 
> Signed-off-by: Heikki Krogerus <[e-mail]>
> Co-developed-by: Daniel Scally <[e-mail]>
> Signed-off-by: Daniel Scally <[e-mail]>
> ---
> Changes in v2:
> 
> 	- Changed commit to specify port name prefix as port@
> 	- Accounted for that rename in *parse_endpoint()
> 
>  drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2b90d380039b..0d14d5ebe441 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +			    struct fwnode_handle *port)
> +{
> +	struct fwnode_handle *old = port;
> +
> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> +			return port;
> +		old = port;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +				      struct fwnode_handle *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *old = endpoint;
> +	struct fwnode_handle *parent;
> +	struct fwnode_handle *port;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	if (endpoint) {
> +		port = software_node_get_parent(endpoint);
> +		parent = software_node_get_parent(port);
> +	} else {
> +		parent = software_node_get_named_child_node(fwnode, "ports");
> +		if (!parent)
> +			parent = software_node_get(&swnode->fwnode);
> +
> +		port = swnode_graph_find_next_port(parent, NULL);
> +	}
> +
> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
> +		endpoint = software_node_get_next_child(port, old);
> +		if (endpoint) {
> +			fwnode_handle_put(port);
> +			break;
> +		}
> +
> +		/* No more endpoints for that port, so stop passing old */
> +		old = NULL;
> +	}
> +
> +	fwnode_handle_put(parent);
> +
> +	return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct software_node_ref_args *ref;
> +	const struct property_entry *prop;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +		return NULL;
> +
> +	ref = prop->pointer;
> +
> +	return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;

If you happen to call this function on a non-port node for whatever reason,
you may end up accessing a pointer that's NULL, can't you? Instead I'd do
something like:

swnode = swnode->parent;
if (swnode && !strcmp(swnode->node->name, "ports"))
	swnode = swnode->parent;

return swnode ? software_node_get(&swnode->fwnode) : NULL;

You can also drop parent as a by-product of this.

> +
> +	return software_node_get(parent);
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	int ret;
> +
> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
> +	if (ret)
> +		return ret;
> +
> +	endpoint->id = swnode->id;
> +	endpoint->local_fwnode = fwnode;
> +
> +	return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>  	.get = software_node_get,
>  	.put = software_node_put,
> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>  	.get_parent = software_node_get_parent,
>  	.get_next_child_node = software_node_get_next_child,
>  	.get_named_child_node = software_node_get_named_child_node,
> -	.get_reference_args = software_node_get_reference_args
> +	.get_reference_args = software_node_get_reference_args,
> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +	.graph_get_port_parent = software_node_graph_get_port_parent,
> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>  
>  /* -------------------------------------------------------------------------- */

Daniel Scally Dec. 21, 2020, 10:01 a.m. UTC | #7

Hi Sakari - thanks for the reviews in previous emails

On 21/12/2020 09:34, Sakari Ailus wrote:
> Hi Daniel and Heikki,
>
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;
> If you happen to call this function on a non-port node for whatever reason,
> you may end up accessing a pointer that's NULL, can't you?

Yes, actually.

> Instead I'd do
> something like:
>
> swnode = swnode->parent;
> if (swnode && !strcmp(swnode->node->name, "ports"))
> 	swnode = swnode->parent;
>
> return swnode ? software_node_get(&swnode->fwnode) : NULL;
>
> You can also drop parent as a by-product of this.
Yes ok, that looks good to me - thanks.
>> +
>> +	return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	endpoint->id = swnode->id;
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct fwnode_operations software_node_ops = {
>>  	.get = software_node_get,
>>  	.put = software_node_put,
>> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>>  	.get_parent = software_node_get_parent,
>>  	.get_next_child_node = software_node_get_next_child,
>>  	.get_named_child_node = software_node_get_named_child_node,
>> -	.get_reference_args = software_node_get_reference_args
>> +	.get_reference_args = software_node_get_reference_args,
>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>  };
>>  
>>  /* -------------------------------------------------------------------------- */

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 2b90d380039b..0d14d5ebe441 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,110 @@  software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+			    struct fwnode_handle *port)
+{
+	struct fwnode_handle *old = port;
+
+	while ((port = software_node_get_next_child(parent, old))) {
+		if (!strncmp(to_swnode(port)->node->name, "port", 4))
+			return port;
+		old = port;
+	}
+
+	return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+				      struct fwnode_handle *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *old = endpoint;
+	struct fwnode_handle *parent;
+	struct fwnode_handle *port;
+
+	if (!swnode)
+		return NULL;
+
+	if (endpoint) {
+		port = software_node_get_parent(endpoint);
+		parent = software_node_get_parent(port);
+	} else {
+		parent = software_node_get_named_child_node(fwnode, "ports");
+		if (!parent)
+			parent = software_node_get(&swnode->fwnode);
+
+		port = swnode_graph_find_next_port(parent, NULL);
+	}
+
+	for (; port; port = swnode_graph_find_next_port(parent, port)) {
+		endpoint = software_node_get_next_child(port, old);
+		if (endpoint) {
+			fwnode_handle_put(port);
+			break;
+		}
+
+		/* No more endpoints for that port, so stop passing old */
+		old = NULL;
+	}
+
+	fwnode_handle_put(parent);
+
+	return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	const struct software_node_ref_args *ref;
+	const struct property_entry *prop;
+
+	if (!swnode)
+		return NULL;
+
+	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+		return NULL;
+
+	ref = prop->pointer;
+
+	return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *parent;
+
+	if (!strcmp(swnode->parent->node->name, "ports"))
+		parent = &swnode->parent->parent->fwnode;
+	else
+		parent = &swnode->parent->fwnode;
+
+	return software_node_get(parent);
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+				   struct fwnode_endpoint *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	int ret;
+
+	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
+	if (ret)
+		return ret;
+
+	endpoint->id = swnode->id;
+	endpoint->local_fwnode = fwnode;
+
+	return 0;
+}
+
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
@@ -551,7 +655,11 @@  static const struct fwnode_operations software_node_ops = {
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
-	.get_reference_args = software_node_get_reference_args
+	.get_reference_args = software_node_get_reference_args,
+	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
+	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
+	.graph_get_port_parent = software_node_graph_get_port_parent,
+	.graph_parse_endpoint = software_node_graph_parse_endpoint,
 };
 
 /* -------------------------------------------------------------------------- */