HDMI Specification Version 1.4b chapter 8.5 mentions:
An HDMI Sink shall not assert high voltage level on its Hot Plug
Detect pin when the E-EDID is not available for reading.
A Source may use a high voltage level Hot Plug Detect signal to
initiate the reading of E-EDID data.
An HDMI Sink shall indicate any change to the contents of the E-EDID
by driving a low voltage level pulse on the Hot Plug Detect pin. This
pulse shall be at least 100 msec.
Use a delayed work to debounce reacting on HPD events to improve
handling of a HPD low voltage level pulse when a sink changes the EDID.
The delayed work is only active between enable_hpd()/hpd_enable() and
disable_hpd()/hpd_disable() calls from core, i.e. enabled after
attach/bind/resume and disabled before detach/unbind/suspend.
The 1100 msec hotplug debounce timeout was arbitrarily picked to match
other drivers using same const, and testing using a Raspberry Pi Monitor
seem to use a 200-300 msec pulse when going from standby to power on
state.
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v5: Change to none-sync disable_delayed_work() in hpd disable ops,
Change to cancel_delayed_work_sync() in remove,
Add cancel_delayed_work_sync() to new suspend helper
v4: Disable/mask delayed_work until enable_hpd()/hpd_enable(),
Read connector status directly from HW regs in hpd_work
v3: New patch
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 60 +++++++++++++++++++++--
1 file changed, 56 insertions(+), 4 deletions(-)
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Asynchronous disable_delayed_work() in hpd_disable callbacks allows the hotplug work to run concurrently with DRM teardown, causing a Use-After-Free.
--
commit 3caa1bb48d6eede4b5fef2227a86e8e2d0b2a913
Author: Jonas Karlman <jonas@kwiboo.se>
drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event
This commit introduces a delayed work item to debounce HDMI hotplug events.
It enables the work during the enable_hpd phase and disables it asynchronously
during the disable_hpd phase, and finally cancels it synchronously when the
module is removed or suspended.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index a6f2702bca83c..f32eea9106b00 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
[ ... ]
> @@ -2517,6 +2520,20 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
> dw_hdmi_connector_status_update(connector, connector->status);
> }
>
> +static void dw_hdmi_connector_enable_hpd(struct drm_connector *connector)
> +{
> + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
> +
> + enable_delayed_work(&hdmi->hpd_work);
> +}
> +
> +static void dw_hdmi_connector_disable_hpd(struct drm_connector *connector)
> +{
> + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
> +
> + disable_delayed_work(&hdmi->hpd_work);
Could this cause a use-after-free during teardown?
Since disable_delayed_work() cancels the work asynchronously without waiting
for it to finish, if the work is currently executing on another cpu, it could
access the drm device after it has been freed during unbind.
> +}
[ ... ]
> @@ -2959,6 +2978,20 @@ static const struct drm_edid *dw_hdmi_bridge_edid_read(struct drm_bridge *bridge
> return dw_hdmi_edid_read(hdmi, connector);
> }
>
> +static void dw_hdmi_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct dw_hdmi *hdmi = bridge->driver_private;
> +
> + enable_delayed_work(&hdmi->hpd_work);
> +}
> +
> +static void dw_hdmi_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct dw_hdmi *hdmi = bridge->driver_private;
> +
> + disable_delayed_work(&hdmi->hpd_work);
Similarly, would this asynchronous cancel allow the hpd_work to run
concurrently with drm teardown?
Because the synchronous cancel_delayed_work_sync() in dw_hdmi_remove() is
executed only after the master unbind fully completes, the work could still
be executing on another cpu.
When the work accesses hdmi->bridge.dev:
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:dw_hdmi_hpd_work() {
...
drm_helper_hpd_irq_event(hdmi->bridge.dev);
...
}
Could it dereference an already freed drm device?
> +}
@@ -50,6 +50,8 @@
#define HDMI14_MAX_TMDSCLK 340000000
+#define HOTPLUG_DEBOUNCE_MS 1100
+
static const u16 csc_coeff_default[3][4] = {
{ 0x2000, 0x0000, 0x0000, 0x0000 },
{ 0x0000, 0x2000, 0x0000, 0x0000 },
@@ -185,6 +187,7 @@ struct dw_hdmi {
hdmi_codec_plugged_cb plugged_cb;
struct device *codec_dev;
enum drm_connector_status last_connector_result;
+ struct delayed_work hpd_work;
};
const struct dw_hdmi_plat_data *dw_hdmi_to_plat_data(struct dw_hdmi *hdmi)
@@ -2517,6 +2520,20 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
dw_hdmi_connector_status_update(connector, connector->status);
}
+static void dw_hdmi_connector_enable_hpd(struct drm_connector *connector)
+{
+ struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
+
+ enable_delayed_work(&hdmi->hpd_work);
+}
+
+static void dw_hdmi_connector_disable_hpd(struct drm_connector *connector)
+{
+ struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
+
+ disable_delayed_work(&hdmi->hpd_work);
+}
+
static void dw_hdmi_connector_destroy(struct drm_connector *connector)
{
struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
@@ -2538,6 +2555,8 @@ static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
.get_modes = dw_hdmi_connector_get_modes,
.atomic_check = dw_hdmi_connector_atomic_check,
+ .enable_hpd = dw_hdmi_connector_enable_hpd,
+ .disable_hpd = dw_hdmi_connector_disable_hpd,
};
static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
@@ -2959,6 +2978,20 @@ static const struct drm_edid *dw_hdmi_bridge_edid_read(struct drm_bridge *bridge
return dw_hdmi_edid_read(hdmi, connector);
}
+static void dw_hdmi_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+ struct dw_hdmi *hdmi = bridge->driver_private;
+
+ enable_delayed_work(&hdmi->hpd_work);
+}
+
+static void dw_hdmi_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+ struct dw_hdmi *hdmi = bridge->driver_private;
+
+ disable_delayed_work(&hdmi->hpd_work);
+}
+
static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -2972,6 +3005,8 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
.mode_valid = dw_hdmi_bridge_mode_valid,
.detect = dw_hdmi_bridge_detect,
.edid_read = dw_hdmi_bridge_edid_read,
+ .hpd_enable = dw_hdmi_bridge_hpd_enable,
+ .hpd_disable = dw_hdmi_bridge_hpd_disable,
};
/* -----------------------------------------------------------------------------
@@ -3092,10 +3127,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
status == connector_status_connected ?
"plugin" : "plugout");
- if (hdmi->bridge.dev) {
- drm_helper_hpd_irq_event(hdmi->bridge.dev);
- drm_bridge_hpd_notify(&hdmi->bridge, status);
- }
+ mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
+ msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
}
hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
@@ -3105,6 +3138,19 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static void dw_hdmi_hpd_work(struct work_struct *work)
+{
+ struct dw_hdmi *hdmi = container_of(work, struct dw_hdmi, hpd_work.work);
+ enum drm_connector_status status;
+
+ if (WARN_ON(!hdmi->bridge.dev))
+ return;
+
+ drm_helper_hpd_irq_event(hdmi->bridge.dev);
+ status = dw_hdmi_phy_read_hpd(hdmi, hdmi->phy.data);
+ drm_bridge_hpd_notify(&hdmi->bridge, status);
+}
+
static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {
{
.type = DW_HDMI_PHY_DWC_HDMI_TX_PHY,
@@ -3389,6 +3435,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
goto err_res;
}
+ INIT_DELAYED_WORK(&hdmi->hpd_work, dw_hdmi_hpd_work);
+ disable_delayed_work(&hdmi->hpd_work);
+
ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
dw_hdmi_irq, IRQF_SHARED,
dev_name(dev), hdmi);
@@ -3521,6 +3570,8 @@ EXPORT_SYMBOL_GPL(dw_hdmi_probe);
void dw_hdmi_remove(struct dw_hdmi *hdmi)
{
+ cancel_delayed_work_sync(&hdmi->hpd_work);
+
drm_bridge_remove(&hdmi->bridge);
if (hdmi->audio && !IS_ERR(hdmi->audio))
@@ -3570,6 +3621,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
void dw_hdmi_suspend(struct dw_hdmi *hdmi)
{
+ cancel_delayed_work_sync(&hdmi->hpd_work);
}
EXPORT_SYMBOL_GPL(dw_hdmi_suspend);