On Tue, Jan 07, 2025 at 03:36:20PM +0100, Maxime Ripard wrote: > On Wed, Dec 25, 2024 at 01:10:13AM +0200, Dmitry Baryshkov wrote: > > CEC adapter functions are protected by drm_dev_enter() / _exit(), so it > > is safe to keep CEC registered until DRM device is being torn down. Drop > > custom CEC unsregistration code and rely on the DRM core to unregister > > CEC adapter. > > > > Signed-off-by: Dmitry Baryshkov <[email protected]> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 53 > > ++++++------------------------------------ > > 1 file changed, 7 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index > > 3086c2ad3bb2e8fafdc1f37ba985aa5785d49f9a..7b39000369e50f5368384575aa81919df5a7133c > > 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -2752,13 +2752,6 @@ static const struct cec_adap_ops > > vc4_hdmi_cec_adap_ops = { > > .adap_transmit = vc4_hdmi_cec_adap_transmit, > > }; > > > > -static void vc4_hdmi_cec_release(void *ptr) > > -{ > > - struct drm_connector *connector = ptr; > > - > > - drm_connector_cec_unregister(connector); > > -} > > - > > static int vc4_hdmi_cec_init(struct drm_connector *connector) > > { > > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > @@ -2796,51 +2789,19 @@ static int vc4_hdmi_cec_register(struct vc4_hdmi > > *vc4_hdmi) > > { > > struct platform_device *pdev = vc4_hdmi->pdev; > > struct device *dev = &pdev->dev; > > - int ret; > > > > if (!of_property_present(dev->of_node, "interrupts")) { > > dev_warn(dev, "'interrupts' DT property is missing, no CEC\n"); > > return 0; > > } > > > > - ret = drm_connector_hdmi_cec_adapter_register(&vc4_hdmi->connector, > > - &vc4_hdmi_cec_adap_ops, > > - > > vc4_hdmi->variant->card_name, > > - 1, > > - vc4_hdmi_cec_init, > > - NULL, > > - &pdev->dev); > > - if (ret < 0) > > - return ret; > > - > > - /* > > - * NOTE: Strictly speaking, we should probably use a DRM-managed > > - * registration there to avoid removing the CEC adapter by the > > - * time the DRM driver doesn't have any user anymore. > > - * > > - * However, the CEC framework already cleans up the CEC adapter > > - * only when the last user has closed its file descriptor, so we > > - * don't need to handle it in DRM. > > - * > > - * By the time the device-managed hook is executed, we will give > > - * up our reference to the CEC adapter and therefore don't > > - * really care when it's actually freed. > > - * > > - * There's still a problematic sequence: if we unregister our > > - * CEC adapter, but the userspace keeps a handle on the CEC > > - * adapter but not the DRM device for some reason. In such a > > - * case, our vc4_hdmi structure will be freed, but the > > - * cec_adapter structure will have a dangling pointer to what > > - * used to be our HDMI controller. If we get a CEC call at that > > - * moment, we could end up with a use-after-free. Fortunately, > > - * the CEC framework already handles this too, by calling > > - * cec_is_registered() in cec_ioctl() and cec_poll(). > > - */ > > I'd really like to keep that comment, somewhere.
Ack, I will try to rephrase it so that it still fits the changed source. -- With best wishes Dmitry
