Messages in this thread Patch in this message | | | Date | Wed, 26 Aug 2020 17:37:30 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH 5/9] fsl-msi: Provide default retrigger callback |
| |
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@kernel.org> >> --- >> 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...
| |