Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] vc4_v3d_unbind() accesses hardware without resuming from runtime suspend, leading to system hangs. - [High] Component bind callbacks improperly use devm_ APIs on the component device, causing memory leaks and dangling IRQ handlers. -- --- Patch [1]: [PATCH] drm/vc4: hvs/v3d: Fix null dereference in unbind --- commit 70be44b543f0237e5db3dfd6dff591c323b131ef Author: Gregor Herburger <[email protected]> drm/vc4: hvs/v3d: Fix null dereference in unbind The hvs and v3d drivers use dev_get_drvdata(master) in their unbind functions. Since the vc4-drm gets removed before its dependent drivers (vc4_hvs/vc4_v3d) the vc4_hvs_unbind/vc4_v3d_unbind functions try to get drvdata of its master and fails with a null dereference error. > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > index 184d51ea3fa57..e715147d091ca 100644 > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > @@ -1752,7 +1752,7 @@ static int vc4_hvs_bind(struct device *dev, struct > device *master, void *data) > static void vc4_hvs_unbind(struct device *dev, struct device *master, > void *data) > { > - struct drm_device *drm = dev_get_drvdata(master); > + struct drm_device *drm = data; [Severity: High] This is a pre-existing issue, but does vc4_hvs_bind() improperly use devm_ APIs? The component framework leaves the component device bound to its base driver even when the aggregate DRM master unbinds. Since devm_ APIs are attached to the component device (dev), they are only freed when the component device is removed, not when it is unbound. Specifically, devm_request_irq() in vc4_hvs_bind() leaves the IRQ handler registered after unbind. Since vc4_hvs_unbind() nullifies vc4->hvs, will a subsequent interrupt cause vc4_hvs_irq_handler() to dereference a NULL pointer? > struct vc4_dev *vc4 = to_vc4_dev(drm); > struct vc4_hvs *hvs = vc4->hvs; > struct drm_mm_node *node, *next; > diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c > index d31b906cb8e78..f32410420d3e4 100644 > --- a/drivers/gpu/drm/vc4/vc4_v3d.c > +++ b/drivers/gpu/drm/vc4/vc4_v3d.c > @@ -494,7 +494,7 @@ static int vc4_v3d_bind(struct device *dev, struct device > *master, void *data) > static void vc4_v3d_unbind(struct device *dev, struct device *master, > void *data) > { > - struct drm_device *drm = dev_get_drvdata(master); > + struct drm_device *drm = data; [Severity: High] This is a pre-existing issue, but does vc4_v3d_bind() leak memory on unbind? Since devm_ APIs attached to the component device (&pdev->dev) are only freed when the component device is removed (not unbound), does the devm_kzalloc() in vc4_v3d_bind() leak memory on every unbind/bind cycle? > struct vc4_dev *vc4 = to_vc4_dev(drm); > > vc4_irq_uninstall(drm); [Severity: High] This is a pre-existing issue, but can this code access hardware while it is runtime suspended? When the DRM driver is unbound, vc4_v3d_unbind() is called. The V3D hardware may be in an autosuspended state because vc4_v3d_bind() enables runtime PM autosuspend. This calls vc4_irq_uninstall() (which accesses V3D_INTDIS and V3D_INTCTL) and subsequently writes to V3D_BPOA and V3D_BPOS. Will accessing these registers without a preceding pm_runtime_get_sync() to wake the hardware cause an AXI bus timeout and system hang on ARM systems? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
