linux-kernel - Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code

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: <20170302133013.4dee0b4a@bbrezillon>
Date:   Thu, 2 Mar 2017 13:30:13 +0100
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Alexander Dahl <ada@...rsis.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Nicolas Ferre <nicolas.ferre@...el.com>,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        Lee Jones <lee.jones@...aro.org>,
        Samuel Ortiz <sameo@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code

Hi Alexander,

On Thu, 02 Mar 2017 13:02:16 +0100
Alexander Dahl <ada@...rsis.com> wrote:

> Hei hei,
> 
> With 
> 
> #define ATMEL_SMC_MODE_TDF(x)           (((x) - 1) << 16)
> 
> from include/linux/mfd/syscon/atmel-smc.h you added this:
> 
> > +	ret = of_property_read_u32(np, "atmel,smc-tdf-ns", &val);
> > +	if (!ret) {
> > +		required = true;
> > +		ncycles = DIV_ROUND_UP(val, clk_period_ns);
> > +		if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}  
> 
> […]
> 
> > +		smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
> > +	}  
> 
> This was the same algorithm at some other location in atmel-ebi.c 
> before:
> 
> 	#define AT91_SMC_TDF_(x)		((((x) - 1) << 16) & AT91_SMC_TDF)
> 
> 	val = DIV_ROUND_UP(timings->tdf_ns, clk_rate);
> 	if (val > AT91_SMC_TDF_MAX)
> 		val = AT91_SMC_TDF_MAX;
> 	regmap_fields_write(fields->mode, conf->cs,
> 			    config->mode | AT91_SMC_TDF_(val));
> 
> The hardware manual (AT91SAM9G20) says values from 0 to 15 (4bit, 0x0 to 
> 0xF) are possible and I guess the goal is to set it to a value 
> corresponding to the value in ns from the dts or to 15 if it's greater 
> (or -EINVAL in the new version).
> 
> However how can one set it to zero? Put in zero to the div you get zero 
> for ncycles or val and that goes as x into (((x) - 1) << 16) which 
> results in 0xF ending up as TDF_CYCLES in the mode register, right?

Indeed.

> 
> I can of course set a slightly greater value, which ends up in a 
> calculated register value of zero, but that seems more a hack to me and 
> is not obvious if I just look at the DTS.

No, we should fix the bug.

> 
> If I'm right this might be topic of another bugfix patch, or should it 
> be done right in a v2 of this one?

It should be done right in a v2. Something like:

		if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
			ncycles = ATMEL_SMC_MODE_TDF_MIN;

with

#define ATMEL_SMC_MODE_TDF_MIN 1

I don't think we need to backport the fix, since no-one uses this driver
yet.

Thanks for this report.

Boris