Re: drm: of: Improve error handling in bridge/panel detection

[Paul Cercueil <paul@crapouillou.net>]

Hi Paul,

Le jeu., avril 7 2022 at 11:34:08 +0200, Paul Kocialkowski 
<paul.kocialkowski@bootlin.com> a écrit :
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or 
> unavailable.
> 
> Make the default return code of the function -ENODEV to handle this 
> and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device 
> is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
> 
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
> 
> Add comments around to clarify this behavior.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for 
> bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

This fixes the ingenic-drm driver, which was broken by the commit this 
patch addresses.

So:
Tested-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8716da6369a6..97ea9d2016ff 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct 
> device_node *node,
>  				struct drm_panel **panel,
>  				struct drm_bridge **bridge)
>  {
> +	if (!of_device_is_available(node))
> +		return -ENODEV;
> +
>  	if (panel) {
>  		*panel = of_drm_find_panel(node);
>  		if (!IS_ERR(*panel))
> @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct 
> device_node *np,
>  				struct drm_bridge **bridge)
>  {
>  	struct device_node *node;
> -	int ret;
> +	int ret = -ENODEV;
> 
>  	if (!panel && !bridge)
>  		return -EINVAL;
> @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct 
> device_node *np,
>  			ret = find_panel_or_bridge(node, panel, bridge);
>  			of_node_put(node);
> 
> -			if (!ret)
> -				return 0;
> +			/*
> +			 * If the graph/remote node is present we consider it
> +			 * to be the legitimate candidate here and return
> +			 * whatever code we got from find_panel_or_bridge.
> +			 */
> +			return ret;
>  		}
>  	}
> 
> @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct 
> device_node *np,
>  		ret = find_panel_or_bridge(node, panel, bridge);
>  		of_node_put(node);
> 
> -		/* Stop at the first found occurrence. */
> +		/*
> +		 * Note that an unrelated (available) child node will cause
> +		 * find_panel_or_bridge to return -EPROBE_DEFER because there
> +		 * is no way to distinguish the node from a legitimate
> +		 * panel/bridge that didn't register yet. Keep iterating nodes
> +		 * and only return on the first found occurrence.
> +		 */
>  		if (!ret)
>  			return 0;
>  	}
> 
> -	return -EPROBE_DEFER;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);