On Thu, 16 Sep 2010 15:26:24 +0400 Anton Vorontsov <cbouatmai...@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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
