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

[Rostislav Lisovy]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/