lkml.org 
[lkml]   [2020]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -next] irq-chip/gic-v3-its: Fixed an issue where the ITS executes the residual commands in the queue again when the ITS wakes up from sleep mode.
[dropping Jason, whose email address has been bouncing for weeks now]

On 2020-11-07 10:42, Xu Qiang wrote:
> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do nothing

Which platform?

> in its suspend and resuse function.On the other hand,firmware stores
> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
> and restores these registers in the resume. As a result, the ITS
> executes
> the residual commands in the queue.

Which firmware are you using? I just had a look at the trusted firmware
source
code, and while it definitely does something that *looks* like what you
are
describing, it doesn't re-enable the ITS on resume.

So what are you running?

>
> Memory corruption may occur in the following scenarios:
>
> The kernel sends three commands in the following sequence:
> 1.mapd(deviceA, ITT_addr1, valid:1)
> 2.mapti(deviceA):ITS write ITT_addr1 memory;
> 3.mapd(deviceA, ITT_addr1, valid:0) and kfree(ITT_addr1);

The ITS doesn't 'kfree' stuff.

> 4.mapd(deviceA, ITT_addr2, valid:1);
> 5.mapti(deviceA):ITS write ITT_addr2 memory;

I don't think this example is relevant. The core of the problem is that
the ITS gets re-enabled by your firmware. What are the affected systems?

>
> To solve this problem,dropping the checks for
> ITS_FLAGS_SAVE_SUSPEND_STATE.
>
> Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index 0fec31931e11..06f2c1c252b9 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -42,7 +42,6 @@
> #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0)
> #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1)
> #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2)
> -#define ITS_FLAGS_SAVE_SUSPEND_STATE (1ULL << 3)
>
> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0)
> #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1)
> @@ -4741,9 +4740,6 @@ static int its_save_disable(void)
> list_for_each_entry(its, &its_nodes, entry) {
> void __iomem *base;
>
> - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> - continue;
> -
> base = its->base;
> its->ctlr_save = readl_relaxed(base + GITS_CTLR);
> err = its_force_quiescent(base);
> @@ -4762,9 +4758,6 @@ static int its_save_disable(void)
> list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
> void __iomem *base;
>
> - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> - continue;
> -
> base = its->base;
> writel_relaxed(its->ctlr_save, base + GITS_CTLR);
> }
> @@ -4784,9 +4777,6 @@ static void its_restore_enable(void)
> void __iomem *base;
> int i;
>
> - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> - continue;
> -
> base = its->base;
>
> /*
> @@ -5074,9 +5064,6 @@ static int __init its_probe_one(struct resource
> *res,
> ctlr |= GITS_CTLR_ImDe;
> writel_relaxed(ctlr, its->base + GITS_CTLR);
>
> - if (GITS_TYPER_HCC(typer))
> - its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
> -
> err = its_init_domain(handle, its);
> if (err)
> goto out_free_tables;

I'm OK with the patch itself, but I don't want to paper over broken
firmware.
I'll get TF-A fixed one way or another, but I want to be sure yours is
too.
If firmware does its job correctly, we shouldn't have to do all of this.

M.
--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-11-07 17:54    [W:0.079 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site