Re: [PATCH v4 01/14] device property: Add fwnode_graph_get_port_by_id()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fwnode_graph_get_port_by_id() unexpectedly skips unconnected or disabled ports.
- [Low] Missing `const` qualifier on the `fwnode` parameter in the new fwnode graph API.
--

--- Patch [1]: [PATCH v4 01/14] device property: Add fwnode_graph_get_port_by_id() ---
commit 4c6b92ebbe0a97ba47216632078d3f24afd73cf4
Author: Chen-Yu Tsai <wenst@chromium.org>

device property: Add fwnode_graph_get_port_by_id()

This commit introduces a helper function, fwnode_graph_get_port_by_id(),
to retrieve a port firmware node matching a given ID. It is intended to 
be used for cases like USB power sequencing where a reference to the port 
firmware node is needed.

> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index e08eadd66f4f9..c5e7e00f1b0cd 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
[ ... ]
> @@ -1346,6 +1346,28 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
>  
> +/**
> + * fwnode_graph_get_port_by_id - get the port matching a given id
> + * @fwnode: parent fwnode_handle containing the graph
> + * @id: id of the port
> + *
> + * The caller is responsible for calling fwnode_handle_put() on the returned
> + * fwnode pointer.
> + *
> + * Return: A 'port' firmware node pointer with refcount incremented.
> + */
> +struct fwnode_handle *fwnode_graph_get_port_by_id(struct fwnode_handle *fwnode, u32 id)

[Severity: Low]
Should the fwnode parameter have a const qualifier?

fwnode_graph_get_endpoint_by_id() takes a const struct fwnode_handle *, so
there appears to be no technical reason for this function to require a mutable
pointer. Keeping it non-const forces callers holding a const pointer to cast
away constness to use this helper.

> +{
> +	struct fwnode_handle *ep;
> +
> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, id, 0, FWNODE_GRAPH_ENDPOINT_NEXT);

[Severity: High]
Does this unexpectedly skip unconnected or disabled ports?

Because the FWNODE_GRAPH_DEVICE_DISABLED flag is omitted here,
fwnode_graph_get_endpoint_by_id() will implicitly filter out all endpoints
where fwnode_graph_remote_available() is false. If a local port only contains
unconnected endpoints or its remote device is disabled, this will incorrectly
return NULL.

Will callers (like the upcoming USB power sequencing) fail to find local port
firmware nodes if the port happens to be unconnected?

> +	if (!ep)
> +		return NULL;
> +
> +	return fwnode_get_next_parent(ep);
> +}

> diff --git a/include/linux/property.h b/include/linux/property.h
> index 14c304db46648..e04901c0bd8f7 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
[ ... ]
> @@ -505,6 +505,7 @@ int fwnode_get_phy_mode(const struct fwnode_handle *fwnode);
>  
>  void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);
>  
> +struct fwnode_handle *fwnode_graph_get_port_by_id(struct fwnode_handle *fwnode, u32 id);

[Severity: Low]
Can we also update the exported header prototype to match, propagating the
const qualifier to API consumers?

>  struct fwnode_handle *fwnode_graph_get_next_endpoint(

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709095726.704448-1-wenst@chromium.org?part=1