Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] devm_reset_control_get_optional_exclusive in component bind causes -EBUSY on rebind - [Medium] drm/rockchip: vop2: AXI reset is ineffective if clocks are gated during bind - [Medium] drm/rockchip: asymmetrical teardown disables PM runtime before core clocks --
commit e900afe14d3a61065ef92e05c15c885d78bc0d20 Author: Cristian Ciocaltea <[email protected]> drm/rockchip: vop2: Reset AXI and DCLK to improve robustness Assert the AXI reset in the bind and CRTC disable paths, and the VP DCLK reset in the enable path. These resets are intended to leave the hardware in a clean state for the next use. > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index 14c073f61c646..afa4bc6cbcc4d 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -953,6 +974,8 @@ static int vop2_enable(struct vop2 *vop2) > > static void vop2_disable(struct vop2 *vop2) > { > + vop2_reset_assert_deassert(vop2, vop2->axi_rst); > + > rockchip_drm_dma_detach_device(vop2->drm, vop2->dev); > > pm_runtime_put_sync(vop2->dev); [Severity: Medium] This problem wasn't introduced by this patch, but does the hardware support having the PM runtime domain disabled before the core clocks? In vop2_disable(), pm_runtime_put_sync() drops the device's PM reference, which can power off the domain. However, the core clocks (pclk, aclk, hclk) are disabled later in vop2_core_clks_disable_unprepare(). Dropping the power domain while functional clocks are still running might violate hardware teardown sequencing and cause lockups. Since this patch modifies the disable sequence by adding the reset, could the asymmetry be corrected here as well? [ ... ] > @@ -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); [Severity: High] Will this leak the exclusive reset handle if the DRM master is unbound and later rebound? Since devm_reset_control_get_optional_exclusive() is tied to the component device (vop2->dev) rather than the aggregate DRM master device, the devres allocation won't be released when the component unbind callback executes because the component device itself isn't removed. Upon rebind, a second call to this function will find the reset line already exclusively acquired. This triggers a WARN_ON() in the reset core and fails the bind with -EBUSY, breaking the driver after an unbind cycle. [ ... ] > @@ -2955,6 +2986,13 @@ 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"); [Severity: High] Similar to the dclk_rst allocation in vop2_create_crtcs(), will allocating this device-managed resource here in the component bind path cause an -EBUSY failure on rebind? > + if (IS_ERR(vop2->axi_rst)) > + return dev_err_probe(drm->dev, PTR_ERR(vop2->axi_rst), > + "failed to get axi reset\n"); > + > + vop2_reset_assert_deassert(vop2, vop2->axi_rst); [Severity: Medium] Will this synchronous reset be effective if the AXI and AHB clocks are gated at this point? The reset pulse is sent immediately after acquiring the handle in vop2_bind(), but this happens before pm_runtime_enable() is called and before the core clocks are explicitly prepared and enabled. If the driver is loaded as a module or probed late, the kernel's unused clock framework may have already gated the clocks. Pulsing a synchronous reset while the IP block's clocks are gated usually acts as a silent no-op. When vop2_enable() later powers on the domain, the hardware might wake up with its previous dirty state, defeating the purpose of the reset. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
