Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Unchecked return value of drm_encoder_init() leads to NULL pointer dereference on error or unbind paths. - [High] Incorrect usage of devres (devm_*) functions inside component bind callbacks causes resource leaks and duplicate registrations. -- commit 58124ed88e765fc2655a16706ba7e0962ef4b3df Author: Diogo Silva <[email protected]> drm/rockchip: Remove dependency on DRM simple helpers Simple KMS helper are deprecated since they only add an intermediate layer between drivers and the atomic modesetting. This patch removes the drm_simple_encoder_init() helper usage in the rockchip drivers by open coding it and using the encoder atomic helpers directly. > diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c > b/drivers/gpu/drm/rockchip/cdn-dp-core.c > index e159e1627f3c2..68aa66915e8ac 100644 > --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c > +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c [ ... ] > @@ -988,8 +991,8 @@ static int cdn_dp_bind(struct device *dev, struct device > *master, void *data) > dev->of_node); > DRM_DEBUG_KMS("possible_crtcs = 0x%x\n", encoder->possible_crtcs); > > - ret = drm_simple_encoder_init(drm_dev, encoder, > - DRM_MODE_ENCODER_TMDS); > + ret = drm_encoder_init(drm_dev, encoder, &cdn_dp_encoder_funcs, > + DRM_MODE_ENCODER_TMDS, NULL); [Severity: High] This is a pre-existing issue, but could using devm_drm_bridge_add() and other devres functions inside a component bind callback cause resources to permanently leak on unbind? The DRM component framework calls bind and unbind callbacks during the master DRM device lifecycle. However, the component platform devices themselves are not destroyed during unbind. Because devm allocations are tied to the platform device's lifetime, they are not freed when unbind runs. When cdn_dp_bind() is called again, new resources are allocated and registered, causing duplicate registrations (such as devm_extcon_register_notifier) and undefined behavior on re-bind. > if (ret) { > DRM_ERROR("failed to initialize encoder with drm\n"); > return ret; [ ... ] > diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c > b/drivers/gpu/drm/rockchip/rk3066_hdmi.c > index 9b8981fa19aee..f28d6a883e099 100644 > --- 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 isn't a bug introduced by this patch, but should the return value of drm_encoder_init() be checked here? If drm_encoder_init() fails, the encoder->funcs pointer remains NULL and initialization continues because the error is ignored. If a subsequent initialization step fails, or during a later component unbind in rk3066_hdmi_unbind(), the code unconditionally executes: hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder); Can this lead to a NULL pointer dereference? The patch replaced the function call without adding the necessary error check that was properly added to other drivers updated in this same patch. [Severity: High] This is also a pre-existing issue, but similar to the devres issues in cdn_dp_bind() and rockchip_rgb_init(), does using devm_kzalloc() and devm_request_threaded_irq() in rk3066_hdmi_bind() cause resources to permanently leak on unbind? Because devm allocations are tied to the platform device's lifetime, they are not freed when unbind runs, leaking resources and causing duplicate IRQ handler registrations on re-bind. > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &rk3066_hdmi_bridge_funcs; [ ... ] > diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c > b/drivers/gpu/drm/rockchip/rockchip_rgb.c > index add3123e5ce70..2ad24b914989f 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c > +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c [ ... ] > @@ -127,7 +130,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev, > encoder = &rgb->encoder.encoder; > encoder->possible_crtcs = drm_crtc_mask(crtc); > > - ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE); > + ret = drm_encoder_init(drm_dev, encoder, &rockchip_rgb_encoder_funcs, > + DRM_MODE_ENCODER_NONE, NULL); [Severity: High] This is a pre-existing issue, but does the devm allocation in rockchip_rgb_init() leak memory when the VOP unbinds and re-binds? rockchip_rgb_init() is called from vop_bind(), and devm_kzalloc() ties the memory to the VOP device's lifetime, which is not destroyed during an unbind. When bind is called again, new resources are allocated and registered, leaking the old ones. > if (ret < 0) { > DRM_DEV_ERROR(drm_dev->dev, > "failed to initialize encoder: %d\n", ret); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
