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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux