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

[Johan Hovold <johan@kernel.org>]

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:
> > On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > > 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.
> 
> > > +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