Messages in this thread | | | Subject | Re: [PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC engine | From | Rostislav Lisovy <> | Date | Thu, 02 Oct 2014 15:20:36 +0200 |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |