Re: [PATCH v9 23/61] drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unprotected lockless access to `hdmi->curr_conn` between atomic commit threads and the HPD notification thread, leading to a data race.
--

--- Patch [23]: [PATCH v9 23/61] drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper ---
commit f35dc78b72396dddf94643e1b74b8f00fbaf1275
Author: Cristian Ciocaltea <[email protected]>

drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper

This commit exports a helper function for platform drivers to report hot-plug
detection events. It targets the affected connector and ensures context
detection is invoked on reconnection events for SCDC state recovery.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index ee74266b7aa9c..be1bd765cb033 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c

[ ... ]

> @@ -785,6 +787,8 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	dw_hdmi_qp_mod(hdmi, op_mode, OPMODE_DVI, LINK_CONFIG0);
>  
>  	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
> +
> +	hdmi->curr_conn = connector;

[Severity: Medium]
Is a WRITE_ONCE() needed here? hdmi->curr_conn appears to be read locklessly
by the HPD threaded IRQ or work queue context. Without WRITE_ONCE(), could
the compiler reorder the store before hardware initialization is complete?

>  }
>  
>  static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
> @@ -793,6 +797,7 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
>  	struct dw_hdmi_qp *hdmi = bridge->driver_private;
>  	struct drm_connector *connector;
>  
> +	hdmi->curr_conn = NULL;

[Severity: Medium]
Does this assignment to hdmi->curr_conn also need a WRITE_ONCE() to ensure
safe concurrent access during concurrent hotplug events and modesets?

[ ... ]

> @@ -1409,6 +1414,37 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_qp_bind);
>  
> +/**
> + * dw_hdmi_qp_hpd_notify() - Notify a hot-plug detection event
> + * @hdmi: pointer to the DW HDMI QP controller
> + *
> + * Platform drivers should call this from their threaded IRQ handler or work
> + * function to notify the bridge of a connection status change.
> + * The bridge's .read_hpd() phy_ops callback is used to read the current
> + * connection status.
> + */
> +void dw_hdmi_qp_hpd_notify(struct dw_hdmi_qp *hdmi)
> +{
> +	struct drm_connector *conn = hdmi->curr_conn;

[Severity: Medium]
Since this is called from a threaded HPD interrupt handler or work function,
can this lockless read of hdmi->curr_conn race with the KMS modeset thread?

If the writes in the atomic commit paths use WRITE_ONCE(), does this need a
corresponding READ_ONCE() to prevent compiler optimizations like register
caching from causing stale reads?

> +	enum drm_connector_status status;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=23