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

[Marc Zyngier <maz@kernel.org>]

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