Messages in this thread | | | Date | Sat, 07 Nov 2020 16:54:32 +0000 | From | Marc Zyngier <> | Subject | Re: [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...
| |