linux-kernel - Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Jan 2017 18:22:28 +0100
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     Jean-Jacques Hiblot <jjhiblot@...il.com>
Cc:     Wenyou Yang <wenyou.yang@...el.com>,
        Mark Rutland <mark.rutland@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Russell King <linux@....linux.org.uk>,
        Wenyou Yang <wenyou.yang@...rochip.com>,
        Nicolas Ferre <nicolas.ferre@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu
 idle

On 10/01/2017 at 17:50:58 +0100, Jean-Jacques Hiblot wrote :
> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> <alexandre.belloni@...e-electrons.com>:
> > I though a bit more about it, and I don't really like the new compatible
> > string. I don't feel this should be necessary.
> >
> > What about the following:
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> > index b4332b727e9c..0333aca63e44 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
> >  static struct {
> >         unsigned long uhp_udp_mask;
> >         int memctrl;
> > +       bool has_l2_cache;
> >  } at91_pm_data;
> >
> >  void __iomem *at91_ramc_base[2];
> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
> >         u32 lpr0, lpr1 = 0;
> >         u32 saved_lpr0, saved_lpr1 = 0;
> >
> 
> > +       if (at91_pm_data.has_l2_cache) {
> > +               flush_cache_all();
> what is the point of calling flush_cache_all() here ? Do we really
> care that dirty data in L1 is written to DDR ? I may be missing
> something but to me it's just extra latency.

I agree that this one is the main problem.

> > +               outer_disable();
> It seems to me that if there's no L2 cache, then outer_disable()  is a
> no-op. It could be called unconditionally.

It is not on sama5, it will jump to outer_disable which will at least
save the context and restore it

> > +       }
> > +
> >         if (at91_ramc_base[1]) {
> >                 saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
> >                 lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
> > @@ -287,6 +293,9 @@ static void at91_ddr_standby(void)
> >         at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> >         if (at91_ramc_base[1])
> >                 at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> > +
> > +       if (at91_pm_data.has_l2_cache)
> > +               outer_resume();
> 
> same remark as for outer_disable()

It is not either but this is a macro and I admit testing has_l2_cache is
superfluous.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com