On Tue, May 05, 2026 at 04:21:45PM +0800, Guangshuo Li wrote:
> imx8qxp_pxl2dpi_get_available_ep_from_port() returns ERR_PTR()
> on errors. imx8qxp_pxl2dpi_find_next_bridge() stores its return
> value in a __free(device_node) variable before checking IS_ERR().
> When the function returns on the error path, the cleanup action calls
> of_node_put() on the ERR_PTR() value.
>
> Do not let a device_node cleanup variable hold error pointers. Return
> the error code from imx8qxp_pxl2dpi_get_available_ep_from_port()
> directly and pass the endpoint node through an output argument. This
> keeps the cleanup action operating only on NULL or a valid device_node,
> while preserving the existing error codes.
>
> Fixes: ceea3f7806a10 ("drm/bridge: imx8qxp-pxl2dpi: simplify put of
> device_node pointers")
> Cc: [email protected]
> Reviewed-by: Frank Li <[email protected]>
> Signed-off-by: Guangshuo Li <[email protected]>
> ---
> v4:
> - Drop the sentence mentioning the custom static analysis tool.
> - Add Frank's Reviewed-by tag.
> - No functional code changes.
>
> v3:
> - Do not change DEFINE_FREE(device_node, ...).
> - Fix the driver pattern by making
> imx8qxp_pxl2dpi_get_available_ep_from_port() return an int and
> pass the endpoint via an output argument.
> - Update both callers so __free(device_node) never holds ERR_PTR().
>
> v2:
> - Fix DEFINE_FREE(device_node, ...) directly.
>
> drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c | 54 ++++++++++----------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> index 441fd32dc91c..881ebb811eb3 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> @@ -222,52 +222,52 @@ static const struct drm_bridge_funcs
> imx8qxp_pxl2dpi_bridge_funcs = {
> imx8qxp_pxl2dpi_bridge_atomic_get_output_bus_fmts,
> };
>
> -static struct device_node *
> +static int
> imx8qxp_pxl2dpi_get_available_ep_from_port(struct imx8qxp_pxl2dpi *p2d,
> - u32 port_id)
> + u32 port_id,
> + struct device_node **ep)
> {
> - struct device_node *port, *ep;
> + struct device_node *port __free(device_node) =
Can you provide a minimal fix for stable tree by not using the cleanup
action? You can add the cleanup action with follow-up patch(es).
> + of_graph_get_port_by_id(p2d->dev->of_node, port_id);
> int ep_cnt;
>
> - port = of_graph_get_port_by_id(p2d->dev->of_node, port_id);
> + *ep = NULL;
> if (!port) {
> DRM_DEV_ERROR(p2d->dev, "failed to get port@%u\n", port_id);
> - return ERR_PTR(-ENODEV);
> + return -ENODEV;
> }
>
> ep_cnt = of_get_available_child_count(port);
> if (ep_cnt == 0) {
> DRM_DEV_ERROR(p2d->dev, "no available endpoints of port@%u\n",
> port_id);
> - ep = ERR_PTR(-ENODEV);
> - goto out;
> + return -ENODEV;
> } else if (ep_cnt > 1) {
> DRM_DEV_ERROR(p2d->dev,
> "invalid available endpoints of port@%u\n",
> port_id);
> - ep = ERR_PTR(-EINVAL);
> - goto out;
> + return -EINVAL;
> }
>
> - ep = of_get_next_available_child(port, NULL);
> - if (!ep) {
> + *ep = of_get_next_available_child(port, NULL);
> + if (!*ep) {
> DRM_DEV_ERROR(p2d->dev,
> "failed to get available endpoint of port@%u\n",
> port_id);
> - ep = ERR_PTR(-ENODEV);
> - goto out;
> + return -ENODEV;
> }
> -out:
> - of_node_put(port);
> - return ep;
> +
> + return 0;
> }
>
> static int imx8qxp_pxl2dpi_find_next_bridge(struct imx8qxp_pxl2dpi *p2d)
> {
> - struct device_node *ep __free(device_node) =
> - imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1);
> - if (IS_ERR(ep))
> - return PTR_ERR(ep);
> + struct device_node *ep __free(device_node) = NULL;
Why do you need to initialize ep to NULL?
> + int ret;
> +
> + ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 1, &ep);
> + if (ret)
> + return ret;
>
> struct device_node *remote __free(device_node) =
> of_graph_get_remote_port_parent(ep);
> if (!remote || !of_device_is_available(remote)) {
> @@ -287,26 +287,24 @@ static int imx8qxp_pxl2dpi_find_next_bridge(struct
> imx8qxp_pxl2dpi *p2d)
>
> static int imx8qxp_pxl2dpi_set_pixel_link_sel(struct imx8qxp_pxl2dpi *p2d)
> {
> - struct device_node *ep;
> + struct device_node *ep __free(device_node) = NULL;
Same here:
- Can you provide a minimal fix for stable tree?
- Why do you need to initialize ep to NULL?
> struct of_endpoint endpoint;
> int ret;
>
> - ep = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 0);
> - if (IS_ERR(ep))
> - return PTR_ERR(ep);
> + ret = imx8qxp_pxl2dpi_get_available_ep_from_port(p2d, 0, &ep);
> + if (ret)
> + return ret;
>
> ret = of_graph_parse_endpoint(ep, &endpoint);
> if (ret) {
> DRM_DEV_ERROR(p2d->dev,
> "failed to parse endpoint of port@0: %d\n", ret);
> - goto out;
> + return ret;
> }
>
> p2d->pl_sel = endpoint.id;
> -out:
> - of_node_put(ep);
>
> - return ret;
> + return 0;
> }
>
> static int imx8qxp_pxl2dpi_parse_dt_companion(struct imx8qxp_pxl2dpi *p2d)
> --
> 2.43.0
>
--
Regards,
Liu Ying
