| Message ID | 20200824102317.1038259-6-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic: generalize use of HW-based retriggering | expand |
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. > } > > /**
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.
On Wed, 26 Aug 2020 17:37:30 +0100, Marc Zyngier <maz@kernel.org> wrote: > > 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: Well, not quite. > > 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); This only checks the parent, so we still need to evaluate the top-level. Something like this: diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c index c48ce19a257f..8ccd32a0cc80 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) +{ + if (desc->irq_data.chip->irq_retrigger) + return desc->irq_data.chip->irq_retrigger(&desc->irq_data); + +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY + return irq_chip_retrigger_hierarchy(&desc->irq_data); +#else + 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 */ With that, I believe we can drop most of the patches in this series and only keep the GIC ones. M.
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; } /**
Signed-off-by: Marc Zyngier <maz@kernel.org> --- drivers/bus/fsl-mc/fsl-mc-msi.c | 2 ++ 1 file changed, 2 insertions(+)