| 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: <568A5C80.3030301@imgtec.com>
Date: Mon, 4 Jan 2016 11:50:24 +0000
From: Harvey Hunt <harvey.hunt@...tec.com>
To: Boris Brezillon <boris.brezillon@...e-electrons.com>
CC: <linux-mtd@...ts.infradead.org>, <computersforpeace@...il.com>,
<alex@...x-smith.me.uk>, Alex Smith <alex.smith@...tec.com>,
"Zubair Lutfullah Kakakhel" <Zubair.Kakakhel@...tec.com>,
David Woodhouse <dwmw2@...radead.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 2/3] mtd: nand: jz4780: driver for NAND devices on
JZ4780 SoCs
Hi Boris,
On 04/01/16 11:47, Boris Brezillon wrote:
> Hi Harvey,
>
> On Mon, 4 Jan 2016 10:24:15 +0000
> Harvey Hunt <harvey.hunt@...tec.com> wrote:
>
>
>>>> +
>>>> +static void jz4780_bch_disable(struct jz4780_bch *bch)
>>>> +{
>>>> + writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
>>>> + writel(BCH_BHCR_BCHE, bch->base + BCH_BHCCR);
>>>
>>> Not sure what BCH_BHCR_BCHE means, but if BCHE stands for "BCH Enable",
>>> do you really have to keep this bit set when disabling the engine?
>>
>> The JZ4780 has the BHCR (BCH Control Register) as well as the BHCCR
>> (BCH Control Clear Register) and BHCSR (BCH Control Set Register).
>> Setting the bit BCH_BHCR_BCHE in BHCCR clears the corresponding bit in
>> BHCR, which disables the BCH controller.
>>
>
> Okay, thanks for the explanation. I guess BCHE stands for BCH Engine
> then.
>
> [...]
>
>>>
>>>> +
>>>> +static int jz4780_nand_init_ecc(struct jz4780_nand_chip *nand, struct device *dev)
>>>> +{
>>>> + struct nand_chip *chip = &nand->chip;
>>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>>> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(chip->controller);
>>>> + struct nand_ecclayout *layout = &nand->ecclayout;
>>>> + u32 start, i;
>>>> +
>>>> + chip->ecc.bytes = fls((1 + 8) * chip->ecc.size) *
>>>> + (chip->ecc.strength / 8);
>>>> +
>>>> + if (nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
>>>> + chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
>>>> + chip->ecc.calculate = jz4780_nand_ecc_calculate;
>>>> + chip->ecc.correct = jz4780_nand_ecc_correct;
>>>> + } else if (!nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
>>>> + dev_err(dev, "HW BCH selected, but BCH controller not found\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + if (chip->ecc.mode != NAND_ECC_NONE)
>>>> + dev_info(dev, "using %s (strength %d, size %d, bytes %d)\n",
>>>> + (nfc->bch) ? "hardware BCH" : "software hamming",
>>>
>>> As said in my previous review, '!= NAND_ECC_HW' does not necessarily
>>> imply '== NAND_ECC_SOFT' (i.e. hamming ECC), so I'd suggest printing
>>> something like "software ECC".
>>
>> Done.
>>
>>>
>>>> + chip->ecc.strength, chip->ecc.size, chip->ecc.bytes);
>>>> + else
>>>> + dev_info(dev, "not using ECC\n");
>>>
>>> You should probably complain about the invalid NAND_ECC_HW_SYNDROME
>>> value and return -EINVAL in this case.
>>>
>
> Don't forget that aspect ^.
>
I forgot to mention it in my email, but I've not forgotten it in my
patchset. :-)
>>>> +
>>>> + /* The NAND core will generate the ECC layout. */
>>>> + if (chip->ecc.mode == NAND_ECC_SOFT || chip->ecc.mode == NAND_ECC_SOFT_BCH)
>>>> + return 0;
>>>> +
>>>> + /* Generate ECC layout. ECC codes are right aligned in the OOB area. */
>>>> + layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
>>>> +
>>>> + if (layout->eccbytes > mtd->oobsize - 2) {
>>>> + dev_err(dev,
>>>> + "invalid ECC config: required %d ECC bytes, but only %d are available",
>>>> + layout->eccbytes, mtd->oobsize - 2);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + start = mtd->oobsize - layout->eccbytes;
>>>> + for (i = 0; i < layout->eccbytes; i++)
>>>> + layout->eccpos[i] = start + i;
>>>> +
>>>> + layout->oobfree[0].offset = 2;
>>>> + layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;
>>>> +
>>>> + chip->ecc.layout = layout;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int jz4780_nand_init_chip(struct platform_device *pdev,
>>>> + struct jz4780_nand_controller *nfc,
>>>> + struct device_node *np,
>>>> + unsigned int chipnr)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct jz4780_nand_chip *nand;
>>>> + struct jz4780_nand_cs *cs;
>>>> + struct resource *res;
>>>> + struct nand_chip *chip;
>>>> + struct mtd_info *mtd;
>>>> + const __be32 *reg;
>>>> + int ret = 0;
>>>> +
>>>> + cs = &nfc->cs[chipnr];
>>>> +
>>>> + reg = of_get_property(np, "reg", NULL);
>>>> + if (!reg)
>>>> + return -EINVAL;
>>>> +
>>>> + cs->bank = be32_to_cpu(*reg);
>>>> +
>>>> + jz4780_nemc_set_type(nfc->dev, cs->bank, JZ4780_NEMC_BANK_NAND);
>>>> +
>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, chipnr);
>>>> + cs->base = devm_ioremap_resource(dev, res);
>>>> + if (IS_ERR(cs->base))
>>>> + return PTR_ERR(cs->base);
>>>> +
>>>> + nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL);
>>>> + if (!nand)
>>>> + return -ENOMEM;
>>>> +
>>>> + nand->busy_gpio = devm_gpiod_get_optional(dev, "rb", GPIOD_IN);
>>>> +
>>>> + if (IS_ERR(nand->busy_gpio)) {
>>>> + ret = PTR_ERR(nand->busy_gpio);
>>>> + dev_err(dev, "failed to request busy GPIO: %d\n", ret);
>>>> + return ret;
>>>> + } else if (nand->busy_gpio) {
>>>> + nand->chip.dev_ready = jz4780_nand_dev_ready;
>>>> + }
>>>> +
>>>> + nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW);
>>>> +
>>>> + if (IS_ERR(nand->wp_gpio)) {
>>>> + ret = PTR_ERR(nand->wp_gpio);
>>>> + dev_err(dev, "failed to request WP GPIO: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + chip = &nand->chip;
>>>> + mtd = nand_to_mtd(chip);
>>>> + mtd->priv = chip;
>>>> + mtd->owner = THIS_MODULE;
>>>> + mtd->name = DRV_NAME;
>>>> + mtd->dev.parent = dev;
>>>> +
>>>> + chip->IO_ADDR_R = cs->base + OFFSET_DATA;
>>>> + chip->IO_ADDR_W = cs->base + OFFSET_DATA;
>>>> + chip->chip_delay = RB_DELAY_US;
>>>> + chip->options = NAND_NO_SUBPAGE_WRITE;
>>>> + chip->select_chip = jz4780_nand_select_chip;
>>>> + chip->cmd_ctrl = jz4780_nand_cmd_ctrl;
>>>> + chip->ecc.mode = NAND_ECC_HW;
>>>> + chip->controller = &nfc->controller;
>>>> + nand_set_flash_node(chip, np);
>>>> +
>>>> + ret = nand_scan_ident(mtd, 1, NULL);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = jz4780_nand_init_ecc(nand, dev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = nand_scan_tail(mtd);
>>>> + if (ret)
>>>> + goto err_release_bch;
>>>> +
>>>> + ret = mtd_device_register(mtd, NULL, 0);
>>>> + if (ret)
>>>> + goto err_release_nand;
>>>> +
>>>
>>> You probably miss a list_add_tail() call here (otherwise chips are
>>> registered but not unregistered when ->remove() is called):
>>>
>>> list_add_tail(&nand->chip_list, &nfc->chips);
>>
>> Thanks for spotting this - I've fixed it now,
>>
>>>
>>>> + return 0;
>>>> +
>>>> +err_release_nand:
>>>> + nand_release(mtd);
>>>> +
>>>> +err_release_bch:
>>>> + if (nfc->bch)
>>>> + jz4780_bch_release(nfc->bch);
>>>
>>> Why are you releasing the BCH engine here, isn't it the role of the
>>> NAND controller to do that? BTW, it's already done in
>>> jz4780_nand_remove().
>>
>> I don't think jz4780_nand_remove() will get called if we fail to
>> initialise the chips, so perhaps it would be better to move this into
>> jz4780_nand_probe?
>
> No, jz4780_nand_remove() won't get called in case of failure, I was
> just trying to point the asymmetry here: if it's released in
> ->remove() and assigned in ->probe(), then it should be released in
> ->probe() in case of failure. So yes, moving it in jz4780_nand_probe()
> is the right thing to do.
I'll do that now.
>
> Best Regards,
>
> Boris
>
Thanks,
Harvey
--
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