| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4ECCE393.2030600@freescale.com>
Date: Wed, 23 Nov 2011 20:14:11 +0800
From: LiuShuo <b35362@...escale.com>
To: Scott Wood <scottwood@...escale.com>
CC: <dwmw2@...radead.org>, <Artem.Bityutskiy@...ia.com>,
<linux-mtd@...ts.infradead.org>, <linuxppc-dev@...ts.ozlabs.org>,
<akpm@...ux-foundation.org>, <linux-kernel@...r.kernel.org>,
<leoli@...escale.com>, Liu Shuo <Shuo.Liu@...escale.com>,
Shengzhou Liu <Shengzhou.Liu@...escale.com>
Subject: Re: [PATCH v3 3/3] mtd/nand : workaround for Freescale FCM to support
large-page Nand chip
于 2011年11月23日 07:55, Scott Wood 写道:
> On 11/15/2011 03:29 AM, b35362@...escale.com wrote:
>> From: Liu Shuo<b35362@...escale.com>
>>
>> Freescale FCM controller has a 2K size limitation of buffer RAM. In order
>> to support the Nand flash chip whose page size is larger than 2K bytes,
>> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save
>> them to a large buffer.
>>
>> Signed-off-by: Liu Shuo<Shuo.Liu@...escale.com>
>> Signed-off-by: Shengzhou Liu<Shengzhou.Liu@...escale.com>
>> Signed-off-by: Li Yang<leoli@...escale.com>
>> ---
>> drivers/mtd/nand/fsl_elbc_nand.c | 216 +++++++++++++++++++++++++++++++++++---
>> 1 files changed, 199 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
>> index c2c231b..415f87e 100644
>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>> @@ -55,7 +55,9 @@ struct fsl_elbc_mtd {
>> struct device *dev;
>> int bank; /* Chip select bank number */
>> u8 __iomem *vbase; /* Chip select base virtual address */
>> - int page_size; /* NAND page size (0=512, 1=2048) */
>> + int page_size; /* NAND page size (0=512, 1=2048, 2=4096...),
>> + * the mutiple of 2048.
>> + */
> That "..." isn't very descriptive. What happens with 8192-byte pages?
> Is it 3 or 4?
>
> Please just get rid of this and use mtd->writesize.
>
>> + for (i = 1; i< priv->page_size; i++) {
>> + /*
>> + * Maybe there are some reasons of FCM hardware timming,
>> + * we must insert a FIR_OP_NOP(0x00) before FIR_OP_RB.
>> + */
> s/timming/timing/
>
>> /* PAGEPROG reuses all of the setup from SEQIN and adds the length */
>> case NAND_CMD_PAGEPROG: {
>> + int len;
>> dev_vdbg(priv->dev,
>> "fsl_elbc_cmdfunc: NAND_CMD_PAGEPROG "
>> "writing %d bytes.\n", elbc_fcm_ctrl->index);
>> -
>> /* if the write did not start at 0 or is not a full page
>> * then set the exact length, otherwise use a full page
>> * write so the HW generates the ECC.
>> */
>> - if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 ||
>> + if (elbc_fcm_ctrl->column>= mtd->writesize) {
>> + /* write oob */
>> + if (priv->page_size> 1) {
>> + /* when pagesize of chip is greater than 2048,
>> + * we have to write full page to write spare
>> + * region, so we fill '0xff' to main region
>> + * and some bytes of spare region which we
>> + * don't want to rewrite.
>> + * (write '1' won't change the original value)
>> + */
>> + memset(elbc_fcm_ctrl->buffer, 0xff,
>> + elbc_fcm_ctrl->column);
> I don't like relying on this -- can we use RNDIN instead to do a
> discontiguous write?
>
I have no better way to implement it now.
Some chips have 'NOP' limitation, so I don't use the FIR_OP_UA to do a
oob write.
>> + len = 2112;
> len = min(elbc_fcm_ctrl->index, 2112);
when do a oob write (writesize > 2048), elbc_fcm_ctrl->index is greater
writesize
(is 4096 at least).
>> + } else
>> + len = mtd->writesize + mtd->oobsize -
>> + elbc_fcm_ctrl->column;
>> + out_be32(&lbc->fbcr, len);
> len = elbc_fcm_ctrl->index - elbc_fcm_ctrl->column;
>
> Use braces on both sides of the if/else if it's needed on one side.
>> + } else if (elbc_fcm_ctrl->column != 0 ||
>> elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize)
>> out_be32(&lbc->fbcr, elbc_fcm_ctrl->index);
> This should have set fbcr to index - column as well (after adjusting --
> though really it's not a supported use case. We should only be seeing
> column != 0 for oob.
>
I have make a independent to fix this issue.
(In fact,documentation says FCM will stop automatically after
reading the last byte of spare region)
>> @@ -625,10 +776,16 @@ static int fsl_elbc_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
>> return -EINVAL;
>> }
>>
>> - for (i = 0; i< len; i++)
>> - if (in_8(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index + i])
>> - != buf[i])
>> - break;
>> + if (mtd->writesize> 2048)
>> + for (i = 0; i< len; i++)
>> + if (elbc_fcm_ctrl->buffer[elbc_fcm_ctrl->index + i]
>> + != buf[i])
>> + break;
>> + else
>> + for (i = 0; i< len; i++)
>> + if (in_8(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index + i])
>> + != buf[i])
>> + break;
> Please use braces around multiline if/for bodies, even if they're
> technically a single statement -- especially when you've got a dangling
> else.
>
>> elbc_fcm_ctrl->index += len;
>> return i == len&& elbc_fcm_ctrl->status == LTESR_CC ? 0 : -EIO;
>> @@ -657,6 +814,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
>> struct fsl_elbc_mtd *priv = chip->priv;
>> struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>> struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
>> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
>> unsigned int al;
>>
>> /* calculate FMR Address Length field */
>> @@ -707,12 +865,17 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
>> dev_dbg(priv->dev, "fsl_elbc_init: mtd->oobsize = %d\n",
>> mtd->oobsize);
>>
>> + kfree(elbc_fcm_ctrl->buffer);
>> + elbc_fcm_ctrl->buffer = NULL;
>> +
>> /* adjust Option Register and ECC to match Flash page size */
>> if (mtd->writesize == 512) {
>> priv->page_size = 0;
>> clrbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
>> - } else if (mtd->writesize == 2048) {
>> - priv->page_size = 1;
>> + } else if (mtd->writesize>= 2048) {
>> + /* page_size = writesize / 2048 */
>> + priv->page_size = mtd->writesize>> 11;
> Why not just write priv->page_size = mtd->writesize / 2048 in the code
> rather than the comment (it compiles to the same code)? Other than
> because you were requested to remove priv->page_size, that is. :-)
>
>> setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
>> /* adjust ecc setup if needed */
>> if ((in_be32(&lbc->bank[priv->bank].br)& BR_DECC) ==
>> @@ -723,6 +886,14 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
>> &fsl_elbc_oob_lp_eccm0;
>> chip->badblock_pattern =&largepage_memorybased;
>> }
>> +
>> + /* re-malloc if pagesize> 2048*/
>> + if (mtd->writesize> 2048) {
>> + elbc_fcm_ctrl->buffer = kmalloc(mtd->writesize +
>> + mtd->oobsize, GFP_KERNEL);
>> + if (!elbc_fcm_ctrl->buffer)
>> + return -ENOMEM;
>> + }
>> } else {
>> dev_err(priv->dev,
>> "fsl_elbc_init: page size %d is not supported\n",
> buffer is a controller-wide resource, but you're setting it based on the
> most-recently-probed NAND chip. It should be large enough to
> accommodate the largest page size in use on any connected chip. Even if
> all chips in the system have the same page size, you're introducing a
> race if a previously-probed chip is already in use (the controller lock
> is not held here).
>
> Just allocate a buffer large enough for a 16K page size in the main init
> function, and print an error in the tail if you encounter a larger page
> size.
>
>> @@ -886,6 +1057,17 @@ static int __devinit fsl_elbc_nand_probe(struct platform_device *pdev)
>> goto err;
>> }
>> elbc_fcm_ctrl->counter++;
>> + /*
>> + * Freescale FCM controller has a 2K size limitation of buffer
>> + * RAM, so elbc_fcm_ctrl->buffer have to be used if writesize
>> + * of chip is greater than 2048.
>> + * We malloc a large enough buffer at this point, because we
>> + * don't know writesize before calling nand_scan(). We will
>> + * re-malloc later if needed.
>> + */
>> + elbc_fcm_ctrl->buffer = kmalloc(4096 * 6, GFP_KERNEL);
>> + if (!elbc_fcm_ctrl->buffer)
>> + return -ENOMEM;
> Clean up properly if you fail to allocate the buffer. This includes
> freeing elbc_fcm_ctrl, setting fsl_lbc_ctrl_dev->nand to NULL, and
> releasing fsl_elbc_nand_mutex.
> -Scott
- LiuShuo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists