Re: [PATCH 5/9] fsl-msi: Provide default retrigger callback

[Marc Zyngier]

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...