Re: [PATCH v1 15/27] drm/panthor: Add AM_RESOURCE_GROUP support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Runtime PM handling missing in shared IRQ handler
- [High] Missing synchronize_irq() during runtime suspend
- [High] CPU busy-loop in message retry workqueue
- [Medium] Missing NULL pointer checks for devm_kasprintf allocations
- [Low] Uninitialized irq field in panthor_resource_group
--

commit 97f87eba41075ad8e7f232b378d4561b841f68e5
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Add AM_RESOURCE_GROUP support

Add resource-group probing for the arbitration device, including MMIO
resources, IRQ handling, per-AW AM message helpers, handshake
negotiation, and retry work for deferred outgoing messages. Hook
resource groups into arbitration init, teardown, and runtime PM.

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c b/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c
> @@ -0,0 +1,323 @@
[ ... ]
> +static void rg_retry_messages(struct work_struct *work)
> +{
[ ... ]
> +	if (retry)
> +		queue_work(rg->wq, &rg->msg_retry_work);

[Severity: High]
Could this sequence lead to a CPU busy-loop? If the hardware has pending
messages and panthor_am_msg_retry() returns -EBUSY or -EAGAIN, this will
immediately requeue the work. Because it is an ordered workqueue and no
delay is specified, does this cause the worker thread to monopolize the
CPU at 100% until the hardware processes the messages?

[ ... ]
> +static irqreturn_t rg_irq_raw_handler(int irq, void *data)
> +{
> +	struct panthor_resource_group *rg = data;
> +	unsigned long message_mask;
> +	u32 status;
> +	u8 aw_id;
> +
> +	status = gpu_read(rg->iomem, AM_RG_IRQ_STATUS);
> +	if (!status)
> +		return IRQ_NONE;

[Severity: High]
Is it safe to perform this MMIO read without verifying device activity?
Since the IRQ is registered with IRQF_SHARED, this handler can be invoked
by other devices on the same line while this device is in runtime suspend.

If the device is powered down, the read will typically return ~0u, which
bypasses the !status check and could result in spurious processing of 16
pending messages. Should this use pm_runtime_get_if_active() to verify
the device is awake before accessing hardware?

[ ... ]
> +int panthor_resource_group_suspend(struct panthor_arbitration *adev)
> +{
> +	for (int i = 0; i < AM_ARB_MAX_RG_COUNT; i++) {
> +		struct panthor_resource_group *rg = adev->rg[i];
> +
> +		if (!rg)
> +			continue;
> +
> +		rg_irq_suspend(rg);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does this suspend sequence need to wait for in-flight interrupts? The IRQ
is masked at the device level, but since it is registered as IRQF_SHARED,
rg_irq_raw_handler() might still be executing concurrently on another CPU.
Should there be a synchronize_irq() call here to prevent the device from
powering down while the handler is still running?

[ ... ]
> +static int resource_group_init(struct panthor_arbitration *adev,
> +			       const struct fwnode_handle *child)
> +{
[ ... ]
> +	rg = devm_kzalloc(dev, sizeof(*rg), GFP_KERNEL);
> +	if (!rg)
> +		return -ENOMEM;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "rg%d", res.id);

[Severity: Medium]
Could name be NULL here under memory pressure? devm_kasprintf() isn't
checked for allocation failure before being passed into functions expecting
valid strings, like devm_alloc_ordered_workqueue() below.

[ ... ]
> +	irq = fwnode_irq_get(child, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "%s: Failed to get IRQ", name);
> +
> +	irqdata = irq_get_irq_data(irq);
> +	if (irqdata)
> +		irq_flags = irqd_get_trigger_type(irqdata);

[Severity: Low]
Should the retrieved IRQ number be stored in rg->irq here? The local irq
variable is populated and used for requesting the interrupt, but it doesn't
appear to be assigned to the structure field for future use.

> +
> +	ret = devm_request_irq(
> +		dev, irq, rg_irq_raw_handler, irq_flags | IRQF_SHARED,
> +		devm_kasprintf(dev, GFP_KERNEL, "panthor-%s-irq", name), rg);

[Severity: Medium]
Similar to the earlier devm_kasprintf() call, could this inner allocation
fail and pass a NULL device name to devm_request_irq()?

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