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@xxxxxxxxxxxxxxxx>
Thanks!
