Re: [PATCH v2 14/28] drm/panthor: Add AM_PARTITION_CONTROL support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] drm/panthor: TOCTOU race from multiple AM_PART_STATE reads in
  window helpers
- [Medium] drm/panthor: panthor_partition_control_term() fails on
  in-progress state transitions
- [Low] drm/panthor: unused devm allocations in partition_control_init()
  on -ENODEV
- [Low] drm/panthor: unchecked devm_kasprintf() in
  partition_control_init()
--

commit 9f7b5a10199859069acf5b5904f5b9aca326fc89
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Add AM_PARTITION_CONTROL support

This commit adds the Panthor arbitration platform driver and partition
control support to manage access windows on Mali v15 AM GPUs.

Link: https://lore.kernel.org/all/20260922205802.1AC3E1F000FF@smtp.kernel.org/

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c b/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
> new file mode 100644
> index 0000000000000..330d0bf5fd375
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c

[ ... ]

> +static u32 partition_state_get(struct panthor_partition_control *pc)
> +{
> +	const u32 state = gpu_read(pc->iomem, AM_PART_STATE);
> +
> +	return AM_PART_STATE_GET(state);
> +}
> +
> +static u8 partition_aw_get(struct panthor_partition_control *pc)
> +{
> +	const u32 state = gpu_read(pc->iomem, AM_PART_STATE);
> +
> +	return AM_PART_STATE_WINDOW_GET(state);
> +}

[ ... ]

> +static int yield_now(struct panthor_partition_control *pc)
> +{
> +	guard(spinlock_irqsave)(&pc->lock);
> +
> +	if (partition_state_get(pc) == PART_STATE_RESET)
> +		return 0;
> +
> +	if (partition_state_get(pc) != PART_STATE_WINDOW_OPEN)
> +		return -EAGAIN;
> +
> +	gpu_write(pc->iomem, AM_PART_COMMAND,
> +		  AM_PART_SET_COMMAND(PART_CMD_YIELD_NOW));
> +
> +	return 0;
> +}
> +
> +static int yield_idle(struct panthor_partition_control *pc)
> +{
> +	guard(spinlock_irqsave)(&pc->lock);
> +
> +	if (partition_state_get(pc) == PART_STATE_RESET)
> +		return 0;
> +
> +	if (partition_state_get(pc) != PART_STATE_WINDOW_OPEN)
> +		return -EAGAIN;
> +
> +	gpu_write(pc->iomem, AM_PART_COMMAND,
> +		  AM_PART_SET_COMMAND(PART_CMD_YIELD_IDLE));
> +
> +	return 0;
> +}
> +
> +static int window_close(struct panthor_partition_control *pc)
> +{
> +	guard(spinlock_irqsave)(&pc->lock);
> +
> +	if (partition_state_get(pc) == PART_STATE_RESET)
> +		return 0;
> +
> +	if (partition_state_get(pc) != PART_STATE_WINDOW_OPEN)
> +		return -EAGAIN;
> +
> +	pc->closing = true;
> +
> +	gpu_write(pc->iomem, AM_PART_COMMAND,
> +		  AM_PART_SET_COMMAND(PART_CMD_CLOSE_WINDOW));
> +
> +	return 0;
> +}
> +
> +static int window_open(struct panthor_partition_control *pc, u8 aw_id)
> +{
> +	if (aw_id >= AM_ARB_MAX_AW_COUNT)
> +		return -EINVAL;
> +
> +	guard(spinlock_irqsave)(&pc->lock);
> +
> +	if (partition_state_get(pc) == PART_STATE_WINDOW_OPEN) {
> +		u8 cur_aw = partition_aw_get(pc);
> +
> +		if (cur_aw == aw_id)
> +			return 0;
> +
> +		dev_warn(pc->dev,
> +			 "%s: OPEN_WINDOW(%u): Already opened for aw=%u, state=%u. Closing...",
> +			 pc->name, aw_id, cur_aw, partition_state_get(pc));
> +
> +		return -EBUSY;
> +	}

[Severity: Medium]
Can an asynchronous hardware state transition between these back-to-back
AM_PART_STATE reads cause yield_now(), yield_idle(), window_close(), and
window_open() to act on inconsistent state?

Each call to partition_state_get() and partition_aw_get() performs a
separate MMIO read of AM_PART_STATE.  If the partition finishes closing or
yielding and transitions into PART_STATE_RESET between the first and
second partition_state_get() in yield_now(), yield_idle(), or
window_close(), the first check is false and the second sees
PART_STATE_RESET != PART_STATE_WINDOW_OPEN, returning -EAGAIN instead of 0
(which in panthor_partition_control_term() triggers WARN_ON() and skips
partition_state_wait()).  In window_open(), if the partition resets
between partition_state_get() and partition_aw_get(), cur_aw reads as 0;
when aw_id is 0, window_open() returns 0 without issuing
PART_CMD_OPEN_WINDOW.

Also, the dev_warn() in window_open() performs a third MMIO read of
AM_PART_STATE and still says "Closing..." even though window_open() no
longer closes the window.  Reading AM_PART_STATE once into a local
variable under pc->lock and extracting both fields from that value would
avoid these races.

[ ... ]

> +void panthor_partition_control_term(struct panthor_arbitration *adev)
> +{
> +	for (int i = 0; i < AM_ARB_MAX_PC_COUNT; i++) {
> +		struct panthor_partition_control *pc = adev->pc[i];
> +
> +		if (!pc)
> +			continue;
> +
> +		partition_irq_suspend(pc);
> +
> +		if (!pm_runtime_status_suspended(pc->dev))
> +			if (!WARN_ON(window_close(pc) < 0))
> +				WARN_ON(partition_state_wait(pc, PART_STATE_RESET));
> +	}
> +}

[Severity: Medium]
What happens in panthor_partition_control_term() if a partition is already
transitioning through PART_STATE_WINDOW_CLOSED or
PART_STATE_WINDOW_OPENING when teardown runs?

window_open(), window_close(), yield_now(), and yield_idle() issue
commands without waiting for the state transition to complete, and
window_close() returns -EAGAIN for any state other than PART_STATE_RESET
and PART_STATE_WINDOW_OPEN.  If a transition is still in flight when
panthor_partition_control_term() runs, window_close() returns -EAGAIN,
triggering WARN_ON() and skipping partition_state_wait() while the
hardware is still active.  Commit "drm/panthor: Reset partitions before
starting arbitration" later in this series adds partition_ensure_reset()
to handle in-progress states during init, so calling it here as well would
ensure teardown waits for the reset to complete.

> +static int partition_control_init(struct panthor_arbitration *adev, int i)
> +{
> +	struct panthor_partition_control *pc;
> +	struct device *dev = adev->dev;
> +	struct irq_data *irqdata;
> +	struct resource *res;
> +	void __iomem *iomem;
> +	u32 irq_flags = 0;
> +	char *name;
> +	int irq;
> +	int ret;
> +
> +	if (adev->pc[i])
> +		return dev_warn_probe(dev, -ENODEV,
> +				      "partition %u already present", i);
> +
> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "pc%d", i);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource_byname(to_platform_device(dev),
> +					   IORESOURCE_MEM, name);
> +	if (!res)
> +		return -ENODEV;

[Severity: Low]
When platform_get_resource_byname() finds no resource and returns -ENODEV,
panthor_partition_control_init() ignores -ENODEV and probe succeeds,
leaving pc and name allocated on dev until the driver unbinds.  Could
pc be allocated after looking up res (or the unused devm allocations be
freed before returning -ENODEV)?

[ ... ]

> +	ret = devm_request_irq(
> +		dev, irq, partition_irq_raw_handler, irq_flags | IRQF_SHARED,
> +		devm_kasprintf(dev, GFP_KERNEL, "panthor-%s-irq", name), pc);
> +	if (ret)
> +		return ret;

[Severity: Low]
Can devm_kasprintf() return NULL here and pass a NULL devname into
devm_request_irq() instead of failing with -ENOMEM?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110153907.3241978-1-karunika.choo@arm.com?part=14