Re: [PATCH 18/24] drm/msm/hdmi: cache is_hdmi instead of storing the connector

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing State Synchronization when connector state is forced via sysfs.
- [Medium] Data Race on `hdmi->is_hdmi` boolean.

Pre-existing issues:
- [High] NULL Pointer Dereference on drmm-managed connector destroy hook.
--

--- Patch [18]: [PATCH 18/24] drm/msm/hdmi: cache is_hdmi instead of storing the connector ---
commit c8e64d4ebbe7d3c6a0a2cb94dd5d7a4af528ed77
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/hdmi: cache is_hdmi instead of storing the connector

This commit updates the HDMI driver to cache the is_hdmi boolean from the
EDID rather than retaining a driver-side pointer to the drm_connector. The
cached value is refreshed via a new hpd_notify bridge callback.

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 1d5a4b63f3c8f..62961dddd8f5e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c

[ ... ]

> @@ -204,10 +205,8 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
>  	return 0;
>  
>  fail:
> -	if (hdmi->connector) {
> -		hdmi->connector->funcs->destroy(hdmi->connector);
> -		hdmi->connector = NULL;
> -	}
> +	if (connector)
> +		connector->funcs->destroy(connector);

[Severity: High]
This is a pre-existing issue, but does calling the destroy hook here cause a
NULL pointer dereference? 
    
The connector is created earlier in msm_hdmi_modeset_init() via
drm_bridge_connector_init(), which uses the drmm-managed API
drmm_connector_init(). The DRM-managed API explicitly requires funcs->destroy
to be NULL, otherwise it returns -EINVAL. 
    
This means connector->funcs->destroy is guaranteed to be NULL when this error
path is taken, which would cause a panic when dereferenced.

>  
>  	return ret;
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 7abb9243dba57..84a423af73e93 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c

[ ... ]

> @@ -452,6 +452,17 @@ static enum drm_mode_status msm_hdmi_bridge_tmds_char_rate_valid(const struct dr
>  	return 0;
>  }
>  
> +static void msm_hdmi_bridge_hpd_notify(struct drm_bridge *bridge,
> +				       struct drm_connector *connector,
> +				       enum drm_connector_status status)
> +{
> +	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> +	struct hdmi *hdmi = hdmi_bridge->hdmi;
> +
> +	/* called after the EDID update, so display_info is up to date */
> +	hdmi->is_hdmi = connector->display_info.is_hdmi;

[Severity: High]
Can the cached hdmi->is_hdmi state become stale if the connector is forced
via sysfs? 

When userspace forces the connector state (for example, by echoing on to the
status file), the DRM core sets the status and invokes 
connector->funcs->force(), which maps to drm_bridge_connector_force(). 

This force operation successfully updates the EDID and display_info.is_hdmi,
but it bypasses the standard detect hook and does not invoke the hpd_notify
callback. Will this missing state synchronization cause HDMI-specific hardware
programming to incorrectly fall back to DVI mode and drop audio?

[Severity: Medium]
Could there be a data race when accessing hdmi->is_hdmi? 

This variable is updated here in msm_hdmi_bridge_hpd_notify() from the
asynchronous hotplug workqueue thread. Concurrently, it is read in
msm_hdmi_set_mode() under the reg_lock spinlock, and in msm_hdmi_audio_update()
from audio subsystem paths. 

Since there is no common lock protecting this cross-thread state access, and
neither READ_ONCE() nor WRITE_ONCE() is used, will this trigger KCSAN
warnings?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=18