Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

On Wed, Jan 18, 2023 at 10:26:16AM +0100, Johan Hovold wrote:

On Tue, Jan 17, 2023 at 10:34:07PM +0100, Thomas Gleixner wrote:

quoted

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

quoted

Avoid looking for an existing mapping twice when creating a new mapping
using irq_create_fwspec_mapping() by factoring out the actual allocation
which is shared with irq_create_mapping_affinity().

This changelog is incomplete and it took me a while to figure out why
this is before the race fix.

The point is that you need __irq_create_mapping_affinity() later to fix
the shared mapping race. The double check avoidance is just a nice side
effect.

So please spell it out and make it clear that this needs to be
backported too, e.g. by adding:

The split out internal function will be used to fix a shared interrupt
mapping race. This change is therefore tagged with the same fixes tag.

Fixes: ....

Sure. It was originally part of the fix of the race, but I was told to
clean up the code first (and not worry about backporting).

I'll see what I can do about reordering these again with the aim of
making things easier to backport.

quoted

quoted

+static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
+						  irq_hw_number_t hwirq,
+						  const struct irq_affinity_desc *affinity)

Please rename to irq_create_mapping_affinity_locked() so it's clear what
this is about and what the calling convention is. A lockdep assert to
that effect would be nice too.

Will do.

Actually this cannot be done as part of this patch as the function is
still being called without the lock held until the actual
shared-interrupt mapping fix. I have a vague recollection that this was
part of the reason I went with the double underscore prefix.

I'll rename the function using a __locked suffix as part of the race
fix, but a lockdep assert feels like overkill here as this static
function is only in two places where the lock has just been taken (and
the asserts in the revmap helper will eventually catch any future
hypothetical offenders).

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel