[PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions
scottwood at freescale.com
Fri Sep 17 02:14:48 EST 2010
On Thu, 16 Sep 2010 15:26:24 +0400
Anton Vorontsov <cbouatmailru at gmail.com> wrote:
> On Thu, Sep 16, 2010 at 06:39:40PM +0800, Zang Roy-R61911 wrote:
> > But my code has some assignment for "foo" instead of a simple
> > allocation, how about this way for my code:
> This will surely work, and all the rest is just a matter of
> taste. So, I'm fine with it. But see below, I think I found
> some new, quite serious issues.
> > DEFINE_MUTEX(fsl_elbc_mutex);
> I'd place the mutex inside the fsl_lbc_ctrl_dev,
> i.e. fsl_lbc_ctrl_dev->nand_lock. This is to avoid more
> global variables.
I wouldn't. If the lock is only meaningful to the NAND driver, it
should be declared in the NAND driver.
Besides, it's not any less of a global just because it's sitting inside
a singleton struct.
Perhaps it should be declared as a static local inside the probe
function, if it's just to guard against this one race.
> > elbc_fcm_ctrl->read_bytes = 0;
> > elbc_fcm_ctrl->index = 0;
> > elbc_fcm_ctrl->addr = NULL;
> I guess these variables should be per chip select, as
> otherwise there will be tons of races when somebody try
> to access two or more NAND chips simultaneously.
The NAND layer has its own per-controller mutex that prevents this.
> So, I'd suggest to redo the whole thing this way: don't allocate
> elbc_fcm_ctrl in this driver, but make an array inside the
> fsl_lbc_ctrl_dev. I.e.
There is not a separate controller per chip select.
> Btw, even before this patch, it seems that the driver had
> all these bugs/races, i.e. ctrl->controller.lock was not
> used at all. Ugh.
It is used, search nand_base.c for controller->lock.
More information about the Linuxppc-dev