Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The return value of kmb_initialize_clocks() is ignored in kmb_hw_init(), leading to potential hardware hangs. - [High] Calling drm_atomic_helper_shutdown() after disabling clocks and PM runtime leads to hardware access on a powered-down device and a system hang. - [High] Dereferencing an ERR_PTR during error handling of DSI initialization causes a kernel panic. -- commit ce6024d25a2085ae8c8a68d80d92293c0cb8ad15 Author: Mukesh Ojha <[email protected]> drm: kmb: Use devm_of_reserved_mem_device_init() This commit updates the driver to use the devres-managed variant of of_reserved_mem_device_init(), simplifying the error and removal paths by removing manual cleanup calls. > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c > index 7c2eb1152fc2f..88fe377940f30 100644 > --- a/drivers/gpu/drm/kmb/kmb_drv.c > +++ b/drivers/gpu/drm/kmb/kmb_drv.c > @@ -140,13 +140,12 @@ static int kmb_hw_init(struct drm_device *drm, unsigned > long flags) > /* Allocate LCD interrupt resources */ > irq_lcd = platform_get_irq(pdev, 0); [Severity: High] This is a pre-existing issue, but I noticed that just before this hunk in kmb_hw_init(), the return value of kmb_initialize_clocks() is discarded: /* Enable display clocks */ kmb_initialize_clocks(kmb, &pdev->dev); /* Register irqs here - section 17.3 in databook If kmb_initialize_clocks() fails, the driver proceeds with probe and will eventually access unclocked MMIO registers during modeset. Can this lead to synchronous external aborts and system hangs? [ ... ] > @@ -465,8 +459,6 @@ static void kmb_remove(struct platform_device *pdev) > pm_runtime_put_sync(drm->dev); > pm_runtime_disable(drm->dev); > > - of_reserved_mem_device_release(drm->dev); > - > /* Release clks */ > kmb_display_clk_disable(kmb); > [Severity: High] This isn't a bug introduced by this patch, but does the shutdown sequence in kmb_remove() cause hardware access on a powered-down device? Right after this hunk, the function completes with: /* Unregister DSI host */ kmb_dsi_host_unregister(kmb->kmb_dsi); drm_atomic_helper_shutdown(drm); } Since pm_runtime_disable() and kmb_display_clk_disable() are called prior to drm_atomic_helper_shutdown(), atomic_disable callbacks will read and write hardware registers while clocks are disabled. Will this cause a bus fault or system hang during driver unbind? [Severity: High] This is also a pre-existing issue, but outside the diff in kmb_probe(), does the error handling for kmb_dsi_init() cause an invalid pointer dereference? If kmb_dsi_init() fails, it returns an ERR_PTR: kmb->kmb_dsi = kmb_dsi_init(dsi_pdev); if (IS_ERR(kmb->kmb_dsi)) { drm_err(&kmb->drm, "failed to initialize DSI\n"); ret = PTR_ERR(kmb->kmb_dsi); goto err_free1; } The error handling path unconditionally passes this ERR_PTR to kmb_dsi_host_unregister(), which passes it to kmb_dsi_clk_disable(). This dereferences the pointer when attempting to access kmb_dsi->clk_mipi: drivers/gpu/drm/kmb/kmb_dsi.c:kmb_dsi_clk_disable() { clk_disable_unprepare(kmb_dsi->clk_mipi); Will this result in a kernel panic due to dereferencing the ERR_PTR on memory allocation failure during initialization? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
