linux-kernel - Re: [PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC engine

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: <1412256036.20895.10.camel@lp-lvrv.comap.cz>
Date:	Thu, 02 Oct 2014 15:20:36 +0200
From:	Rostislav Lisovy <lisovy@...il.com>
To:	Roger Quadros <rogerq@...com>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	Brian Norris <computersforpeace@...il.com>,
	Tony Lindgren <tony@...mide.com>,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	michal.vokac@...ap.cz, sojkam1@....cvut.cz,
	Rostislav Lisovy <lisovy@...ica.cz>
Subject: Re: [PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC
 engine

Hello Roger;
Thanks for the review.

On Čt, 2014-10-02 at 15:51 +0300, Roger Quadros wrote:
> Don't you think this approach is racy?
> 
> IMHO the lock must be held across the entire page operation

I still think it is done in this way.

> i.e.
> hold ecc lock
> ecc.hwctl
> chip->read/write_buf
> ecc.calculate
> ecc.correct
> release ecc lock

According to my understanding of the code 'ecc.correct' does not access
the ECC engine directly, it gets the '*read_ecc' and '*calc_ecc' from
the 'ecc.calculate'.

Once again the work flow you described + my locking approach:
* ecc.hwctl       <-- mutex_lock() just before accessing the ECC engine
* chip->read/write_buf
* ecc.calculate   <-- mutex_unlock() just after reading from ECC engine
* ecc.correct

> 
> else we risk simultaneous NAND operations on multiple chips
> stomping on each other in between the entire sequence.
> 
> Then on further investigation isn't nand_get_device() already doing
> the same
> thing as you are attempting here?
> 
> The chip->controller->lock is meant for serializing NAND controller
> access.
> 
> so instead of adding a new lock in the omap2 nand driver we need to
> ensure that
> we are maintaining the same nand_hw_control (controller) structure
> across multiple NAND chips.
> 
> Let's move this controller structure out of omap_nand_info and keep it
> global to the driver
> and make sure every NAND instance uses it.

Ok; I will take a look at it. It looks like the appropriate lock to use.
I am just a bit unsure if the 'spinlock' is the correct synchronization
primitive here.

Best regards;
Rostislav

--
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/