Messages in this thread | | | Date | Thu, 10 Dec 2015 08:13:15 +0100 | Subject | Re: mtd, nand, omap2: parse cmdline partition fail | From | Frans Klaver <> |
| |
On Thu, Dec 10, 2015 at 12:19 AM, Brian Norris <computersforpeace@gmail.com> 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
| |