Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] gpu: out-of-bounds 64-bit access on 32-bit AM_SYS_IRQ registers - [High] pm: missing clock re-enablement in panthor_system_suspend() error path - [Medium] driver: missing MODULE_DEVICE_TABLE prevents module autoloading - [Medium] pm: missing hibernation callbacks in panthor_system_pm_ops --
commit c920f013092a669426d1524348fe769140c7f85d Author: Karunika Choo <[email protected]> drm/panthor: Add AM_SYSTEM platform driver This commit introduces a small AM_SYSTEM platform driver for Mali v15 AM systems. It provides GPU-wide control, managing shared clock and regulator resources, and performs the initial GPU reset sequence during probe. > diff --git a/drivers/gpu/drm/panthor/system/panthor_system_drv.c > b/drivers/gpu/drm/panthor/system/panthor_system_drv.c > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/panthor/system/panthor_system_drv.c [ ... ] > +static int __panthor_system_reset(struct panthor_system *sdev, u32 cmd) > +{ > + u64 val; > + > + /* Clear stale RESET_COMPLETED IRQs before issuing reset */ > + gpu_write64(sdev->iomem, AM_SYS_IRQ_CLEAR, AM_SYS_IRQ_RESET_COMPLETED); [Severity: High] Does this code perform an out-of-bounds access? The AM_SYS_IRQ registers such as AM_SYS_IRQ_CLEAR (at 0x4C) appear to be 32-bit registers. Using gpu_write64 splits the access across two 32-bit registers, which might clobber the adjacent register at 0x50 with the upper 32 bits. > + > + gpu_write(sdev->iomem, AM_SYS_COMMAND, cmd); > + > + return read_poll_timeout(gpu_read64, val, > + (val & AM_SYS_IRQ_RESET_COMPLETED), > + AM_SYS_RESET_SLEEP_US, AM_RESET_TIMEOUT_US, > false, > + sdev->iomem, AM_SYS_IRQ_RAWSTAT); [Severity: High] Can this similarly cause unintended side-effects? AM_SYS_IRQ_RAWSTAT (at 0x40) is also a 32-bit register. Using gpu_read64 will read from both 0x40 and 0x44, which might not be safe for hardware registers. > +} > + > +static int panthor_system_reset(struct panthor_system *sdev) > +{ > + int ret; > + > + ret = __panthor_system_reset(sdev, AM_SYS_CMD_SOFT_RESET); > + if (ret) { > + dev_err(sdev->dev, "SOFT_RESET failed, attempting HARD_RESET"); > + > + ret = __panthor_system_reset(sdev, AM_SYS_CMD_HARD_RESET); > + if (ret) { > + dev_err(sdev->dev, "HARD_RESET failed"); > + return -EIO; > + } > + } > + > + gpu_write64(sdev->iomem, AM_SYS_IRQ_CLEAR, AM_SYS_IRQ_RESET_COMPLETED); [Severity: High] As mentioned above, should this use the 32-bit gpu_write instead of gpu_write64 to avoid clobbering the register at 0x50? > + > + return 0; > +} > + > +static int panthor_system_suspend(struct device *dev) > +{ > + struct panthor_system *sdev = dev_get_drvdata(dev); > + > + panthor_system_clk_suspend(sdev); > + return panthor_system_regulator_suspend(sdev); [Severity: High] What happens if the regulator suspension fails? The function will return the error directly, but the clock has already been suspended. The PM core might interpret the error to mean the device is still in the RPM_ACTIVE state, but the hardware is physically unclocked. Could this cause bus faults or system crashes on subsequent accesses? Should the clock be re-enabled in the error path? > +} [ ... ] > +static const struct dev_pm_ops panthor_system_pm_ops = { > + .suspend = pm_runtime_force_suspend, > + .resume = pm_runtime_force_resume, > + .runtime_suspend = panthor_system_suspend, > + .runtime_resume = panthor_system_resume, > +}; [Severity: Medium] Does this code handle hibernation correctly? The panthor_system_pm_ops structure manually assigns .suspend and .resume but leaves the hibernation callbacks (.freeze, .thaw, .poweroff, .restore) undefined. During system hibernation or hybrid sleep, the PM core might skip freezing or restoring the device. Should this use DEFINE_RUNTIME_DEV_PM_OPS or SYSTEM_SLEEP_PM_OPS to wire all required sleep callbacks? > + > +static const struct of_device_id panthor_system_dt_match[] = { > + { .compatible = "arm,mali-gen5-am-system" }, > + {} > +}; [Severity: Medium] Is the MODULE_DEVICE_TABLE declaration missing here? Without exporting the match table via MODULE_DEVICE_TABLE(of, panthor_system_dt_match), the module will not have the necessary aliases generated, which prevents udev from automatically loading the module when the compatible hardware is detected. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
