| 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: <20170330181623.5b595827@bbrezillon>
Date: Thu, 30 Mar 2017 18:16:23 +0200
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc: linux-mtd@...ts.infradead.org, Enrico Jorns <ejo@...gutronix.de>,
Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
Dinh Nguyen <dinguyen@...nel.org>,
Marek Vasut <marek.vasut@...il.com>,
Graham Moore <grmoore@...nsource.altera.com>,
David Woodhouse <dwmw2@...radead.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Chuanxiao Dong <chuanxiao.dong@...el.com>,
Jassi Brar <jaswinder.singh@...aro.org>,
linux-kernel@...r.kernel.org,
Brian Norris <computersforpeace@...il.com>,
Richard Weinberger <richard@....at>,
Cyrille Pitchen <cyrille.pitchen@...el.com>
Subject: Re: [PATCH v3 26/37] mtd: nand: denali: fix bank reset function
On Thu, 30 Mar 2017 15:46:12 +0900
Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
> The function denali_nand_reset() is called during the driver probe,
> and polls the INTR__RST_COMP and INTR__TIME_OUT bits. However,
> INTR__RST_COMP is set anyway even if no NAND device is connected to
> that bank.
>
> This can be a problem for ONFi devices. The nand_scan_ident()
> iterates over maxchips, and calls nand_reset() for each chip.
Actually, maxchips is a bad name. What should be passed in argument to
nand_scan_ident() is not the maximum number of CS-line the controller
has, it's the expected number of CS-lines provided by a chip.
If you're using DT, this information should be retrieved from the DT. If
you look at this binding doc [1] you'll see that each NAND chip has a
reg property encoding the CS line. When a chip exposes more than one
CS-line, the reg property should contain 2 entries describing which
controller-side CS lines are connected to the chip CS-lines.
For non-DT cases, this should be exposed by some other means (for
example pdata, but I'm not sure it works well with PCI where everything
is discoverable).
So normally, you shouldn't have a timeout, or something is wrong with
the DT/board description.
Note that you might have different NAND models connected to the same
NAND controller. If you call nand_scan_ident() only once and pass
controllers->max_cs_lines to it, you will only have one chip detected,
which is not what you expect.
> Now, this driver implements ->setup_data_interface() method, so
> nand_setup_data_interface() issues Set Features (0xEF) command to
> each chip. This can cause time-out error since denali_nand_reset()
> did not check the chip existence. If no chip there, the controller
> will wait long for R/B# response, which we know never happens.
> (The timeout error is correctly handled in this driver, so the
> driver will be successfully probed anyway, but it will take longer
> than needed.)
>
> The Reset (0xFF) command also toggles the R/B# pin, and it sets
> INTR__INT_ACT bit. The driver should check this bit to see if the
> chip has responded, then it can update denali->max_banks.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> ---
>
> Changes in v3: None
> Changes in v2:
> - Newly added
>
> drivers/mtd/nand/denali.c | 52 +++++++++++++++++++++--------------------------
> 1 file changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 86b7c75..e4f2699 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -85,33 +85,6 @@ static void index_addr(struct denali_nand_info *denali,
> iowrite32(data, denali->flash_mem + 0x10);
> }
>
> -/* Reset the flash controller */
> -static uint16_t denali_nand_reset(struct denali_nand_info *denali)
> -{
> - int i;
> -
> - for (i = 0; i < denali->max_banks; i++)
> - iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
> - denali->flash_reg + INTR_STATUS(i));
> -
> - for (i = 0; i < denali->max_banks; i++) {
> - iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
> - while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
> - (INTR__RST_COMP | INTR__TIME_OUT)))
> - cpu_relax();
> - if (ioread32(denali->flash_reg + INTR_STATUS(i)) &
> - INTR__TIME_OUT)
> - dev_dbg(denali->dev,
> - "NAND Reset operation timed out on bank %d\n", i);
> - }
> -
> - for (i = 0; i < denali->max_banks; i++)
> - iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
> - denali->flash_reg + INTR_STATUS(i));
> -
> - return PASS;
> -}
> -
> /*
> * Use the configuration feature register to determine the maximum number of
> * banks that the hardware supports.
> @@ -999,7 +972,28 @@ static int denali_setup_data_interface(struct mtd_info *mtd,
> return 0;
> }
>
> -/* Initialization code to bring the device up to a known good state */
> +static void denali_reset_banks(struct denali_nand_info *denali)
> +{
> + int i;
> +
> + denali_clear_irq_all(denali);
> +
> + for (i = 0; i < denali->max_banks; i++) {
> + iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
> + while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
> + (INTR__RST_COMP | INTR__TIME_OUT)))
> + cpu_relax();
> + if (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
> + INTR__INT_ACT))
> + break;
> + }
> +
> + dev_dbg(denali->dev, "%d chips connected\n", i);
> + denali->max_banks = i;
> +
> + denali_clear_irq_all(denali);
> +}
> +
> static void denali_hw_init(struct denali_nand_info *denali)
> {
> /*
> @@ -1019,7 +1013,7 @@ static void denali_hw_init(struct denali_nand_info *denali)
> denali->bbtskipbytes = ioread32(denali->flash_reg +
> SPARE_AREA_SKIP_BYTES);
> detect_max_banks(denali);
> - denali_nand_reset(denali);
> + denali_reset_banks(denali);
> iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED);
> iowrite32(CHIP_EN_DONT_CARE__FLAG,
> denali->flash_reg + CHIP_ENABLE_DONT_CARE);
Powered by blists - more mailing lists