Re: [PATCH v1 11/27] drm/panthor: Add AM_SYSTEM platform driver

[sashiko-bot@kernel.org]

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 <karunika.choo@arm.com>

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/20260922204535.2850094-1-karunika.choo@arm.com?part=11