Hi Valentin,
On 2020-08-26 12:16, Valentin Schneider wrote:
Hi Marc,
Many thanks for picking this up!
Below's the only comment I have, the rest LGTM.
On 24/08/20 11:23, Marc Zyngier wrote:
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
drivers/bus/fsl-mc/fsl-mc-msi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c
b/drivers/bus/fsl-mc/fsl-mc-msi.c
index 8edadf05cbb7..5306ba7dea3e 100644
--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -144,6 +144,8 @@ static void fsl_mc_msi_update_chip_ops(struct
msi_domain_info *info)
*/
if (!chip->irq_write_msi_msg)
chip->irq_write_msi_msg = fsl_mc_msi_write_msg;
+ if (!chip->irq_retrigger)
+ chip->irq_retrigger = irq_chip_retrigger_hierarchy;
AFAICT the closest generic hook we could use here is
msi_create_irq_domain() -> msi_domain_update_chip_ops()
which happens just below the fsl-specific ops update.
However, placing a default .irq_retrigger callback in there would
affect any
and all MSI domain. IOW that would cover PCI and platform MSIs (covered
by
separate patches in this series), but also some x86 ("dmar" & "hpet")
and
TI thingies.
I can't tell right now how bad of an idea it is, but I figured I'd
throw
this out there.
The problem with this approach is that it requires the resend path to be
cooperative and actually check for more than the top-level irq_data.
Otherwise you'd never actually trigger the HW resend if it is below
the top level.
But I like the idea though. Something like this should do the trick, and
is admittedly a bug fix:
diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index c48ce19a257f..d11c729f9679 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -86,6 +86,18 @@ static int irq_sw_resend(struct irq_desc *desc)
}
#endif
+static int try_retrigger(struct irq_desc *desc)
+{
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ return irq_chip_retrigger_hierarchy(&desc->irq_data);
+#else
+ if (desc->irq_data.chip->irq_retrigger)
+ return desc->irq_data.chip->irq_retrigger(&desc->irq_data);
+
+ return 0;
+#endif
+}
+
/*
* IRQ resend
*
@@ -113,8 +125,7 @@ int check_irq_resend(struct irq_desc *desc, bool
inject)
desc->istate &= ~IRQS_PENDING;
- if (!desc->irq_data.chip->irq_retrigger ||
- !desc->irq_data.chip->irq_retrigger(&desc->irq_data))
+ if (!try_retrigger(desc))
err = irq_sw_resend(desc);
/* If the retrigger was successfull, mark it with the REPLAY bit */
In general, introducing a irq_chip_retrigger_hierarchy() call
shouldn't be problematic as long as we don't overwrite an existing
callback.
I'll have a look at respining the series with that in mind.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
