linux-kernel - Re: [LINUX PATCH v12 3/3] mtd: rawnand: arasan: Add support for Arasan NAND Flash Controller

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAB7pii1WbALtH1AHjocpC976ytrCV5gM6JGXawoN--scvEx8Mw@mail.gmail.com>
Date:   Mon, 12 Nov 2018 11:55:36 +0100
From:   Martin Lund <martin.lund@...p-it-simple.com>
To:     naga.sureshkumar.relli@...inx.com
Cc:     boris.brezillon@...tlin.com, miquel.raynal@...tlin.com,
        richard@....at, dwmw2@...radead.org, computersforpeace@...il.com,
        marek.vasut@...il.com, michals@...inx.com, nagasuresh12@...il.com,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        robh@...nel.org
Subject: Re: [LINUX PATCH v12 3/3] mtd: rawnand: arasan: Add support for
 Arasan NAND Flash Controller

Hi Naga,

Just a few review comments for the v12 version.

On Fri, Nov 9, 2018 at 6:00 AM Naga Sureshkumar Relli
<naga.sureshkumar.relli@...inx.com> wrote:
> +#define PKT_OFST               0x00
> +#define PKT_CNT_SHIFT          12
> +
> +#define MEM_ADDR1_OFST         0x04
> +#define MEM_ADDR2_OFST         0x08

For the sake of readability I think *_OFFSET is preferred, especially
since the driver already includes short macro names. I think this is
similar to the EVNT vs EVENT point.
The majority of drivers in the Linux kernel do not shorten OFFSET -> OFST.


> +static void anfc_rw_pio_op(struct mtd_info *mtd, u8 *buf, int len,
> +                          bool do_read, int prog, int pktcount, int pktsize)
> +{
> +       struct nand_chip *chip = mtd_to_nand(mtd);
> +       struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> +       struct anfc_nand_chip *achip = to_anfc_nand(chip);
> +       u32 *bufptr = (u32 *)buf;
> +       u32 cnt = 0, intr = 0;
> +
> +       anfc_config_dma(nfc, 0);
> +
> +       if (pktsize == 0)
> +               pktsize = len;
> +
> +       anfc_setpktszcnt(nfc, pktsize, pktcount);
> +
> +       if (!achip->strength)
> +               intr = MBIT_ERROR;
> +
> +       if (do_read)
> +               intr |= READ_READY;
> +       else
> +               intr |= WRITE_READY;
> +       anfc_enable_intrs(nfc, intr);
> +       writel(prog, nfc->base + PROG_OFST);
> +       while (cnt < pktcount) {
> +               anfc_wait_for_event(nfc);
> +               cnt++;
> +               if (cnt == pktcount)
> +                       anfc_enable_intrs(nfc, XFER_COMPLETE);
> +               if (do_read)
> +                       ioread32_rep(nfc->base + DATA_PORT_OFST, bufptr,
> +                                    pktsize / 4);
> +               else
> +                       iowrite32_rep(nfc->base + DATA_PORT_OFST, bufptr,
> +                                     pktsize / 4);
> +               bufptr += (pktsize / 4);
> +               if (cnt < pktcount)
> +                       anfc_enable_intrs(nfc, intr);
> +       }
> +       anfc_wait_for_event(nfc);
> +}

Throughout the driver all calls to anfc_wait_for_event() ignores the
timeout return value. It would be nice to see some error handling in
case it times out - at minimum consider printing out an error message
since timeout on NAND operations are fairly critical and should
generally not occur. Perhaps even an argument can be made for
returning -ETIMEDOUT in case of timeout.

I'm putting a focus on this because I see the original non-upstream
Arasan driver sometimes timeout on NAND operations when I stress test
it via the UBI stress test. Not sure what the cause for the timeout is
yet but either way an error message would have been helpful.

Br, Martin