[PATCH 0/9] irqchip/gic: generalize use of HW-based retriggering

[PATCH 0/9] irqchip/gic: generalize use of HW-based retriggering

[Marc Zyngier]

Valentin recently pointed out that that relying on SW-based retrigger
with any of the GIC interrupt controller is both inefficient and
slightly broken, as it messes the GIC's own state machine.

In order to move the GIC over to use its natural HW-based triggering
mechanism, we need to teach all the stacked interrupt controllers that
can pile on a GIC to use the hierarchy-based retrigger helper. This
includes the bus-specific irqchips, such as PCI, FSL-MC, and the funky
platform-MSI.

Marc Zyngier (7):
  irqchip/git-v3-its: Implement irq_retrigger callback for
    device-triggered LPIs
  PCI/MSI: Provide default retrigger callback
  platform-msi: Provide default retrigger callback
  fsl-msi: Provide default retrigger callback
  irqchip/mbigen: Use hierarchy retrigger helper
  irqchip/mvebu-icu: Use hierarchy retrigger helper
  irqchip/mvebu-sei: Use hierarchy retrigger helper

Valentin Schneider (2):
  irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger()
  irqchip/gic-v2, v3: Prevent SW resends entirely

 drivers/base/platform-msi.c      |  2 ++
 drivers/bus/fsl-mc/fsl-mc-msi.c  |  2 ++
 drivers/irqchip/irq-gic-v3-its.c |  6 ++++++
 drivers/irqchip/irq-gic-v3.c     | 12 +++++++++++-
 drivers/irqchip/irq-gic.c        | 12 +++++++++++-
 drivers/irqchip/irq-mbigen.c     |  1 +
 drivers/irqchip/irq-mvebu-icu.c  |  2 ++
 drivers/irqchip/irq-mvebu-sei.c  |  2 ++
 drivers/pci/msi.c                |  2 ++
 9 files changed, 39 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 1/9] irqchip/gic-v2, v3: Implement irq_chip->irq_retrigger()

[Marc Zyngier]

From: Valentin Schneider <valentin.schneider@arm.com>

While digging around IRQCHIP_EOI_IF_HANDLED and irq/resend.c, it has come
to my attention that the IRQ resend situation seems a bit precarious for
the GIC(s).

When marking an IRQ with IRQS_PENDING, handle_fasteoi_irq() will bail out
and issue an irq_eoi(). Should the IRQ in question be re-enabled,
check_irq_resend() will trigger a SW resend, which will go through the flow
handler again and issue *another* irq_eoi() on the *same* IRQ
activation. This is something the GIC spec clearly describes as a bad idea:
any EOI must match a previous ACK.

Implement irq_chip.irq_retrigger() for the GIC chips by setting the GIC
pending bit of the relevant IRQ. After being called by check_irq_resend(),
this will eventually trigger a *new* interrupt which we will handle as usual.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20200730170321.31228-2-valentin.schneider@arm.com
---
 drivers/irqchip/irq-gic-v3.c | 7 +++++++
 drivers/irqchip/irq-gic.c    | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 324f280ff606..b507bc7c5cda 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1207,6 +1207,11 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 #define gic_smp_init()		do { } while(0)
 #endif
 
+static int gic_retrigger(struct irq_data *data)
+{
+	return !gic_irq_set_irqchip_state(data, IRQCHIP_STATE_PENDING, true);
+}
+
 #ifdef CONFIG_CPU_PM
 static int gic_cpu_pm_notifier(struct notifier_block *self,
 			       unsigned long cmd, void *v)
@@ -1242,6 +1247,7 @@ static struct irq_chip gic_chip = {
 	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
 	.irq_set_affinity	= gic_set_affinity,
+	.irq_retrigger          = gic_retrigger,
 	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 	.irq_nmi_setup		= gic_irq_nmi_setup,
@@ -1258,6 +1264,7 @@ static struct irq_chip gic_eoimode1_chip = {
 	.irq_eoi		= gic_eoimode1_eoi_irq,
 	.irq_set_type		= gic_set_type,
 	.irq_set_affinity	= gic_set_affinity,
+	.irq_retrigger          = gic_retrigger,
 	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a27ba2cc1dce..e92ee2b6d7a5 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -347,6 +347,11 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 }
 #endif
 
+static int gic_retrigger(struct irq_data *data)
+{
+	return !gic_irq_set_irqchip_state(data, IRQCHIP_STATE_PENDING, true);
+}
+
 static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 {
 	u32 irqstat, irqnr;
@@ -417,6 +422,7 @@ static const struct irq_chip gic_chip = {
 	.irq_unmask		= gic_unmask_irq,
 	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
+	.irq_retrigger          = gic_retrigger,
 	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 	.flags			= IRQCHIP_SET_TYPE_MASKED |
-- 
2.27.0

[PATCH 2/9] irqchip/git-v3-its: Implement irq_retrigger callback for device-triggered LPIs

[Marc Zyngier]

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 95f097448f97..2808545a963e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1720,6 +1720,11 @@ static int its_irq_set_irqchip_state(struct irq_data *d,
 	return 0;
 }
 
+static int its_irq_retrigger(struct irq_data *d)
+{
+	return !its_irq_set_irqchip_state(d, IRQCHIP_STATE_PENDING, true);
+}
+
 /*
  * Two favourable cases:
  *
@@ -1971,6 +1976,7 @@ static struct irq_chip its_irq_chip = {
 	.irq_set_affinity	= its_set_affinity,
 	.irq_compose_msi_msg	= its_irq_compose_msi_msg,
 	.irq_set_irqchip_state	= its_irq_set_irqchip_state,
+	.irq_retrigger		= its_irq_retrigger,
 	.irq_set_vcpu_affinity	= its_irq_set_vcpu_affinity,
 };
 
-- 
2.27.0

[PATCH 3/9] PCI/MSI: Provide default retrigger callback

[Marc Zyngier]

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pci/msi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 30ae4ffda5c1..c4d31ce2d951 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1446,6 +1446,8 @@ static void pci_msi_domain_update_chip_ops(struct msi_domain_info *info)
 		chip->irq_mask = pci_msi_mask_irq;
 	if (!chip->irq_unmask)
 		chip->irq_unmask = pci_msi_unmask_irq;
+	if (!chip->irq_retrigger)
+		chip->irq_retrigger = irq_chip_retrigger_hierarchy;
 }
 
 /**
-- 
2.27.0

[PATCH 4/9] platform-msi: Provide default retrigger callback

[Marc Zyngier]

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/base/platform-msi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index c4a17e5edf8b..0a043936b259 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -101,6 +101,8 @@ static void platform_msi_update_chip_ops(struct msi_domain_info *info)
 		chip->irq_set_affinity = msi_domain_set_affinity;
 	if (!chip->irq_write_msi_msg)
 		chip->irq_write_msi_msg = platform_msi_write_msg;
+	if (!chip->irq_retrigger)
+		chip->irq_retrigger = irq_chip_retrigger_hierarchy;
 	if (WARN_ON((info->flags & MSI_FLAG_LEVEL_CAPABLE) &&
 		    !(chip->flags & IRQCHIP_SUPPORTS_LEVEL_MSI)))
 		info->flags &= ~MSI_FLAG_LEVEL_CAPABLE;
-- 
2.27.0

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

[Marc Zyngier]

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;
 }
 
 /**
-- 
2.27.0

[PATCH 6/9] irqchip/mbigen: Use hierarchy retrigger helper

[Marc Zyngier]

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-mbigen.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index ff7627b57772..8af35225b46d 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -142,6 +142,7 @@ static struct irq_chip mbigen_irq_chip = {
 	.irq_eoi =		mbigen_eoi_irq,
 	.irq_set_type =		mbigen_set_type,
 	.irq_set_affinity =	irq_chip_set_affinity_parent,
+	.irq_retrigger = 	irq_chip_retrigger_hierarchy,
 };
 
 static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
-- 
2.27.0

[PATCH 7/9] irqchip/mvebu-icu: Use hierarchy retrigger helper

[Marc Zyngier]

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-mvebu-icu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
index 91adf771f185..02f4cc899824 100644
--- a/drivers/irqchip/irq-mvebu-icu.c
+++ b/drivers/irqchip/irq-mvebu-icu.c
@@ -136,6 +136,7 @@ static struct irq_chip mvebu_icu_nsr_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_set_type		= irq_chip_set_type_parent,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 };
 
 static struct irq_chip mvebu_icu_sei_chip = {
@@ -145,6 +146,7 @@ static struct irq_chip mvebu_icu_sei_chip = {
 	.irq_unmask		= irq_chip_unmask_parent,
 	.irq_set_type		= irq_chip_set_type_parent,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 };
 
 static int
-- 
2.27.0

[PATCH 8/9] irqchip/mvebu-sei: Use hierarchy retrigger helper

[Marc Zyngier]

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-mvebu-sei.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-mvebu-sei.c b/drivers/irqchip/irq-mvebu-sei.c
index 18832ccc8ff8..4fc258649785 100644
--- a/drivers/irqchip/irq-mvebu-sei.c
+++ b/drivers/irqchip/irq-mvebu-sei.c
@@ -133,6 +133,7 @@ static struct irq_chip mvebu_sei_ap_irq_chip = {
 	.irq_unmask		= irq_chip_unmask_parent,
 	.irq_set_affinity       = irq_chip_set_affinity_parent,
 	.irq_set_type		= mvebu_sei_ap_set_type,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 };
 
 static void mvebu_sei_cp_compose_msi_msg(struct irq_data *data,
@@ -162,6 +163,7 @@ static struct irq_chip mvebu_sei_cp_irq_chip = {
 	.irq_set_affinity       = irq_chip_set_affinity_parent,
 	.irq_set_type		= mvebu_sei_cp_set_type,
 	.irq_compose_msi_msg	= mvebu_sei_cp_compose_msi_msg,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 };
 
 static int mvebu_sei_domain_alloc(struct irq_domain *domain, unsigned int virq,
-- 
2.27.0

[PATCH 9/9] irqchip/gic-v2, v3: Prevent SW resends entirely

[Marc Zyngier]

From: Valentin Schneider <valentin.schneider@arm.com>

The GIC irqchips can now use a HW resend when a retrigger is invoked by
check_irq_resend(). However, should the HW resend fail, check_irq_resend()
will still attempt to trigger a SW resend, which is still a bad idea for
the GICs.

Prevent this from happening by setting IRQD_HANDLE_ENFORCE_IRQCTX on all
GIC IRQs. Technically per-cpu IRQs do not need this, as their flow handlers
never set IRQS_PENDING, but this aligns all IRQs wrt context enforcement:
this also forces all GIC IRQ handling to happen in IRQ context (as defined
by in_irq()).

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20200730170321.31228-3-valentin.schneider@arm.com
---
 drivers/irqchip/irq-gic-v3.c | 5 ++++-
 drivers/irqchip/irq-gic.c    | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index b507bc7c5cda..4e9387aafed8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1279,6 +1279,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hw)
 {
 	struct irq_chip *chip = &gic_chip;
+	struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
 
 	if (static_branch_likely(&supports_deactivate_key))
 		chip = &gic_eoimode1_chip;
@@ -1296,7 +1297,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		irq_domain_set_info(d, irq, hw, chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
 		irq_set_probe(irq);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+		irqd_set_single_target(irqd);
 		break;
 
 	case LPI_RANGE:
@@ -1310,6 +1311,8 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		return -EPERM;
 	}
 
+	/* Prevents SW retriggers which mess up the ACK/EOI ordering */
+	irqd_set_handle_enforce_irqctx(irqd);
 	return 0;
 }
 
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e92ee2b6d7a5..b59bcef69bf3 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -975,6 +975,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 				irq_hw_number_t hw)
 {
 	struct gic_chip_data *gic = d->host_data;
+	struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(irq));
 
 	if (hw < 32) {
 		irq_set_percpu_devid(irq);
@@ -984,8 +985,11 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
 		irq_set_probe(irq);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+		irqd_set_single_target(irqd);
 	}
+
+	/* Prevents SW retriggers which mess up the ACK/EOI ordering */
+	irqd_set_handle_enforce_irqctx(irqd);
 	return 0;
 }
 
-- 
2.27.0

Re: [PATCH 3/9] PCI/MSI: Provide default retrigger callback

[Bjorn Helgaas]

On Mon, Aug 24, 2020 at 11:23:11AM +0100, Marc Zyngier wrote:
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/msi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 30ae4ffda5c1..c4d31ce2d951 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1446,6 +1446,8 @@ static void pci_msi_domain_update_chip_ops(struct msi_domain_info *info)
>  		chip->irq_mask = pci_msi_mask_irq;
>  	if (!chip->irq_unmask)
>  		chip->irq_unmask = pci_msi_unmask_irq;
> +	if (!chip->irq_retrigger)
> +		chip->irq_retrigger = irq_chip_retrigger_hierarchy;
>  }
>  
>  /**
> -- 
> 2.27.0
> 

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

[Valentin Schneider]

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.


>  }
>
>  /**

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

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

[Marc Zyngier]

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.

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