Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753392AbbLIIeX (ORCPT ); Wed, 9 Dec 2015 03:34:23 -0500 Received: from down.free-electrons.com ([37.187.137.238]:57707 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752179AbbLIIeU (ORCPT ); Wed, 9 Dec 2015 03:34:20 -0500 Date: Wed, 9 Dec 2015 09:32:10 +0100 From: Boris Brezillon To: Brian Norris Cc: David Woodhouse , linux-mtd@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Hartley Sweeten , Ryan Mallon , Shawn Guo , Sascha Hauer , Imre Kaloz , Krzysztof Halasa , Tony Lindgren , linux-omap@vger.kernel.org, Alexander Clouter , Thomas Petazzoni , Gregory CLEMENT , Jason Cooper , Sebastian Hesselbarth , Andrew Lunn , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Marek Vasut , Steven Miao , adi-buildroot-devel@lists.sourceforge.net, Mikael Starvik , Jesper Nilsson , linux-cris-kernel@axis.com, Josh Wu , Wan ZongShun , Ezequiel Garcia , Maxim Levitsky , Kukjin Kim , Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi@googlegroups.com, Stefan Agner , Greg Kroah-Hartman , devel@driverdev.osuosl.org, Julia Lawall Subject: Re: [PATCH v3 bis 12/25] mtd: nand: use the mtd instance embedded in struct nand_chip Message-ID: <20151209093210.007a63b3@bbrezillon> In-Reply-To: <20151209001741.GT120110@google.com> References: <1448967802-25796-1-git-send-email-boris.brezillon@free-electrons.com> <1449046201-22921-1-git-send-email-boris.brezillon@free-electrons.com> <20151209001741.GT120110@google.com> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.27; 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2727 Lines: 89 Hi Brian, On Tue, 8 Dec 2015 16:17:41 -0800 Brian Norris wrote: > > > diff --git a/drivers/mtd/nand/cmx270_nand.c b/drivers/mtd/nand/cmx270_nand.c > > index 43bded6..84d027e 100644 > > --- a/drivers/mtd/nand/cmx270_nand.c > > +++ b/drivers/mtd/nand/cmx270_nand.c > > @@ -160,10 +160,8 @@ static int __init cmx270_init(void) > > gpio_direction_input(GPIO_NAND_RB); > > > > /* Allocate memory for MTD device structure and private data */ > > - cmx270_nand_mtd = kzalloc(sizeof(struct mtd_info) + > > - sizeof(struct nand_chip), > > - GFP_KERNEL); > > - if (!cmx270_nand_mtd) { > > + this = kzalloc(sizeof(struct nand_chip), GFP_KERNEL); > > + if (!this) { > > ret = -ENOMEM; > > goto err_kzalloc; > > } > > @@ -175,8 +173,7 @@ static int __init cmx270_init(void) > > goto err_ioremap; > > } > > > > - /* Get pointer to private data */ > > - this = (struct nand_chip *)(&cmx270_nand_mtd[1]); > > + cmx270_nand_mtd = nand_to_mtd(this); > > So, you make cmx270_nand_mtd no longer kzalloc()'d, but I still see the > cmx270_init() function end with: > > err_scan: > iounmap(cmx270_nand_io); > err_ioremap: > kfree(cmx270_nand_mtd); <----- *** this! *** Oh, crap. > err_kzalloc: > gpio_free(GPIO_NAND_RB); > err_gpio_request: > gpio_free(GPIO_NAND_CS); > > return ret; > > } > > I have a feeling there's a failing in your coccinelle script somewhere. These changes are not automated, because it's kind of hard to address all the different cases where the following pattern is employed; var = kzalloc(sizeof(struct mtd_info) + sizeof(struct nand_chip) + ..., ...); Sometime var is an mtd_info pointer, sometime it's a nand_chip pointer or directly a pointer to the private struct. I'm pretty sure we could come up with a valid coccinelle script, but given the number of drivers using this approach I'm not sure it is worth it... > > Given that I was only through 10 of 49 files changes, I think you might > need to take a comb over your patch better. I'll go over those changes one more time, but from my experience, these kind of bugs are spotted more easily by people who didn't write the code, so other reviews are more than welcome. Also, as you suggested, I'll split the changes in several commits (one per driver) so that you can pick them independently. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/