From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753551AbbLJHNR (ORCPT ); Thu, 10 Dec 2015 02:13:17 -0500 Received: from mail-oi0-f50.google.com ([209.85.218.50]:33527 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751001AbbLJHNQ (ORCPT ); Thu, 10 Dec 2015 02:13:16 -0500 MIME-Version: 1.0 In-Reply-To: <20151209231901.GA64855@google.com> References: <56613747.8040907@denx.de> <566151DE.9070706@denx.de> <20151209231901.GA64855@google.com> Date: Thu, 10 Dec 2015 08:13:15 +0100 Message-ID: Subject: Re: mtd, nand, omap2: parse cmdline partition fail From: Frans Klaver To: Brian Norris Cc: Heiko Schocher , David Woodhouse , Boris BREZILLON , Pekon Gupta , Roger Quadros , Nicholas Mc Guire , linux-mtd@lists.infradead.org, "linux-kernel@vger.kernel.org" , Stefano Babic , "Stahl Martin (Helbling Technik)" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 10, 2015 at 12:19 AM, Brian Norris wrote: > On Fri, Dec 04, 2015 at 09:42:06AM +0100, Heiko Schocher wrote: >> It seems to me add_mtd_device() gets only called for the mtd partitions >> parsed from the cmdline ... > > That's true, when CONFIG_MTD_PARTITIONED_MASTER=n. (I'm really thinking > we should accelerate the adoption of PARTITIONED_MASTER... maybe set it > to default =y?) > > But even with CONFIG_MTD_PARTITIONED_MASTER=y we still have a problem. > > [...] > >> >The fact that this produces different names for you is slightly >> >surprising to me, unless mtd->name is already set to something by the >> >time it reaches add_mtd_device(). Or I overlooked something, which is >> >entirely plausible as well. >> > >> >So effectively this should be the same as doing: >> > >> > mtd->dev.parent = &pdev->dev; >> > mtd->name = dev_name(mtd->dev.parent); > > Yes, except for one thing (and this is the key): the mtd->name only gets > set *after* the partitions are parsed, using the method from commit > 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set"). So > the parsers (including cmdlinepart) get run with a blank (NULL) name, > and you can't detect any partitions, since the name match will never > work. Right, that was something we overlooked earlier. > I have a hack of a patch below (untested) that would hopefully work > (based on current l2-mtd.git). I could port this to a base on 4.4-rc1 if > it works OK, so we can get the regression fixed in this cycle. That would be nice. >> >>But wondering, if there are two or more identical nand chips in the >> >>system, they will have the same mtd->name ... which seems buggy to me... >> > >> >Agree. >> >> Good, so we must fix it ;-) > > Yeah, that's a bad problem. Most people only plan for one chip and then > realize they need 2 later. nand_base should probably support something > to do this easily. Unfortunately, making a change like that could cause > some problems with cmdline naming across kernel versions, if we start > changing the convention for some drivers...(do we consider the MTD name > part of the ABI?) I would expect a name to be just a name; something parsable by humans. By definition that cannot be an ABI. On the other hand, maybe it has grown to become part of the ABI. > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 89d811e7b04a..185dc36c687f 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -592,6 +592,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > struct mtd_partitions parsed; > int ret; > > + if (mtd->dev.parent) { > + if (!mtd->owner && mtd->dev.parent->driver) > + mtd->owner = mtd->dev.parent->driver->owner; > + if (!mtd->name) > + mtd->name = dev_name(mtd->dev.parent); > + } else { > + pr_debug("mtd device won't show a device symlink in sysfs\n"); > + } > + > memset(&parsed, 0, sizeof(parsed)); > > ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); This was the first thing I thought of when this issue was brought up. If you do this, do you still need the chunk of code you copied from in add_mtd_device()? Since we fill in these things on the master, I wouldn't think we do. Thanks, Frans