lkml.org 
[lkml]   [2020]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 5/9] fsl-msi: Provide default retrigger callback
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...

\
 
 \ /
  Last update: 2020-08-26 18:38    [W:0.052 / U:0.836 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site