[v6,08/22] drm: bridge: dw_hdmi: Unregister CEC notifier during connector cleanup
Commit Message
The CEC notifier is being unregistered when the bridge detach,
something that happens earlier than normal connector cleanup.
Change to unregister the CEC notifier at connector cleanup, in the
connector .destroy() func, to align the lifetime of the connector and
the CEC notifier and closer match a drmres handled generic CEC notifier.
Tested-by: Diederik de Haas <diederik@cknow-tech.com> # Rock64, RockPro64, Quartz64-B
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v6: Collect t-b tag
v5: New patch
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving `cec_notifier_conn_unregister` to the connector's `.destroy` callback introduces an asynchronous state corruption vulnerability, breaking CEC functionality on driver rebind.
--
commit 8c5f1d529adb3a91696d1e624a0b3dc31d5287ba
Author: Jonas Karlman <jonas@kwiboo.se>
drm: bridge: dw_hdmi: Unregister CEC notifier during connector cleanup
This commit moves the cec_notifier_conn_unregister call from the bridge
detach function to the connector destroy function. This is intended to align
the lifetime of the connector and the CEC notifier.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index cbbd155780425..5fd26ff8f55b4 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2532,6 +2532,11 @@ static void dw_hdmi_connector_destroy(struct drm_connector *connector)
> {
> struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
>
> + mutex_lock(&hdmi->cec_notifier_mutex);
> + cec_notifier_conn_unregister(hdmi->cec_notifier);
Could this cause state corruption if a user holds the DRM file descriptor
open across a driver unbind/bind cycle?
Because the connector destroy callback runs asynchronously when the last DRM
file descriptor is closed, its execution can be delayed indefinitely.
If the driver is unbound and then rebound while the old file descriptor is
still open, a new dw_hdmi is allocated and a new connector is created. When
cec_notifier_conn_register is called on rebind, it looks up the notifier by
the physical device (hdmi->dev) and shares the exact same cec_notifier with
the new active pipeline.
When the user eventually closes the old file descriptor, the old connector's
destroy callback will fire and call cec_notifier_conn_unregister. This will
clear the connection info and invalidate the physical address, which would
destroy the active CEC state of the newly bound display pipeline.
Should hardware-coupled state like the CEC notifier be invalidated at
hardware boundaries like detach, rather than software refcount boundaries
like destroy?
> + hdmi->cec_notifier = NULL;
> + mutex_unlock(&hdmi->cec_notifier_mutex);
> +
> drm_connector_cleanup(connector);
> drm_bridge_put(&hdmi->bridge);
> }
[ ... ]
@@ -2532,6 +2532,11 @@ static void dw_hdmi_connector_destroy(struct drm_connector *connector)
{
struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
+ mutex_lock(&hdmi->cec_notifier_mutex);
+ cec_notifier_conn_unregister(hdmi->cec_notifier);
+ hdmi->cec_notifier = NULL;
+ mutex_unlock(&hdmi->cec_notifier_mutex);
+
drm_connector_cleanup(connector);
drm_bridge_put(&hdmi->bridge);
}
@@ -2909,16 +2914,6 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
return dw_hdmi_connector_create(hdmi);
}
-static void dw_hdmi_bridge_detach(struct drm_bridge *bridge)
-{
- struct dw_hdmi *hdmi = bridge->driver_private;
-
- mutex_lock(&hdmi->cec_notifier_mutex);
- cec_notifier_conn_unregister(hdmi->cec_notifier);
- hdmi->cec_notifier = NULL;
- mutex_unlock(&hdmi->cec_notifier_mutex);
-}
-
static enum drm_mode_status
dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
const struct drm_display_info *info,
@@ -2996,7 +2991,6 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_reset = drm_atomic_helper_bridge_reset,
.attach = dw_hdmi_bridge_attach,
- .detach = dw_hdmi_bridge_detach,
.atomic_check = dw_hdmi_bridge_atomic_check,
.atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts,
.atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts,