[PATCH 1/9] of: property: add of_graph_get_next_port()

[Tomi Valkeinen]

Hi,

On 09/08/2024 05:10, Kuninori Morimoto wrote:
> 
> Hi Tomi
> 
> Thank you for your review
> 
> 
>>> +/**
>>> + * of_graph_get_next_ports() - get next ports node.
>>> + * @parent: pointer to the parent device node
>>> + * @ports: current ports node, or NULL to get first
>>> + *
>>> + * Return: A 'ports' node pointer with refcount incremented. Refcount
>>> + * of the passed @prev node is decremented.
>>
>> No "prev" argument in the code.
>>
>> The of_graph_get_next_endpoint() function uses "previous" as the
>> argument name (well, the function declaration uses "previous", the
>> implementation uses "prev"...), and I would use the same naming here.
>>
>> Also of_graph_get_next_endpoint() talks about "previous endpoint node",
>> whereas here it's "current ports node". I'd use the same style here, so
>> "previous ports node".
>>
>> The same comments for the of_graph_get_next_port().
> 
> OK, thanks. Will fix in v2
> 
>>> +struct device_node *of_graph_get_next_ports(struct device_node *parent,
>>> +					    struct device_node *ports)
>>> +{
>>> +	if (!parent)
>>> +		return NULL;
>>> +
>>> +	if (!ports) {
>>> +		ports = of_get_child_by_name(parent, "ports");
>>> +
>>> +		/* use parent as its ports of this device if it not exist */
>>
>> I think this needs to be described in the kernel doc. I understand the
>> need for this, but it's somewhat counter-intuitive that this returns the
>> parent node if there are no ports nodes, so it must be highlighted in
>> the documentation.
>>
>> I wonder if a bit more complexity here would be good... I think here we
>> could:
>>
>> - If there are no 'ports' nodes in the parent, but there is a 'port'
>> node in the parent, return the parent node
>> - If there are no 'ports' nor 'port' nodes in the parent, return NULL
> 
> Thanks, but unfortunately, get_next_ports() checks only "ports" and
> doesn't check whether it has "port" node or not.

Yes, my point was if the check should be added. My reasoning is:

If we have this, of_graph_get_next_ports() returns the ports at 0, and that 
makes sense:

parent {
	ports at 0 {
		port at 0 { };
	};
};

If we have this, of_graph_get_next_ports() returns the parent, and 
that's a bit surprising, but I can see the need, and it just needs to be 
documented:

parent {
	port { };
};

But if we have this, does it make sense that of_graph_get_next_ports() 
returns the parent, or should it return NULL:

parent {
	/* No ports or port */
};

> So correct comment here is maybe...
> 
> 	If "parent" doesn't have "ports", it returns "parent" itself as "ports"
> 
> I will add it on v2
> 
>>> +/**
>>> + * of_graph_get_next_port() - get next port node.
>>> + * @parent: pointer to the parent device node
>>> + * @port: current port node, or NULL to get first
>>> + *
>>> + * Return: A 'port' node pointer with refcount incremented. Refcount
>>> + * of the passed @prev node is decremented.
>>> + */
>>> +struct device_node *of_graph_get_next_port(struct device_node *parent,
>>> +					   struct device_node *port)
>>> +{
>>> +	if (!parent)
>>> +		return NULL;
>>> +
>>> +	if (!port) {
>>> +		struct device_node *ports __free(device_node) =
>>> +			of_graph_get_next_ports(parent, NULL);
>>> +
>>> +		return of_get_child_by_name(ports, "port");
>>> +	}
>>> +
>>> +	do {
>>> +		port = of_get_next_child(parent, port);
>>> +		if (!port)
>>> +			break;
>>> +	} while (!of_node_name_eq(port, "port"));
>>> +
>>> +	return port;
>>> +}
>>
>> Hmm... So if I call this with of_graph_get_next_port(dev_node, NULL)
>> (dev_node being the device node of the device), it'll give me the first
>> port in the first ports node, or the first port in the dev_node if there
>> are no ports nodes?
> 
> Yes
> 
>> And if I then continue iterating with of_graph_get_next_port(dev_node,
>> prev_port)... The call will return NULL if the dev_node contains "ports"
>> node (because the dev_node does not contain any "port" nodes)?
>>
>> So if I understand right, of_graph_get_next_port() must always be called
>> with a parent that contains port nodes. Sometimes that's the device's
>> node (if there's just one port) and sometimes that's ports node. If it's
>> called with a parent that contains ports node, it will not work correctly.
>>
>> If the above is right, then should this just return
>> "of_get_child_by_name(parent, "port")" if !port, instead of calling
>> of_graph_get_next_ports()?
> 
> Hmm ?  Do you mean you want access to ports at 1 memeber port after ports at 0 ?
> I have tested below in my test environment
> 
> 	parent {
> (X)		ports at 0 {
> (A)			port at 0 { };
> (B)			port at 1 { };
> 		};
> (Y)		ports at 1 {
> (C)			port at 0 { };
> 		};
> 	};
> 
> In this case, if you call get_next_port() with parent,
> you can get ports at 0 member port.
> 
> 	/* use "paramet" and use "ports at 0" are same result */
> 
> 	// use parent
> 	port = of_graph_get_next_port(parent, NULL); // (A)
> 	port = of_graph_get_next_port(parent, port); // (B)
> 	port = of_graph_get_next_port(parent, port); // NULl
> 
> 	// use ports at 0
> 	ports = of_graph_get_next_ports(parent, NULL); // (X)
> 	port  = of_graph_get_next_port(ports, NULL);   // (A)
> 	port  = of_graph_get_next_port(ports, port);   // (B)
> 	port  = of_graph_get_next_port(ports, port);   // NULl
> 
> If you want to get ports at 1 member port, you need to use ports at 1.
> 
> 	// use ports at 1
> 	ports = of_graph_get_next_ports(parent, NULL);  // (X)
> 	ports = of_graph_get_next_ports(parent, ports); // (Y)
> 	port  = of_graph_get_next_port(ports, NULL);    // (C)
> 
> I have confirmed in my test environment.
> But please double check it. Is this clear for you ?

Ah, I see now. I was expecting of_get_next_child() to return children of 
'parent', but that's actually not the case.

So when you call:

	port = of_graph_get_next_port(parent, NULL); // (A)

the function will call 'ports = of_graph_get_next_ports(parent, NULL)' 
(ports is (X)) and then return of_get_child_by_name(ports, "port") 
(which is (A)). This is fine.

	port = of_graph_get_next_port(parent, port); // (B)

Here the function will call 'port = of_get_next_child(parent, port)', 
where parent is the parent node, and port is (A). The problem is, (A) is 
not a child of the parent. This seems to work, as of_get_next_child() 
does not use the 'parent' for anything if 'port' is given, instead if 
just gives the next sibling of 'port'.

	port = of_graph_get_next_port(parent, port); // NULl

And now when the function calls of_get_next_child(parent, port), it does 
not give the next child of parent, but instead NULL because 'port' has 
no more siblings.

The documentation for of_get_next_child() doesn't mention if calling 
of_get_next_child(parent, child) is valid when the given child is 
actually not a child of the parent. The doc says that 'prev' parameter 
should be "previous child of the parent node", which is not the case 
here. So using it like this does not sound right to me.

And just looking at the behavior of:

 > 	port = of_graph_get_next_port(parent, NULL); // (A)
 > 	port = of_graph_get_next_port(parent, port); // (B)
 > 	port = of_graph_get_next_port(parent, port); // NULl

it does not feel right. Why does of_graph_get_next_port() return only 
the ports of ports at 0? I think it should either return nothing, as there 
are no 'port' nodes in the parent, or it should return all the port 
nodes from all the ports nodes.

Can we just drop the use of of_graph_get_next_ports() from 
of_graph_get_next_port()? In other words, make of_graph_get_next_port() 
iterate the 'port' nodes strictly only in the given parent node.

I think we have the same problem in of_graph_get_next_ports(). If we have:

parent {
	port { };
};

And we do:

ports = of_graph_get_next_ports(parent, NULL)

The returned 'ports' is actually the 'parent'. If we then call:

ports = of_graph_get_next_ports(parent, ports)

we are effectively calling of_graph_get_next_ports(parent, parent). This 
results in of_get_next_child(parent, parent). of_get_next_child() will 
return the next sibling of parent (so, perhaps, a node for some 
unrelated device). It then checks if the name of that node is 'ports'. 
So, while unlikely, if we have:

bus {
	/* our display device */
	display {
		port { };
	};

	/* some odd ports device */
	ports {
	};
};

and you use of_graph_get_next_ports() for display, you'll end up getting 
the 'ports' node.

I have to say, I feel that making the 'ports' node optional in the graph 
DT bindings was a mistake, but nothing we can do about that...

Can you try adding "WARN_ON(node && prev && node != prev->parent)" to 
of_get_next_child()?

>> Or maybe I'm just getting confused here. But in any case, I think it
>> would be very good to describe the behavior on the kernel doc for the
>> different ports/port structure cases (also for
>> of_graph_get_next_ports()), and be clear on what the parameters can be,
>> i.e. what kind of device nodes can be given as parent, and how the
>> function iterates over the ports.
> 
> OK, will do in v2
> 
>>> + * @np: pointer to the parent device node
>>> + *
>>> + * Return: count of port of this device node
>>> + */
>>> +unsigned int of_graph_get_port_count(struct device_node *np)
>>> +{
>>> +	struct device_node *port = NULL;
>>> +	int num = 0;
>>> +
>>> +	for_each_of_graph_port(np, port)
>>> +		num++;
>>> +
>>> +	return num;
>>> +}
>>
>> I my analysis above is right, calling of_graph_get_port_count(dev_node)
>> will return 1, if the dev_node contains "ports" node which contains one
>> or more "port" nodes.
> 
> In my test, it will be..
> 
> 	parent {
> 		ports at 0 {
> 			port at 0 { };
> 			port at 1 { };
> 		};
> 		ports at 1 {
> 			port at 0 { };
> 		};
> 	};
> 
> 	of_graph_get_port_count(parent); // 2 = number of ports at 0

I think the above is a bit surprising, and in my opinion points that 
there is a problem. Why does using 'parent' equate to only using 
'ports at 0'? Again, I would expect either to get 0 (as there are no 'port' 
nodes in parent, or 3.

> 	of_graph_get_port_count(ports0); // 2 = number of ports at 0
> 	of_graph_get_port_count(ports1); // 1 = number of ports at 1
> 
> 
> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto

  Tomi