Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Skipping drm_mode_config_helper_resume() on hardware resume failure leaks the DRM atomic suspend state and breaks future suspends. - [High] Unbalanced hardware state causes a bus hang on the next system suspend if komeda_dev_resume() fails. Pre-existing issues: - [High] Ignored failure of connect_iommu() in komeda_dev_resume() leads to false-success and invalid DMA. -- commit f5ccdd439f7c420a2895a7d27b82a5ce2f112b7d Author: Gustavo Kenji Mendonça Kaneko <[email protected]> drm/arm/komeda: fix error handling for clk_prepare_enable() and callers This commit adds error checking to clk_prepare_enable() in komeda_dev_resume() and propagates the error to callers in the probe and system PM resume paths. Link: https://patch.msgid.link/[email protected] > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c > b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c > index 5ba62e637a616..9aad1d1d28ec0 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c > @@ -313,7 +313,11 @@ void komeda_dev_destroy(struct komeda_dev *mdev) > > int komeda_dev_resume(struct komeda_dev *mdev) > { > - clk_prepare_enable(mdev->aclk); > + int err; > + > + err = clk_prepare_enable(mdev->aclk); > + if (err) > + return err; > > mdev->funcs->enable_irq(mdev); [Severity: High] This is a pre-existing issue, but does ignoring the failure of connect_iommu() further down in komeda_dev_resume() lead to a false-success? If mdev->funcs->connect_iommu(mdev) fails, the function logs an error but still returns 0. The DRM core would then proceed with display initialization on an unattached IOMMU, which could result in unintended DMA behavior and IOMMU fault storms. [ ... ] > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > index 358c1512b0879..fc1816c634087 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c [ ... ] > @@ -139,11 +142,12 @@ static int __maybe_unused komeda_pm_suspend(struct > device *dev) > static int __maybe_unused komeda_pm_resume(struct device *dev) > { > struct komeda_drv *mdrv = dev_get_drvdata(dev); > + int err = 0; > > if (!pm_runtime_status_suspended(dev)) > - komeda_dev_resume(mdrv->mdev); > + err = komeda_dev_resume(mdrv->mdev); > > - return drm_mode_config_helper_resume(&mdrv->kms->base); > + return err ? err : drm_mode_config_helper_resume(&mdrv->kms->base); [Severity: High] If komeda_dev_resume() fails, this early return bypasses drm_mode_config_helper_resume(), which is required to free and clear dev->mode_config.suspend_state. Could this leak the DRM atomic suspend state memory and permanently break subsequent suspends with -EBUSY? [Severity: High] Additionally, if komeda_pm_resume() returns an error early, the PM core still marks the device as active. On the next system suspend, komeda_pm_suspend() would call komeda_dev_suspend(), which unconditionally writes to MMIO via disable_irq(). Could this unbalanced hardware state cause a bus hang or crash on the next suspend cycle due to unclocked MMIO access? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
