Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The return value of `drm_encoder_init()` is ignored, which can lead to a NULL pointer dereference on subsequent error cleanup paths. -- commit 386dea7155c8b56dde923c52be1155ed455ebd09 Author: Diogo Silva <[email protected]> drm/rockchip: dsi: Open-code drm_simple_encoder_init() This patch removes the dependency on the deprecated simple KMS helper from rockchip DRM drivers by inlining drm_simple_encoder_init into the drivers. [ ... ] > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c [ ... ] > @@ -604,7 +607,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, > struct device *master, > } > > drm_encoder_helper_add(encoder, &dw_hdmi_rockchip_encoder_helper_funcs); > - drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); > + drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs, > + DRM_MODE_ENCODER_TMDS, NULL); [Severity: High] This isn't a bug introduced by this patch, but does the return value of drm_encoder_init() need to be checked here? If drm_encoder_init() fails, encoder->dev remains NULL. If the subsequent dw_hdmi_bind() call fails, the error path jumps to err_bind and calls drm_encoder_cleanup(encoder). This explicitly dereferences encoder->dev inside drm_mode_object_unregister(), which would cause a NULL pointer dereference. [ ... ] > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c [ ... ] > @@ -586,7 +589,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, > struct device *master, > return ret; > > drm_encoder_helper_add(encoder, > &dw_hdmi_qp_rockchip_encoder_helper_funcs); > - drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); > + drm_encoder_init(drm, encoder, &dw_hdmi_qp_rockchip_encoder_funcs, > + DRM_MODE_ENCODER_TMDS, NULL); [Severity: High] This is a pre-existing issue, but should this code check the return value of drm_encoder_init() for errors? Similar to the previous file, if this initialization fails and leaves encoder->dev as NULL, a subsequent failure in dw_hdmi_qp_bind() triggers an error path that explicitly calls drm_encoder_cleanup(encoder), leading to a NULL pointer dereference. [ ... ] > diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c > b/drivers/gpu/drm/rockchip/rk3066_hdmi.c > --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c > +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c [ ... ] > @@ -696,7 +699,8 @@ rk3066_hdmi_register(struct drm_device *drm, struct > rk3066_hdmi *hdmi) > return -EPROBE_DEFER; > > drm_encoder_helper_add(encoder, &rk3066_hdmi_encoder_helper_funcs); > - drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); > + drm_encoder_init(drm, encoder, &rk3066_hdmi_encoder_funcs, > + DRM_MODE_ENCODER_TMDS, NULL); [Severity: High] This is a pre-existing issue, but what happens if drm_encoder_init() fails here? If it fails and a subsequent initialization step fails, the error cleanup will call the encoder's destroy function, causing a NULL pointer dereference since encoder->dev would be NULL. [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
