Re: [PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions

[Scott Wood <scottwood@freescale.com>]

On Thu, 16 Sep 2010 15:26:24 +0400
Anton Vorontsov <cbouatmailru@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.
> fsl_lbc_ctrl_dev->nand_ctrl[MAX_CHIP_SELECTS]

NACK.

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.

-Scott