Messages in this thread | | | 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. | From | "xuqiang (M)" <> | Date | Mon, 9 Nov 2020 11:05:37 +0800 |
| |
在 2020/11/8 0:54, Marc Zyngier 写道: > [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? Hisi Ascend 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?
I am using ATF. Since ITS_FLAGS_SAVE_SUSPEND_STATE is not set,ITS driver of OS will
not re-enable ITS in th resume. To make ITS work properly, we changed the ATF code
to re-enable ITS on resume.
> >> >> 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. ITS driver kfree ITT_addr1. > >> 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.
Thanks
Xu.
| |