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().
Tested-by: Hsin-Yi Wang <[email protected]>
Tested-by: Mark-PK Tsai <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
kernel/irq/irqdomain.c | 60 +++++++++++++++++++++++-------------------
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 248e6acfafbe..894bc6ee6348 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -675,6 +675,34 @@ unsigned int irq_create_direct_mapping(struct irq_domain *domain)
EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
#endif
+static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
+ irq_hw_number_t hwirq,
+ const struct irq_affinity_desc *affinity)
+{
+ struct device_node *of_node = irq_domain_get_of_node(domain);
+ int virq;
+
+ pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
+
+ /* Allocate a virtual interrupt number */
+ virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
+ affinity);
+ if (virq <= 0) {
+ pr_debug("-> virq allocation failed\n");
+ return 0;
+ }
+
+ if (irq_domain_associate(domain, virq, hwirq)) {
+ irq_free_desc(virq);
+ return 0;
+ }
+
+ pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
+ hwirq, of_node_full_name(of_node), virq);
+
+ return virq;
+}
+
/**
* irq_create_mapping_affinity() - Map a hardware interrupt into linux irq space
* @domain: domain owning this hardware interrupt or NULL for default domain
@@ -687,14 +715,11 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
* on the number returned from that call.
*/
unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
- irq_hw_number_t hwirq,
- const struct irq_affinity_desc *affinity)
+ irq_hw_number_t hwirq,
+ const struct irq_affinity_desc *affinity)
{
- struct device_node *of_node;
int virq;
- pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
-
/* Look for default domain if necessary */
if (domain == NULL)
domain = irq_default_domain;
@@ -702,34 +727,15 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
return 0;
}
- pr_debug("-> using domain @%p\n", domain);
-
- of_node = irq_domain_get_of_node(domain);
/* Check if mapping already exists */
virq = irq_find_mapping(domain, hwirq);
if (virq) {
- pr_debug("-> existing mapping on virq %d\n", virq);
+ pr_debug("existing mapping on virq %d\n", virq);
return virq;
}
- /* Allocate a virtual interrupt number */
- virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
- affinity);
- if (virq <= 0) {
- pr_debug("-> virq allocation failed\n");
- return 0;
- }
-
- if (irq_domain_associate(domain, virq, hwirq)) {
- irq_free_desc(virq);
- return 0;
- }
-
- pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
- hwirq, of_node_full_name(of_node), virq);
-
- return virq;
+ return __irq_create_mapping_affinity(domain, hwirq, affinity);
}
EXPORT_SYMBOL_GPL(irq_create_mapping_affinity);
@@ -834,7 +840,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
return 0;
} else {
/* Create mapping */
- virq = irq_create_mapping(domain, hwirq);
+ virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
if (!virq)
return virq;
}
--
2.38.2
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: ....
>
> +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.
Thanks,
tglx
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.
Johan
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