[v5,14/21] drm: bridge: dw_hdmi: Use generic CEC notifier helpers
Commit Message
The commit 8b1a8f8b2002 ("drm/display: add CEC helpers code") added
generic CEC helpers to be used by HDMI drivers.
Replace the open-coded CEC notifier handling with use of the generic CEC
notifier helpers. Ensure DRM_DISPLAY_HDMI_CEC_NOTIFIER_HELPER is also
selected when DRM_DW_HDMI_CEC is enabled so that the CEC helpers is
available.
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v5: Collect r-b tag
v4: New patch
---
drivers/gpu/drm/bridge/synopsys/Kconfig | 1 +
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 26 +++++------------------
2 files changed, 6 insertions(+), 21 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use-After-Free and Memory Leak due to lifecycle mismatch with drmm-managed CEC notifier registration
--
commit f25787cb94e7936f6b23599ef90d85d636679290
Author: Jonas Karlman <jonas@kwiboo.se>
drm: bridge: dw_hdmi: Use generic CEC notifier helpers
This commit replaces the open-coded CEC notifier handling with generic CEC
notifier helpers, specifically utilizing drmm_connector_hdmi_cec_notifier_register.
It also selects DRM_DISPLAY_HDMI_CEC_NOTIFIER_HELPER when DRM_DW_HDMI_CEC
is enabled.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 6fb21399f1370..bf893c754539b 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
[ ... ]
> @@ -2525,9 +2521,6 @@ static void dw_hdmi_connector_destroy(struct drm_connector *connector)
> {
> struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
>
> - cec_notifier_conn_unregister(hdmi->cec_notifier);
> - hdmi->cec_notifier = NULL;
> -
> drm_connector_cleanup(connector);
> drm_bridge_put(&hdmi->bridge);
> }
Since drm_connector_cleanup() clears the connector memory with a memset, could
this cause a memory leak of the cec_notifier?
Because drmm_connector_hdmi_cec_notifier_register() ties the notifier cleanup
to the drm_device release, the delayed drmm action will eventually run. But by
that time, connector->cec.data will be NULL due to the memset in
drm_connector_cleanup(). Will cec_notifier_conn_unregister(NULL) then
silently return and leave the notifier allocated?
> @@ -2587,15 +2578,8 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
>
> drm_connector_attach_encoder(connector, hdmi->bridge.encoder);
>
> - cec_fill_conn_info_from_drm(&conn_info, connector);
> -
> - notifier = cec_notifier_conn_register(hdmi->dev, NULL, &conn_info);
> - if (!notifier)
> - return -ENOMEM;
> -
> - hdmi->cec_notifier = notifier;
> -
> - return 0;
> + return drmm_connector_hdmi_cec_notifier_register(connector, NULL,
> + hdmi->dev);
> }
Could tying the CEC registration to the drmm lifecycle lead to a use-after-free
if the bridge unbinds while the DRM device is kept open by userspace?
The dw_hdmi structure (which embeds the connector) is typically allocated via
devm and tied to the bridge platform device. If the bridge unbinds, the
connector memory is freed. However, the drmm cleanup action remains queued on
the DRM device. When the DRM device is finally released, will the drmm
cleanup action dereference the already freed connector pointer?
@@ -49,6 +49,7 @@ config DRM_DW_HDMI_CEC
depends on DRM_DW_HDMI
select CEC_CORE
select CEC_NOTIFIER
+ select DRM_DISPLAY_HDMI_CEC_NOTIFIER_HELPER
help
Support the CE interface which is part of the Synopsys
Designware HDMI block.
@@ -23,12 +23,11 @@
#include <linux/dma-mapping.h>
#include <linux/spinlock.h>
-#include <media/cec-notifier.h>
-
#include <linux/media-bus-format.h>
#include <linux/videodev2.h>
#include <drm/bridge/dw_hdmi.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
#include <drm/display/drm_hdmi_helper.h>
#include <drm/display/drm_scdc_helper.h>
#include <drm/drm_atomic.h>
@@ -183,8 +182,6 @@ struct dw_hdmi {
void (*enable_audio)(struct dw_hdmi *hdmi);
void (*disable_audio)(struct dw_hdmi *hdmi);
- struct cec_notifier *cec_notifier;
-
hdmi_codec_plugged_cb plugged_cb;
struct device *codec_dev;
enum drm_connector_status last_connector_result;
@@ -2453,7 +2450,7 @@ dw_hdmi_connector_status_update(struct drm_connector *connector,
if (status == connector_status_disconnected) {
drm_edid_connector_update(connector, NULL);
- cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
+ drm_connector_cec_phys_addr_invalidate(connector);
return;
}
@@ -2462,8 +2459,7 @@ dw_hdmi_connector_status_update(struct drm_connector *connector,
drm_edid_free(drm_edid);
if (status == connector_status_connected)
- cec_notifier_set_phys_addr(hdmi->cec_notifier,
- connector->display_info.source_physical_address);
+ drm_connector_cec_phys_addr_set(connector);
}
static enum drm_connector_status
@@ -2525,9 +2521,6 @@ static void dw_hdmi_connector_destroy(struct drm_connector *connector)
{
struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
- cec_notifier_conn_unregister(hdmi->cec_notifier);
- hdmi->cec_notifier = NULL;
-
drm_connector_cleanup(connector);
drm_bridge_put(&hdmi->bridge);
}
@@ -2550,8 +2543,6 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs =
static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
{
struct drm_connector *connector = &hdmi->connector;
- struct cec_connector_info conn_info;
- struct cec_notifier *notifier;
int ret;
if (hdmi->version >= 0x200a)
@@ -2587,15 +2578,8 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
drm_connector_attach_encoder(connector, hdmi->bridge.encoder);
- cec_fill_conn_info_from_drm(&conn_info, connector);
-
- notifier = cec_notifier_conn_register(hdmi->dev, NULL, &conn_info);
- if (!notifier)
- return -ENOMEM;
-
- hdmi->cec_notifier = notifier;
-
- return 0;
+ return drmm_connector_hdmi_cec_notifier_register(connector, NULL,
+ hdmi->dev);
}
/* -----------------------------------------------------------------------------