Re: [PATCH v2 2/4] irqchip: irq-eyeq5-msi: add EyeQ5 PCIe MSI controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] irqchip: eyeq5-msi: wrong flow handler in eyeq_msi_domain_alloc()
--

commit 6649bd5a917b478cbaaf4b595ae6e980b906705c
Author: Gregory CLEMENT <gregory.clement@bootlin.com>

irqchip: irq-eyeq5-msi: add EyeQ5 PCIe MSI controller driver

This commit adds an interrupt controller driver for the Mobileye EyeQ5 PCIe
MSI snooper, supporting up to 4 MSI vectors per port in either root complex
(PCI MSI parent) or endpoint (platform MSI parent) mode.

> diff --git a/drivers/irqchip/irq-eyeq5-msi.c b/drivers/irqchip/irq-eyeq5-msi.c
> new file mode 100644
> index 0000000000000..f56e130393cb2
> --- /dev/null
> +++ b/drivers/irqchip/irq-eyeq5-msi.c

[ ... ]

> +static struct irq_chip eyeq_msi_irq_chip = {
> +	.name			= "EyeQ5-MSI",
> +	.irq_mask		= eyeq_msi_mask_irq,
> +	.irq_unmask		= eyeq_msi_unmask_irq,
> +	.irq_compose_msi_msg	= eyeq_msi_compose_msg,
> +#ifdef CONFIG_SMP
> +	.irq_set_affinity	= irq_chip_redirect_set_affinity,
> +#endif
> +};
> +
> +static void eyeq_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *parent_chip = irq_desc_get_chip(desc);
> +	struct eyeq_msi *msi = irq_desc_get_handler_data(desc);
> +	void __iomem *status_reg;
> +	unsigned long pending;
> +	unsigned int bit;
> +
> +	chained_irq_enter(parent_chip, desc);
> +
> +	status_reg = msi->gcsr_base + GCSR_IRQSTATUS;
> +
> +	while (true) {
> +		pending = FIELD_GET(GCSR_IRQSTATUS_MASK, readw(status_reg));
> +		if (!pending)
> +			break;
> +
> +		/*
> +		 * Write-one-to-clear status register.
> +		 */
> +		writew(FIELD_PREP(GCSR_IRQSTATUS_MASK, pending), status_reg);
> +
> +		for_each_set_bit(bit, &pending, EYEQ_MSI_MAX_VECTORS) {
> +			if (!generic_handle_demux_domain_irq(msi->msi_domain, bit))
> +				pr_err_ratelimited("unmapped MSI interrupt %u\n", bit);
> +		}
> +	}
> +
> +	chained_irq_exit(parent_chip, desc);
> +}
> +
> +static int eyeq_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				 unsigned int nr_irqs, void *args)
> +{

[ ... ]

> +	i = 0;
> +	for_each_set_bit(bit, reserved, EYEQ_MSI_MAX_VECTORS) {
> +		irq_domain_set_info(domain, virq + i++, bit, &eyeq_msi_irq_chip,
> +				    msi, handle_simple_irq, NULL, NULL);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Should eyeq_msi_domain_alloc() in drivers/irqchip/irq-eyeq5-msi.c configure
handle_edge_irq (along with an ack/pre_redirect callback on
eyeq_msi_irq_chip) instead of handle_simple_irq?

Using handle_simple_irq here appears to cause two issues when MSI interrupts
are dispatched through eyeq_msi_isr() -> generic_handle_demux_domain_irq():

First, neither eyeq_msi_irq_chip nor the child MSI chip templates configured
via msi_lib_init_dev_msi_info() implement .irq_disable, and
IRQ_DISABLE_UNLAZY is not set. When a consumer calls disable_irq() or
disable_irq_nosync(), irq_disable() -> __irq_disable() only sets
IRQD_IRQ_DISABLED in software and leaves the vector unmasked in
GCSR_IRQMASK, relying on the flow handler to mask the hardware line if an
interrupt fires while disabled.

When an MSI arrives while disabled, eyeq_msi_isr() clears GCSR_IRQSTATUS and
dispatches to handle_simple_irq():

kernel/irq/chip.c:handle_simple_irq() {
	guard(raw_spinlock)(&desc->lock);

	if (!irq_can_handle_pm(desc))
		return;

	if (!irq_can_handle_actions(desc)) {
		if (irqd_needs_resend_when_in_progress(&desc->irq_data))
			desc->istate |= IRQS_PENDING;
		return;
	}
	...
}

Because handle_simple_irq() returns without calling mask_irq(),
eyeq_msi_mask_irq() is never invoked, GCSR_IRQMASK remains unmasked, and
subsequent MSI writes while disabled will keep raising the parent GIC
interrupt and queueing cross-CPU redirect irq_work.

Second, eyeq_msi_isr() clears GCSR_IRQSTATUS before dispatching the vector
and exits chained_irq_exit(), while the target CPU runs handle_simple_irq()
-> handle_irq_event() with IRQD_IRQ_INPROGRESS set and desc->lock released.

If another MSI for the same vector arrives on a different CPU while
IRQD_IRQ_INPROGRESS is set (for example across an affinity change or when
effective_affinity spans multiple CPUs), handle_simple_irq() checks
irq_can_handle_pm():

kernel/irq/chip.c:irq_can_handle_pm() {
	if (irqd_irq_inprogress(&d->irq_data) && d->handle_irq != handle_edge_irq)
		return false;
	...
}

Since desc->handle_irq is not handle_edge_irq and
irqd_needs_resend_when_in_progress() is not set, handle_simple_irq() returns
without marking IRQS_PENDING or looping in handle_irq_event(), dropping the
second MSI interrupt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-eyeq5_msi-v2-0-4aa3d3608064@bootlin.com?part=2