diff mbox series

[v11,39/74] drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops

Message ID 20260901-dw-hdmi-qp-scramb-v11-39-bc12954a0688@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea Sept. 1, 2026, 6:51 p.m. UTC 
The HPD interrupt must not be delivered before the DRM connector is fully
set up, and it should be quiesced while the display pipeline is suspended.
On top of that, the threaded handler re-arms HPD unconditionally after the
hotplug debounce, which must not override an explicit disable requested by
the DRM core.

Request the HPD IRQ in bind() with IRQF_NO_AUTOEN so it stays disabled
until the core arms it, and drive enable_irq()/disable_irq() from the
phy ops invoked by the bridge through its hpd_{enable,disable}()
callbacks.  This ensures the line is only armed after the connector is
ready.  disable_irq() additionally waits for the threaded handler to
finish, so its GRF re-arm can no longer race with and override a
core-requested disable.

Masking the line is not enough on its own, though: the threaded handler
merely queues hpd_work with a 150 ms debounce, so an event taken shortly
before the disable would still probe the connector and read the EDID
long after the core considers HPD quiesced.  Since disable_irq() has
drained the threaded handler, which is the only site arming the work, it
can no longer be requeued and dropping it is race-free.

Use the non-syncing cancel_delayed_work() for that: waiting on a running
callback would pull mode_config.mutex, taken by the work through
drm_connector_helper_hpd_irq_event(), under the hpd_state_mutex held by
drm_bridge_hpd_disable().  That inverts the order of the connector probe
path, which holds mode_config.mutex while reaching hpd_state_mutex via
drm_kms_helper_poll_enable().  A callback already in progress is drained
by the system-sleep and unbind paths instead.

Note that IRQF_NO_AUTOEN is incompatible with IRQF_SHARED; the latter is
dropped as this IRQ has no other users.

Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 33 +++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 6 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 1, 2026, 8:03 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] devm_request_threaded_irq() is improperly used in a component bind() callback, causing resource leaks on unbind() and -EBUSY failures on re-bind.
--

commit 227c25b0617d912242b79d253aafb0a1dcfaa430
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Subject: drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops

The commit changes the HPD IRQ to be requested with IRQF_NO_AUTOEN and manages
its state explicitly through the bridge HPD ops. It also removes the IRQF_SHARED
flag since there are no other users of this IRQ.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index ecf12403101da..1c547016ee009 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
> @@ -688,14 +709,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	if (plat_data.cec_irq < 0)
>  		return plat_data.cec_irq;
>  
> -	irq = platform_get_irq_byname(pdev, "hpd");
> -	if (irq < 0)
> -		return irq;
> +	hdmi->hpd_irq = platform_get_irq_byname(pdev, "hpd");
> +	if (hdmi->hpd_irq < 0)
> +		return hdmi->hpd_irq;
>  
> -	ret = devm_request_threaded_irq(dev, irq,
> +	ret = devm_request_threaded_irq(dev, hdmi->hpd_irq,
>  					cfg->ctrl_ops->hardirq_callback,
>  					cfg->ctrl_ops->irq_callback,
> -					IRQF_SHARED, "dw-hdmi-qp-hpd",
> +					IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
>  					hdmi);
>  	if (ret)
>  		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but dropping the IRQF_SHARED flag
here exposes a pre-existing resource leak as a hard failure during re-bind.

Because devm_request_threaded_irq() is used in the component bind() callback
(dw_hdmi_qp_rockchip_bind()), the IRQ allocation is tied to the platform
device, not the component lifecycle. When the component is unbound (e.g., via
sysfs or DRM master unbind), the IRQ is not freed.

Previously, with IRQF_SHARED, a subsequent re-bind would succeed despite
leaking another registration. Now that IRQF_SHARED is removed, if a user
unbinds and then re-binds the component, won't devm_request_threaded_irq()
fail with -EBUSY, breaking the display driver until a full module reload?

Could this be fixed by using request_threaded_irq() and explicitly freeing it
in the component unbind() callback?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index ecf12403101d..1c547016ee00 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -99,6 +99,7 @@  struct rockchip_hdmi_qp {
 	struct phy *phy;
 	struct gpio_desc *frl_enable_gpio;
 	struct delayed_work hpd_work;
+	int hpd_irq;
 	int port_id;
 	const struct rockchip_hdmi_qp_ctrl_ops *ctrl_ops;
 	unsigned long long tmds_char_rate;
@@ -308,13 +309,23 @@  static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
 	dw_hdmi_qp_rk3588_unmask_hpd(hdmi);
+	enable_irq(hdmi->hpd_irq);
 }
 
 static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
+	disable_irq(hdmi->hpd_irq);
 	dw_hdmi_qp_rk3588_mask_hpd(hdmi);
+
+	/*
+	 * Cannot use the _sync() variant: the work takes mode_config.mutex,
+	 * which the connector probe path already holds when acquiring the
+	 * hpd_state_mutex this caller runs under. Requeuing is already ruled
+	 * out by disable_irq().
+	 */
+	cancel_delayed_work(&hdmi->hpd_work);
 }
 
 static const struct dw_hdmi_qp_phy_ops rk3588_hdmi_phy_ops = {
@@ -355,13 +366,23 @@  static void dw_hdmi_qp_rk3576_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
 	dw_hdmi_qp_rk3576_unmask_hpd(hdmi);
+	enable_irq(hdmi->hpd_irq);
 }
 
 static void dw_hdmi_qp_rk3576_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
+	disable_irq(hdmi->hpd_irq);
 	dw_hdmi_qp_rk3576_mask_hpd(hdmi);
+
+	/*
+	 * Cannot use the _sync() variant: the work takes mode_config.mutex,
+	 * which the connector probe path already holds when acquiring the
+	 * hpd_state_mutex this caller runs under. Requeuing is already ruled
+	 * out by disable_irq().
+	 */
+	cancel_delayed_work(&hdmi->hpd_work);
 }
 
 static const struct dw_hdmi_qp_phy_ops rk3576_hdmi_phy_ops = {
@@ -584,7 +605,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	struct resource *res;
 	struct clk_bulk_data *clks;
 	struct clk *ref_clk;
-	int ret, irq, i;
+	int ret, i;
 
 	if (!dev->of_node)
 		return -ENODEV;
@@ -688,14 +709,14 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (plat_data.cec_irq < 0)
 		return plat_data.cec_irq;
 
-	irq = platform_get_irq_byname(pdev, "hpd");
-	if (irq < 0)
-		return irq;
+	hdmi->hpd_irq = platform_get_irq_byname(pdev, "hpd");
+	if (hdmi->hpd_irq < 0)
+		return hdmi->hpd_irq;
 
-	ret = devm_request_threaded_irq(dev, irq,
+	ret = devm_request_threaded_irq(dev, hdmi->hpd_irq,
 					cfg->ctrl_ops->hardirq_callback,
 					cfg->ctrl_ops->irq_callback,
-					IRQF_SHARED, "dw-hdmi-qp-hpd",
+					IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
 					hdmi);
 	if (ret)
 		return ret;