linux-kernel - RE: [PATCH v3 03/13] pm: at91: Workaround DDRSDRC self-refresh bug with LPDDR1 memories.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6740d1ada0cb4c8bbf765728c7443918@EMAIL.axentia.se>
Date:	Tue, 27 Jan 2015 21:55:52 +0000
From:	Peter Rosin <peda@...ntia.se>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	Wenyou Yang <wenyou.yang@...el.com>,
	"nicolas.ferre@...el.com" <nicolas.ferre@...el.com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>
CC:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"alexandre.belloni@...e-electrons.com" 
	<alexandre.belloni@...e-electrons.com>,
	"sylvain.rochet@...secur.com" <sylvain.rochet@...secur.com>,
	"linux@...im.org.za" <linux@...im.org.za>
Subject: RE: [PATCH v3 03/13] pm: at91: Workaround DDRSDRC self-refresh bug
 with LPDDR1 memories.

I wrote:
> Sergei Shtylyov wrote:
> > On 1/27/2015 8:53 AM, Wenyou Yang wrote:
> >
> > > From: Peter Rosin <peda@...ntia.se>
> >
> > > The DDRSDR controller fails miserably to put LPDDR1 memories in
> > > self-refresh. Force the controller to think it has DDR2 memories
> > > during the self-refresh period, as the DDR2 self-refresh spec is
> > > equivalent to LPDDR1, and is correctly implemented in the controller.
> >
> > > Assume that the second controller has the same fault, but that is
> > > untested.
> >
> > > Signed-off-by: Peter Rosin <peda@...ntia.se>
> > > Acked-by: Nicolas Ferre <nicolas.ferre@...el.com>
> > > ---
> > >   arch/arm/mach-at91/pm_slowclock.S  |   43
> > +++++++++++++++++++++++++++++++-----
> > >   include/soc/at91/at91sam9_ddrsdr.h |    2 +-
> > >   2 files changed, 39 insertions(+), 6 deletions(-)
> >
> > > diff --git a/arch/arm/mach-at91/pm_slowclock.S
> > > b/arch/arm/mach-at91/pm_slowclock.S
> > > index e2bfaf5..1155217 100644
> > > --- a/arch/arm/mach-at91/pm_slowclock.S
> > > +++ b/arch/arm/mach-at91/pm_slowclock.S
> > [...]
> > > @@ -108,14 +118,26 @@ ddr_sr_enable:
> > >
> > >   	/* figure out if we use the second ram controller */
> > >   	cmp	ramc1, #0
> > > -	ldrne	tmp2, [ramc1, #AT91_DDRSDRC_LPR]
> > > -	strne	tmp2, .saved_sam9_lpr1
> > > -	bicne	tmp2, #AT91_DDRSDRC_LPCB
> > > -	orrne	tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH
> > > +	beq	ddr_no_2nd_ctrl
> > > +
> > > +	ldr	tmp2, [ramc1, #AT91_DDRSDRC_MDR]
> > > +	str	tmp2, .saved_sam9_mdr1
> > > +	bic	tmp2, tmp2, #~AT91_DDRSDRC_MD
> > > +	cmp	tmp2, #AT91_DDRSDRC_MD_LOW_POWER_DDR
> > > +	ldreq	tmp2, [ramc1, #AT91_DDRSDRC_MDR]
> > > +	biceq	tmp2, tmp2, #AT91_DDRSDRC_MD
> >
> >     Didn't you forget ~? Either that, or ~ above is not needed, I think.
> 
> The code is correct, the first bic with ~ clears bits not in the relevant
> field in order to compare if LPDDR mode is active. The second bic(eq)
> w/o ~ clears the field, to make way for the bits in the below orreq
> when actually changing the register content into DDR2 mode.
> 
> > > +	orreq	tmp2, tmp2, #AT91_DDRSDRC_MD_DDR2
> > > +	streq	tmp2, [ramc1, #AT91_DDRSDRC_MDR]
> > > +
> > > +	ldr	tmp2, [ramc1, #AT91_DDRSDRC_LPR]
> > > +	str	tmp2, .saved_sam9_lpr1
> > > +	bic	tmp2, #AT91_DDRSDRC_LPCB
> >
> >     Didn't you forget ~? And isn't it 3-operand instruction (as seen in the above
> > code)?
> 
> The logic for the LPR register is from the old code, the "only" thing
> I did to it was changing the instruction sequence to not have the
> ???ne form, i.e. ldrne, strne, bicne, orrne became ldr, str, bic, orr
> with a jump around it instead. So, the original code also had a two
> argument bic(ne), which indeed is strange, and I don't know why
> there is no warning from the assembler. Since there is no warning,
> my guess is that the assembler somehow mends it? Or does the
> patch actually break the second controller? It would be a surprise
> it the assembler handles 2-operand bicne differently from a

s/it the/if the/

> 2-operand bic, no?
> 
> > > +	orr	tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH
> >
> >     Only 2 operands?
> 
> Same argument as above. I didn't touch it (sort of...)
> 
> Should I update the patch and fix this collateral 2-operand problem as
> well? To me, it feels like a separate patch, no?

I have now checked the assembler output, and apparently it mends the
input, just as I thought. That might be a fluke, of course, or it might be a
deliberate shorthand when the destination register is the same as the
following operand? But I also note that there are more instances of this
2 vs. 3 argument syntax, and I suggest that they are all fixed in one go,
if it is determined that they need fixing. I am obviously not an authority
when it comes to arm assembler syntax, so someone else will have to
advise...

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/