Message-ID: <20250718164156.194702d9@booty>
Date: Fri, 18 Jul 2025 16:41:56 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Dave Stevenson
 <dave.stevenson@...pberrypi.com>, Maíra Canal
 <mcanal@...lia.com>, Raspberry Pi Kernel Maintenance
 <kernel-list@...pberrypi.com>, Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman
 <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, Dmitry
 Baryshkov <lumag@...nel.org>
Subject: Re: [PATCH v6 08/10] drm/display: bridge-connector: hook in CEC
 notifier support

Hi Dmitry,

On Sat, 17 May 2025 04:59:44 +0300
Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> wrote:

> Allow HDMI DRM bridges to create CEC notifier. Physical address is
> handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
> being called from .detect() path.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> Reviewed-by: Maxime Ripard <mripard@...nel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>

While working on drm_bridge_connector_init() for unrelated changes I
stumbled upon something in this patch (now committed) which at a
cursory look appears wrong to me.  Even though I still haven't analyzed
in depth I'm reporting it ASAP so you are aware and can either correct
me or confirm there is a bug.

> @@ -662,6 +670,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  			bridge_connector->bridge_dp_audio = bridge;
>  		}
>  
> +		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> +			if (bridge_connector->bridge_hdmi_cec)
> +				return ERR_PTR(-EBUSY);
> +
> +			bridge_connector->bridge_hdmi_cec = bridge;
> +		}
> +
>  		if (!drm_bridge_get_next_bridge(bridge))
>  			connector_type = bridge->type;
>  
> @@ -724,6 +739,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  			return ERR_PTR(ret);
>  	}
>  
> +	if (bridge_connector->bridge_hdmi_cec &&
> +	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> +		ret = drmm_connector_hdmi_cec_notifier_register(connector,
> +								NULL,
> +								bridge->hdmi_cec_dev);

Here you are using the 'bridge' pointer, which is the variable used by
the long drm_for_each_bridge_in_chain() loop at the function top. The
same happens in the following patch. I am not sure this is what was
intended, but I don't understand all the details of your series.

In an older patch [0] you had added a similar change, dereferencing the
same 'bridge' variable after the drm_for_each_bridge_in_chain() loop.
That was a bug fixed by a later patch [1].

Superficially this change (as well as patch 9) appears equally wrong.

Basically the value of 'bridge' here could be NULL or
bridge_connector->bridge_hdmi, depending on the
bridge_connector->bridge_hdmi value.

Is this the what you'd expect?

And if it is, what is the correct fix? Maybe:

	ret = drmm_connector_hdmi_cec_notifier_register(connector,
						NULL,
-						bridge->hdmi_cec_dev);
+						bridge_connector->bridge_hdmi_cec->hdmi_cec_dev);

?

Removing unrelated lines, and adding a few comments, the code flow of
the function is:

struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
						struct drm_encoder *encoder)
{
	struct drm_bridge *bridge, *panel_bridge = NULL;

	drm_for_each_bridge_in_chain(encoder, bridge) {
		/* ...lots of stuff... */

		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
			bridge_connector->bridge_hdmi_cec = bridge;
		}
	}

/* now bridge == NULL */

	if (bridge_connector->bridge_hdmi) {
		bridge = bridge_connector->bridge_hdmi;
	} else {
	}

/* now bridge can be NULL or bridge_connector->bridge_hdmi */

	if (bridge_connector->bridge_hdmi_audio ||
	    bridge_connector->bridge_dp_audio) {
		/* this is the code that got changed by [0] ad fixed by [1] */
		if (bridge_connector->bridge_hdmi_audio)
			bridge = bridge_connector->bridge_hdmi_audio;
		else
			bridge = bridge_connector->bridge_dp_audio;

		dev = bridge->hdmi_audio_dev;

		ret = drm_connector_hdmi_audio_init(connector, dev,
						    &drm_bridge_connector_hdmi_audio_funcs,
						    bridge->hdmi_audio_max_i2s_playback_channels,
						    bridge->hdmi_audio_i2s_formats,
						    bridge->hdmi_audio_spdif_playback,
						    bridge->hdmi_audio_dai_port);
	}

/* This is the code added by this patch */
	if (bridge_connector->bridge_hdmi_cec &&
	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
		ret = drmm_connector_hdmi_cec_notifier_register(connector,
								NULL,
								bridge->hdmi_cec_dev);
		if (ret)
			return ERR_PTR(ret);
	}

/* This is the code added by patch 09/10 */
	if (bridge_connector->bridge_hdmi_cec &&
	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
		ret = drmm_connector_hdmi_cec_register(connector,
						       &drm_bridge_connector_hdmi_cec_funcs,
						       bridge->hdmi_cec_adapter_name,
						       bridge->hdmi_cec_available_las,
						       bridge->hdmi_cec_dev);
		if (ret)
			return ERR_PTR(ret);
	}
}

[0] https://cgit.freedesktop.org/drm-misc/commit/?id=231adeda9f67
    -> hunk @@ -641,11 +705,16 @@
[1] https://cgit.freedesktop.org/drm-misc/commit/?id=10357824151262636fda879845f8b64553541106

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists