Re: [PATCH v3 07/14] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally <djrscally@gmail.com>]

On 24/12/2020 12:53, Laurent Pinchart wrote:
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		/*
>> +		 * ports have naming style "port@n", so we search for children
>> +		 * that follow that convention (but without assuming anything
>> +		 * about the index number)
>> +		 */
>> +		if (!strncmp(to_swnode(port)->node->name, "port@",
>> +			     FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> 
> I would either add a macro to replace the prefix ("port@"), or drop
> FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both
> worlds, the string and its length are defined in two different places
> :-)
> 
> I would personally drop the macro, but I don't mind either way as long
> as the string and its length are defined in the same place.

OK, pending outcome of the discussion in the other thread I'll do both
things the same way - whatever the decision there is.


>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	/* Ports have naming style "port@n", we need to select the n */
>> +	ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>> +			10, &endpoint->port);
> 
> Same here.
> 
> I wonder if we should add a check to ensure parent->node->name is long
> enough (and possibly even start with the right prefix), as otherwise the
> pointer passed to kstrtou32() may be past the end of the string. Maybe
> this is overkill, if we can rely on the fact that software nodes have
> correct names.

Not necessarily actually; ports yes but endpoints no, so I think the
danger is there. I'll add the check.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!