Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] The commit claims to inline `drm_simple_encoder_init` for the entire meson driver, but it only modifies `meson_encoder_hdmi.c`, leaving other encoders unconverted. Pre-existing issues: - [High] Missing `drm_bridge_remove()` on error paths in `meson_encoder_hdmi_probe()` leaves the bridge registered in the global `bridge_list`, leading to a dangling pointer and potential Use-After-Free. -- commit 640b38eb942af61527b421bb5d62cb275bca9574 Author: Liam Zuiderhoek <[email protected]> GPU: drm: meson: inline call to drm_simple_encoder_init() This commit inlines the function drm_simple_encoder_init for the meson driver. It replaces the simple encoder init call with drm_encoder_init and a custom cleanup struct. > This commit inlines the function drm_simple_encoder_init for the meson > driver. [Severity: Medium] Is the commit message description accurate for the scope of this patch? The commit states it inlines the function for the meson driver, but this patch only modifies the HDMI encoder (meson_encoder_hdmi.c). The CVBS (meson_encoder_cvbs_probe) and DSI (meson_encoder_dsi_probe) encoders in the same driver still use drm_simple_encoder_init(). Should this patch update those files as well, or should the commit message be clarified to specify it only targets the HDMI encoder? > diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c > b/drivers/gpu/drm/meson/meson_encoder_hdmi.c > index 55c0601df3c62..0dd3a76ad959e 100644 > --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c > +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c [ ... ] > @@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv) > meson_encoder_hdmi->priv = priv; > > /* Encoder */ > - ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder, > - DRM_MODE_ENCODER_TMDS); > + ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder, > + &drm_simple_encoder_funcs_cleanup, > + DRM_MODE_ENCODER_TMDS, NULL); > if (ret) { > dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n"); > goto err_put_node; [Severity: High] This is a pre-existing issue, but does this error path leave the bridge registered in the global bridge list, leading to a potential use-after-free? In meson_encoder_hdmi_probe(), drm_bridge_add() is called before the encoder initialization. If we fail here at drm_encoder_init() (or later during drm_bridge_connector_init()), the code jumps to err_put_node which does not call drm_bridge_remove(): err_put_node: of_node_put(remote); return ret; Similarly, if cec_notifier_conn_register() fails later in the probe, it returns directly without cleaning up the bridge. The cleanup function meson_encoder_hdmi_remove() will not catch this because priv->encoders[MESON_ENC_HDMI] is only assigned at the very end of a successful probe. If the still-registered bridge is later found via of_drm_find_and_get_bridge() and its callbacks are invoked, could it dereference the freed priv pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
