Re: [PATCH v4 8/8] irqchip/al-fic: add irq_set_affinity callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] irqchip: al-fic unconditionally returns IRQ_SET_MASK_OK_DONE without updating effective affinity
- [Low] irqchip: data race when reading parent irq affinity mask in al-fic
--

commit dc7525463e65d6270acce3615164b09eeb03dc09
Author: Eliav Farber <farbere@amazon.com>

irqchip/al-fic: add irq_set_affinity callback

This commit adds an irq_set_affinity callback to the al-fic irqchip that
unconditionally returns IRQ_SET_MASK_OK_DONE. This prevents drivers from
failing to probe when their interrupt is routed through a FIC instance,
which lacks per-child affinity control.

> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> index 141d9d492d43b..1ac360cf9404d 100644
> --- a/drivers/irqchip/irq-al-fic.c
> +++ b/drivers/irqchip/irq-al-fic.c

[ ... ]

> @@ -157,6 +157,36 @@ static int al_fic_irq_retrigger(struct irq_data *data)

[ ... ]

> +static int al_fic_irq_set_affinity(struct irq_data *data,
> +				   const struct cpumask *mask, bool force)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
> +	const struct cpumask *parent_affinity;
> +	struct al_fic *fic = gc->private;
> +	struct irq_data *parent_data;
> +
> +	parent_data = irq_get_irq_data(fic->parent_irq);
> +	if (!parent_data)
> +		return -EINVAL;
> +
> +	parent_affinity = irq_data_get_affinity_mask(parent_data);
> +	if (!cpumask_equal(mask, parent_affinity))
> +		pr_warn_once("%pOF: affinity change requested but FIC is fixed to parent IRQ %u CPU routing\n",
> +			     fic->node, fic->parent_irq);

[Severity: Low]
Does this execute a lockless read of the parent's cpumask without holding the
parent's desc->lock or wrapping the access in a data_race() annotation?

When a user writes to /proc/irq/<child>/smp_affinity, it calls
al_fic_irq_set_affinity() under the child's desc->lock and locklessly reads
the parent's mask. If another thread concurrently writes to
/proc/irq/<parent>/smp_affinity, it updates the parent's mask under the
parent's desc->lock.

Could this cause an unannotated data race that triggers KCSAN splats and
potentially causes a torn read resulting in a spurious warning message?

If a lock cannot be taken due to lock-inversion risks, would it make sense
to add a data_race() annotation here?

> +
> +	return IRQ_SET_MASK_OK_DONE;

[Severity: Medium]
Since this callback returns IRQ_SET_MASK_OK_DONE unconditionally without
calling irq_data_update_effective_affinity(), does the child's effective
mask remain empty?

Generic IRQ descriptors allocate the effective affinity mask with
zero-initialization. Returning success without propagating the parent's
affinity into the child's effective affinity mask leaves it empty.

This unconditionally triggers a core kernel warning in
irq_validate_effective_affinity() (irq_chip AL-FIC did not update eff.
affinity mask).

Additionally, an empty effective mask breaks threaded IRQ affinity updates.
irq_thread_check_affinity() copies the empty mask and passes it to
set_cpus_allowed_ptr(), which fails with -EINVAL, causing threaded handlers
to remain on arbitrary CPUs instead of following the hardware routing.

Would it be better to call irq_data_update_effective_affinity() before
returning?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008090058.38591-1-farbere@amazon.com?part=8