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

[Anton Vorontsov <cbouatmailru@gmail.com>]

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.

> ...
> static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> {
> ...
>         mutex_lock(&fsl_lbc_mutex);
>         if (!fsl_lbc_ctrl_dev->nand) {
>                 elbc_fcm_ctrl = kzalloc(sizeof(*elbc_fcm_ctrl), GFP_KERNEL);
>                 if (!elbc_fcm_ctrl) {
>                         dev_err(fsl_lbc_ctrl_dev->dev, "failed to allocate "
>                                         "memory\n");
>                         ret = -ENOMEM;
>                         goto err;
>                 }
> 
>                 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.

(Plus, you don't need these = 0 and = NULL as you used
 kzalloc() for allocation.)

> 
>                 spin_lock_init(&elbc_fcm_ctrl->controller.lock);
>                 init_waitqueue_head(&elbc_fcm_ctrl->controller.wq);

Some of these may need to be per chip select too.

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]
or something like that.

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.

>                 fsl_lbc_ctrl_dev->nand = elbc_fcm_ctrl;
>         } else
>                 elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;

Per coding style this should be

} else {
	elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
}

>         mutex_unlock(&fsl_lbc_mutex);
> 
>         elbc_fcm_ctrl->chips[bank] = priv;
> ...
> }

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2