Extract the GRF-level HPD interrupt mask/unmask register writes into
dedicated dw_hdmi_qp_rk35{88,76}_{mask,unmask}_hpd() helpers, and use
them from the .enable_hpd()/.disable_hpd() phy ops as well as the
hardirq, threaded re-arm and io_init() paths.
This is a preparatory refactor that separates the SoC-level HPD mask
from the phy op entry points, so a subsequent change can gate the
platform HPD IRQ line without affecting the debounce paths.
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 | 54 ++++++++++++++++++--------
1 file changed, 37 insertions(+), 17 deletions(-)
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free and resource leak due to devm_ usage in component bind
--
commit cea67879af1ca4ad52b13c14999be678a504386f
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
drm/rockchip: dw_hdmi_qp: Factor out HPD interrupt (un)mask helpers
Extract the GRF-level HPD interrupt mask/unmask register writes into
dedicated dw_hdmi_qp_rk35{88,76}_{mask,unmask}_hpd() helpers, and use
them from the .enable_hpd()/.disable_hpd() phy ops as well as the
hardirq, threaded re-arm and io_init() paths.
This is a preparatory refactor that separates the SoC-level HPD mask
from the phy op entry points, so a subsequent change can gate the
platform HPD IRQ line without affecting the debounce paths.
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 9e48ec06eb660..ecf12403101da 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
This is a pre-existing issue, but I noticed a potential problem in
dw_hdmi_qp_rockchip_bind():
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c:dw_hdmi_qp_rockchip_bind() {
...
hdmi = drmm_kzalloc(drm, sizeof(*hdmi), GFP_KERNEL);
...
ret = devm_clk_bulk_get_all_enabled(dev, &clks);
...
ret = devm_request_threaded_irq(dev, hdmi->hpd_irq,
cfg->ctrl_ops->hardirq_callback,
cfg->ctrl_ops->irq_callback,
IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
hdmi);
...
}
[Severity: High]
Can this component bind sequence result in a Use-After-Free?
The hdmi structure is allocated with drmm_kzalloc() on the master DRM
device, meaning it is freed when the master DRM device unbinds.
However, the IRQ is registered using devm_request_threaded_irq() on the
component platform device (dev).
Because devm_ resources are tied to the platform device's probe/remove
lifecycle rather than the component bind/unbind lifecycle, they are not
freed during component_unbind().
If the master device unbinds, the hdmi structure is freed, but the IRQ
remains active with a pointer to the freed memory.
If the IRQ fires after unbind, will it access the freed hdmi struct?
Additionally, are the devm_ allocations in the component bind leaked on
every component unbind and re-bind sequence?
Since devm_clk_bulk_get_all_enabled() is tied to the component device,
won't these clocks be leaked when the component unbinds and re-binds?
A similar issue appears in dw_hdmi_qp_bind():
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c:dw_hdmi_qp_bind() {
...
hdmi = devm_drm_bridge_alloc(dev, struct dw_hdmi_qp, bridge,
&dw_hdmi_qp_bridge_funcs);
...
}
[Severity: High]
Does this code leak the bridge structure every time the component is
re-bound?
Because devm_drm_bridge_alloc() is called on the component device, it
appears this memory will not be freed during component unbind.
@@ -277,9 +277,8 @@ dw_hdmi_qp_rk3588_read_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
return val ? connector_status_connected : connector_status_disconnected;
}
-static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
+static void dw_hdmi_qp_rk3588_unmask_hpd(struct rockchip_hdmi_qp *hdmi)
{
- struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
u32 val;
if (hdmi->port_id)
@@ -292,9 +291,8 @@ static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
}
-static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
+static void dw_hdmi_qp_rk3588_mask_hpd(struct rockchip_hdmi_qp *hdmi)
{
- struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
u32 val;
if (hdmi->port_id)
@@ -305,6 +303,20 @@ static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data
regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
}
+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);
+}
+
+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;
+
+ dw_hdmi_qp_rk3588_mask_hpd(hdmi);
+}
+
static const struct dw_hdmi_qp_phy_ops rk3588_hdmi_phy_ops = {
.init = dw_hdmi_qp_rk3588_phy_init,
.disable = dw_hdmi_qp_rk3588_phy_disable,
@@ -325,23 +337,31 @@ dw_hdmi_qp_rk3576_read_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
connector_status_connected : connector_status_disconnected;
}
+static void dw_hdmi_qp_rk3576_unmask_hpd(struct rockchip_hdmi_qp *hdmi)
+{
+ regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0,
+ FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_CLR, 1) |
+ FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 0));
+}
+
+static void dw_hdmi_qp_rk3576_mask_hpd(struct rockchip_hdmi_qp *hdmi)
+{
+ regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0,
+ FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 1));
+}
+
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;
- u32 val;
- val = (FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_CLR, 1) |
- FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 0));
-
- regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
+ dw_hdmi_qp_rk3576_unmask_hpd(hdmi);
}
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;
- regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0,
- FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 1));
+ dw_hdmi_qp_rk3576_mask_hpd(hdmi);
}
static const struct dw_hdmi_qp_phy_ops rk3576_hdmi_phy_ops = {
@@ -375,7 +395,7 @@ static irqreturn_t dw_hdmi_qp_rk3576_hardirq(int irq, void *dev_id)
regmap_read(hdmi->regmap, RK3576_IOC_HDMI_HPD_STATUS, &intr_stat);
if (intr_stat & RK3576_HDMI_OHPD_INT) {
- dw_hdmi_qp_rk3576_disable_hpd(NULL, hdmi);
+ dw_hdmi_qp_rk3576_mask_hpd(hdmi);
return IRQ_WAKE_THREAD;
}
@@ -389,7 +409,7 @@ static irqreturn_t dw_hdmi_qp_rk3576_irq(int irq, void *dev_id)
mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
- dw_hdmi_qp_rk3576_enable_hpd(NULL, hdmi);
+ dw_hdmi_qp_rk3576_unmask_hpd(hdmi);
return IRQ_HANDLED;
}
@@ -404,7 +424,7 @@ static irqreturn_t dw_hdmi_qp_rk3588_hardirq(int irq, void *dev_id)
intr_stat &= hdmi->port_id ? RK3588_HDMI1_OHPD_INT : RK3588_HDMI0_OHPD_INT;
if (intr_stat) {
- dw_hdmi_qp_rk3588_disable_hpd(NULL, hdmi);
+ dw_hdmi_qp_rk3588_mask_hpd(hdmi);
return IRQ_WAKE_THREAD;
}
@@ -418,7 +438,7 @@ static irqreturn_t dw_hdmi_qp_rk3588_irq(int irq, void *dev_id)
mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
- dw_hdmi_qp_rk3588_enable_hpd(NULL, hdmi);
+ dw_hdmi_qp_rk3588_unmask_hpd(hdmi);
return IRQ_HANDLED;
}
@@ -435,7 +455,7 @@ static void dw_hdmi_qp_rk3576_io_init(struct rockchip_hdmi_qp *hdmi)
regmap_write(hdmi->regmap, 0xa404, 0xffff0102);
- dw_hdmi_qp_rk3576_disable_hpd(NULL, hdmi);
+ dw_hdmi_qp_rk3576_mask_hpd(hdmi);
}
static void dw_hdmi_qp_rk3588_io_init(struct rockchip_hdmi_qp *hdmi)
@@ -460,7 +480,7 @@ static void dw_hdmi_qp_rk3588_io_init(struct rockchip_hdmi_qp *hdmi)
val = FIELD_PREP_WM16(RK3588_HDMI0_GRANT_SEL, 1);
regmap_write(hdmi->vo_regmap, RK3588_GRF_VO1_CON9, val);
- dw_hdmi_qp_rk3588_disable_hpd(NULL, hdmi);
+ dw_hdmi_qp_rk3588_mask_hpd(hdmi);
}
static void dw_hdmi_qp_rk3576_enc_init(struct rockchip_hdmi_qp *hdmi,