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
