Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Using devm_* APIs in component bind callbacks causes memory leaks, duplicate resource registrations on unbind/rebind, and fatal bind failures. The new patch exacerbates this by requesting exclusive reset controls. -- --- Patch [7]: [PATCH v3 07/14] drm/rockchip: vop2: Reset AXI and DCLK to improve robustness --- commit 9b374dfb00e64eeab45f2b86b059f7b28a55a177 Author: Cristian Ciocaltea <[email protected]> drm/rockchip: vop2: Reset AXI and DCLK to improve robustness This patch adds assertions for the AXI reset in the CRTC disable path and the VP DCLK reset in the enable path. These optional resets are intended to leave the hardware in a clean state and prevent random display output glitches on platforms like RK3588. > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -2596,6 +2621,12 @@ static int vop2_create_crtcs(struct vop2 *vop2) > return dev_err_probe(drm->dev, PTR_ERR(vp->dclk), > "failed to get %s\n", dclk_name); > > + vp->dclk_rst = > devm_reset_control_get_optional_exclusive(vop2->dev, > + > dclk_name); > + if (IS_ERR(vp->dclk_rst)) > + return dev_err_probe(drm->dev, PTR_ERR(vp->dclk_rst), > + "failed to get %s reset\n", > dclk_name); > + [Severity: High] This is a pre-existing issue, but will this exclusive reset control request fail with -EBUSY on component rebind? Because vop2_create_crtcs() is called from the component bind path in vop2_bind() in drivers/gpu/drm/rockchip/rockchip_drm_vop2.c, any resources allocated with devm_ functions tied to the platform device (vop2->dev) are not released when the component is unbound. When the component is rebound, the previous exclusive reset control is still held. It appears this might cause devm_reset_control_get_optional_exclusive() to deterministically fail with -EBUSY since the prior exclusive grab was never released. Should this use reset_control_get_optional_exclusive() instead and be manually released in the unbind path, or use a component-specific managed API? > np = of_graph_get_remote_node(dev->of_node, i, -1); > if (!np) { > drm_dbg(vop2->drm, "%s: No remote for vp%d\n", > __func__, i); [ ... ] > @@ -2955,6 +2986,11 @@ static int vop2_bind(struct device *dev, struct device > *master, void *data) > return dev_err_probe(drm->dev, PTR_ERR(vop2->pll_hdmiphy1), > "failed to get pll_hdmiphy1\n"); > > + vop2->axi_rst = devm_reset_control_get_optional_exclusive(vop2->dev, > "axi"); > + if (IS_ERR(vop2->axi_rst)) > + return dev_err_probe(drm->dev, PTR_ERR(vop2->axi_rst), > + "failed to get axi reset\n"); > + [Severity: High] This is also a pre-existing issue, but will requesting an exclusive reset control tied to the platform device here also cause a fatal -EBUSY error during a component rebind? Similar to the dclk_rst allocation, vop2->axi_rst is allocated in vop2_bind() with the devm_ API. When the DRM component master is unbound and then rebound, vop2_bind() executes again and attempts to grab the exclusive reset control. Since the previous devm_ allocation is tied to the persistent platform device and wasn't released during component unbind, will this fail immediately with -EBUSY? > vop2->irq = platform_get_irq(pdev, 0); > if (vop2->irq < 0) > return dev_err_probe(drm->dev, vop2->irq, "cannot find irq for > vop2\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
