Commit 49aa76b16676 ("mtd: rawnand: do not execute nand_scan_ident()
if maxchips is zero") gave a new meaning for calling nand_scan_ident()
with maxchips=0.
It is a special usage for some drivers such as docg4, but actually
the Denali driver may pass maxchips=0 to nand_scan() when the driver
is enabled but no NAND chip is found on the board for some reasons.
If nand_scan_with_ids() is called with maxchips=0, nand_scan_ident()
is skipped, then nand_set_defaults() is skipped as well. Thus, the
driver must set chip->controller beforehand. Otherwise, nand_attach()
causes NULL pointer dereference.
In fact, the Denali controller knows the number of connected chips
before calling nand_scan_ident(); if DEVICE_RESET fails, there is no
chip in that chip select. Then, denali_reset_banks() sets the maxchips
to the number of detected chips. If no chip is found, maxchips is zero.
In this case, there is no point for calling nand_scan() because we know
it will fail for sure. Let's make the probe function fail immediately.
Signed-off-by: Masahiro Yamada <[email protected]>
---
Changes in v2:
- Return -ENODEV immediately if no chip was found on the board
for some reasons.
This is the smallest, and safest change for the fixes branch.
(I will investigate later if further cleanups are possible or not.)
drivers/mtd/nand/raw/denali.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index ca18612..67b2065 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -1338,6 +1338,11 @@ int denali_init(struct denali_nand_info *denali)
denali_enable_irq(denali);
denali_reset_banks(denali);
+ if (!denali->max_banks) {
+ /* Error out earlier if no chip is found for some reasons. */
+ ret = -ENODEV;
+ goto disable_irq;
+ }
denali->active_bank = DENALI_INVALID_BANK;
--
2.7.4
On Mon, 27 Aug 2018 16:01:41 +0900
Masahiro Yamada <[email protected]> wrote:
> Commit 49aa76b16676 ("mtd: rawnand: do not execute nand_scan_ident()
> if maxchips is zero") gave a new meaning for calling nand_scan_ident()
> with maxchips=0.
>
> It is a special usage for some drivers such as docg4, but actually
> the Denali driver may pass maxchips=0 to nand_scan() when the driver
> is enabled but no NAND chip is found on the board for some reasons.
>
> If nand_scan_with_ids() is called with maxchips=0, nand_scan_ident()
> is skipped, then nand_set_defaults() is skipped as well. Thus, the
> driver must set chip->controller beforehand. Otherwise, nand_attach()
> causes NULL pointer dereference.
>
> In fact, the Denali controller knows the number of connected chips
> before calling nand_scan_ident(); if DEVICE_RESET fails, there is no
> chip in that chip select. Then, denali_reset_banks() sets the maxchips
> to the number of detected chips. If no chip is found, maxchips is zero.
>
> In this case, there is no point for calling nand_scan() because we know
> it will fail for sure. Let's make the probe function fail immediately.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Looks good to me. I'm waiting for Miquel's ack and I'll queue it to
mtd/fixes.
Thanks,
Boris
Hi Masahiro,
Masahiro Yamada <[email protected]> wrote on Mon, 27 Aug
2018 16:01:41 +0900:
> Commit 49aa76b16676 ("mtd: rawnand: do not execute nand_scan_ident()
> if maxchips is zero") gave a new meaning for calling nand_scan_ident()
> with maxchips=0.
>
> It is a special usage for some drivers such as docg4, but actually
> the Denali driver may pass maxchips=0 to nand_scan() when the driver
> is enabled but no NAND chip is found on the board for some reasons.
>
> If nand_scan_with_ids() is called with maxchips=0, nand_scan_ident()
> is skipped, then nand_set_defaults() is skipped as well. Thus, the
> driver must set chip->controller beforehand. Otherwise, nand_attach()
> causes NULL pointer dereference.
>
> In fact, the Denali controller knows the number of connected chips
> before calling nand_scan_ident(); if DEVICE_RESET fails, there is no
> chip in that chip select. Then, denali_reset_banks() sets the maxchips
> to the number of detected chips. If no chip is found, maxchips is zero.
>
> In this case, there is no point for calling nand_scan() because we know
> it will fail for sure. Let's make the probe function fail immediately.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> Changes in v2:
> - Return -ENODEV immediately if no chip was found on the board
> for some reasons.
> This is the smallest, and safest change for the fixes branch.
> (I will investigate later if further cleanups are possible or not.)
>
> drivers/mtd/nand/raw/denali.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index ca18612..67b2065 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -1338,6 +1338,11 @@ int denali_init(struct denali_nand_info *denali)
>
> denali_enable_irq(denali);
> denali_reset_banks(denali);
> + if (!denali->max_banks) {
> + /* Error out earlier if no chip is found for some reasons. */
> + ret = -ENODEV;
> + goto disable_irq;
> + }
>
> denali->active_bank = DENALI_INVALID_BANK;
>
Acked-by: Miquel Raynal <[email protected]>
Boris, This is for you :)
Thanks,
Miquèl
On Mon, 27 Aug 2018 16:01:41 +0900
Masahiro Yamada <[email protected]> wrote:
> Commit 49aa76b16676 ("mtd: rawnand: do not execute nand_scan_ident()
> if maxchips is zero") gave a new meaning for calling nand_scan_ident()
> with maxchips=0.
>
> It is a special usage for some drivers such as docg4, but actually
> the Denali driver may pass maxchips=0 to nand_scan() when the driver
> is enabled but no NAND chip is found on the board for some reasons.
>
> If nand_scan_with_ids() is called with maxchips=0, nand_scan_ident()
> is skipped, then nand_set_defaults() is skipped as well. Thus, the
> driver must set chip->controller beforehand. Otherwise, nand_attach()
> causes NULL pointer dereference.
>
> In fact, the Denali controller knows the number of connected chips
> before calling nand_scan_ident(); if DEVICE_RESET fails, there is no
> chip in that chip select. Then, denali_reset_banks() sets the maxchips
> to the number of detected chips. If no chip is found, maxchips is zero.
>
> In this case, there is no point for calling nand_scan() because we know
> it will fail for sure. Let's make the probe function fail immediately.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Queued to mtd/master after adding
49aa76b16676 ("mtd: rawnand: do not execute nand_scan_ident() if maxchips is zero")
Thanks,
Boris
> ---
>
> Changes in v2:
> - Return -ENODEV immediately if no chip was found on the board
> for some reasons.
> This is the smallest, and safest change for the fixes branch.
> (I will investigate later if further cleanups are possible or not.)
>
> drivers/mtd/nand/raw/denali.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index ca18612..67b2065 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -1338,6 +1338,11 @@ int denali_init(struct denali_nand_info *denali)
>
> denali_enable_irq(denali);
> denali_reset_banks(denali);
> + if (!denali->max_banks) {
> + /* Error out earlier if no chip is found for some reasons. */
> + ret = -ENODEV;
> + goto disable_irq;
> + }
>
> denali->active_bank = DENALI_INVALID_BANK;
>