From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A12A7C4321D for ; Fri, 24 Aug 2018 12:55:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4DC6C21564 for ; Fri, 24 Aug 2018 12:55:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4DC6C21564 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727561AbeHXQaE (ORCPT ); Fri, 24 Aug 2018 12:30:04 -0400 Received: from mail.bootlin.com ([62.4.15.54]:47950 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726636AbeHXQaE (ORCPT ); Fri, 24 Aug 2018 12:30:04 -0400 Received: by mail.bootlin.com (Postfix, from userid 110) id B4E48207A3; Fri, 24 Aug 2018 14:55:30 +0200 (CEST) Received: from bbrezillon (AAubervilliers-681-1-53-19.w90-88.abo.wanadoo.fr [90.88.170.19]) by mail.bootlin.com (Postfix) with ESMTPSA id 6300020728; Fri, 24 Aug 2018 14:55:20 +0200 (CEST) Date: Fri, 24 Aug 2018 14:55:19 +0200 From: Boris Brezillon To: Masahiro Yamada Cc: linux-mtd@lists.infradead.org, Miquel Raynal , linux-kernel@vger.kernel.org, Marek Vasut , Brian Norris , Richard Weinberger , David Woodhouse Subject: Re: [PATCH] mtd: rawnand: denali: do not pass zero maxchips to nand_scan() Message-ID: <20180824145519.330133a4@bbrezillon> In-Reply-To: <1534839799-14112-1-git-send-email-yamada.masahiro@socionext.com> References: <1534839799-14112-1-git-send-email-yamada.masahiro@socionext.com> X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Masahiro, On Tue, 21 Aug 2018 17:23:19 +0900 Masahiro Yamada 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 in fact > 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, i.e. nand_set_defaults() is skipped. Therefore, the > driver must have 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, it is zero. > > The reason of this trick was, as commit f486287d2372 ("mtd: nand: > denali: fix bank reset function to detect the number of chips") > explained, nand_scan_ident() issued Set Features (0xEF) command > to all CS lines, some of which may not be connected with a chip. > Then, the driver would wait until R/B# response, which never happens. > > This problem was solved by commit 107b7d6a7ad4 ("mtd: rawnand: avoid > setting again the timings to mode 0 after a reset"). In the current > code, nand_setup_data_interface() is called from nand_scan_tail(), > which is after the chip detection is done. > > Remove the code that is causing NULL pointer dereference. Now, the > maxchips passed to nand_scan() is the maximum number of chip selects > supported by the IP (typically 4 or 8). Leave all the chip detection > process to nand_scan_ident(). > > Signed-off-by: Masahiro Yamada > --- > > drivers/mtd/nand/raw/denali.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c > index ca18612..3e4b8e1 100644 > --- a/drivers/mtd/nand/raw/denali.c > +++ b/drivers/mtd/nand/raw/denali.c > @@ -1086,7 +1086,6 @@ static void denali_reset_banks(struct denali_nand_info *denali) > } > > dev_dbg(denali->dev, "%d chips connected\n", i); > - denali->max_banks = i; Shouldn't we instead avoid calling nand_scan() when denali->max_banks=0? I mean, what's the point of calling this function if you know for sure it will fail. Last question: do we still need this denali_reset_banks()? If it's only about resetting the chip to detect how many are actually present, that's already done by nand_scan(). > } > > static void denali_hw_init(struct denali_nand_info *denali)