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

[Marc Zyngier]

On Wed, 26 Aug 2020 17:37:30 +0100,
Marc Zyngier <maz@xxxxxxxxxx> 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@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:

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.

-- 
Without deviation from the norm, progress is not possible.