Re: [PATCH v2 2/2] irqchip/irq-realtek-rtl: Add multicore support

[Thomas Gleixner]

On Thu, Jun 04 2026 at 15:06, Markus Stockhausen wrote:

>  static void realtek_ictl_unmask_irq(struct irq_data *i)
>  {
> +	unsigned int cpu;
> +
>	 guard(raw_spinlock)(&irq_lock);
> -	enable_gimr(i->hwirq);
> +	for_each_cpu(cpu, irq_data_get_effective_affinity_mask(i))
> +		enable_gimr(cpu, i->hwirq);
>  }

Ok.

>  static void realtek_ictl_mask_irq(struct irq_data *i)
>  {
> +	unsigned int cpu;
> +
>	 guard(raw_spinlock)(&irq_lock);
> -	disable_gimr(i->hwirq);
> +	for_each_cpu(cpu, &realtek_ictl_cpu_configurable)
> +		disable_gimr(cpu, i->hwirq);
> +}

Why not using the effective mask here? CPUs which are not in the
effective mask are disabled already.

> +static int realtek_ictl_irq_affinity(struct irq_data *i, const struct cpumask *dest, bool force)
> +{
> +	cpumask_t cpu_configure, cpu_disable, cpu_enable;
> +	unsigned int cpu;

Looking deeper at this specific part:

> +	cpumask_and(&cpu_configure, cpu_present_mask, &realtek_ictl_cpu_configurable);

Why do you need that?

cpu_configurable should arguably never contain non-present CPUs.

If you want to protect against a bonkers device tree, then do so in the
probe function.

But ....

> +	cpumask_and(&cpu_enable, &cpu_configure, dest);
> +	cpumask_andnot(&cpu_disable, &cpu_configure, dest);

Assume that cpu_configure and dest do not overlap, then you end up with
_zero_ target CPUs, i.e. a non-working interrupt.

The general asssumption of the core interrupt code is that except for
strict per CPU interrupts, which are managed separately, interrupts can
be routed to any online CPU.

So I think your init logic is broken:

> +		realtek_ictl_base[cpu] = of_iomap(node, cpu);
> +		if (realtek_ictl_base[cpu]) {

If this mapping fails, then you should fail the initialization and your
loop around that should do:

            for_each_present_cpu()

and not try to figure that out via NR_CPUS:

> +	for (unsigned int cpu = 0; cpu < NR_CPUS; cpu++) {

and the failed mapping logic. At the point where the driver is
initialized the present CPU mask is stable and authoritative.

If a mapping for a present CPU fails, then the driver has to fail the
initialization as you otherwise run into the stale interrupt situation
described above.

With that fixed you can drop this whole realtek_ictl_cpu_configurable
dance as the core will never set a non-present CPU in the destination
mask. It even guarantees that the CPUs in the mask are online unless
@force = true. The latter is only for scenarios where pseudo per CPU
interrupts have to be affined before a CPU goes online, so irrelevant
for your use case.

> +	scoped_guard(raw_spinlock, &irq_lock) {
> +		for_each_cpu(cpu, &cpu_disable)
> +			disable_gimr(cpu, i->hwirq);
> +		for_each_cpu(cpu, &cpu_enable) {
> +			if (!irqd_irq_masked(i))
> +				enable_gimr(cpu, i->hwirq);
> +		}
> +	}

This can be simplified to:

	if (!irqd_irq_masked(i))
        	realtek_ictl_mask_irq(i);

	irq_data_update_effective_affinity(i, &dest);

	if (!irqd_irq_masked(i))
        	realtek_ictl_unmask_irq(i);

Sorry that I failed to catch those things right away.

Thanks,

        tglx