Message ID | 1422752930-696-1-git-send-email-nks@flawful.org |
---|---|
State | Accepted |
Headers | show |
On Sun, Feb 01, 2015 at 02:08:50AM +0100, Niklas Cassel wrote: > Calling mtd_device_parse_register with the same mtd_info > (e.g. registering several partitions on a single device) > would add the same reboot notifier twice, causing an > infinte loop in notifier_chain_register during boot up. No driver should be calling mtd_device_parse_register() multiple times on the same mtd_info. Under what context do you see this? What driver? > Signed-off-by: Niklas Cassel <nks@flawful.org> > --- > drivers/mtd/mtdcore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index de79576..98efc1e 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > err = -ENODEV; > } > > - if (mtd->_reboot) { > + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) { > mtd->reboot_notifier.notifier_call = mtd_reboot_notifier; > register_reboot_notifier(&mtd->reboot_notifier); > } Brian
On 02/02/2015 12:07 AM, Brian Norris wrote: > On Sun, Feb 01, 2015 at 02:08:50AM +0100, Niklas Cassel wrote: >> Calling mtd_device_parse_register with the same mtd_info >> (e.g. registering several partitions on a single device) >> would add the same reboot notifier twice, causing an >> infinte loop in notifier_chain_register during boot up. > No driver should be calling mtd_device_parse_register() multiple times > on the same mtd_info. Under what context do you see this? What driver? arch/cris/arch-v32/drivers/axisflashmap.c It calls mtd_device_register for different partitions on same device using the same mtd_info. What do you suggest this driver should do instead? > >> Signed-off-by: Niklas Cassel <nks@flawful.org> >> --- >> drivers/mtd/mtdcore.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> index de79576..98efc1e 100644 >> --- a/drivers/mtd/mtdcore.c >> +++ b/drivers/mtd/mtdcore.c >> @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, >> err = -ENODEV; >> } >> >> - if (mtd->_reboot) { >> + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) { >> mtd->reboot_notifier.notifier_call = mtd_reboot_notifier; >> register_reboot_notifier(&mtd->reboot_notifier); >> } > Brian
On Mon, Feb 02, 2015 at 08:08:17AM +0100, Niklas Cassel wrote: > > On 02/02/2015 12:07 AM, Brian Norris wrote: > > On Sun, Feb 01, 2015 at 02:08:50AM +0100, Niklas Cassel wrote: > >> Calling mtd_device_parse_register with the same mtd_info > >> (e.g. registering several partitions on a single device) > >> would add the same reboot notifier twice, causing an > >> infinte loop in notifier_chain_register during boot up. > > No driver should be calling mtd_device_parse_register() multiple times > > on the same mtd_info. Under what context do you see this? What driver? > arch/cris/arch-v32/drivers/axisflashmap.c Yikes, that file is ugly! I am reminded of (one of the many reasons) why ARM ditched board files. > It calls mtd_device_register for different partitions on same device using the same mtd_info. > > What do you suggest this driver should do instead? Well, it would be really great if it could aggregate the partitions it wants all into one list (array) and pass that to the MTD core all at once. It also looks like this should have all been written as an MTD partition parser (struct mtd_part_parser), and some additional platform data to select the default maps if they aren't found on the flash. But there are a lot of odd cases in that code that make it difficult to do now. I'm tempted to suggest just calling add_mtd_partitions() directly, since you couldn't possibly be making good use out of the "parse" part of the mtd_device_parse_register() function. If you were, you'd get a ton of duplicate partitions, since the parser (e.g., cmdlinepart) would get called a bunch of times, and it would add the same partitions for *every* time you're calling mtd_device_register(). [1] But (as noted in mtdcore.h) add_mtd_partitions() is really private to the core MTD files. So... any chance you can rewrite this as an MTD parser + platform data? Also, now that you point this issue out, I see that one other driver might have the same problem as you -- diskonchip.c. It has a dubiously-formed partition registration setup. I'm tempted to remove its first mtd_device_register() and drop its 'no_autopart' parameter and see if anyone complains. If we have enough problems with this code, though, I might rip out some of: 3efe41be224c mtd: implement common reboot notifier boilerplate until I can find a better way. Brian [1] This suggests that your code is quite broken already, but just happens to work for the particular cases that are hard-coded here.
> On 02/02/2015 12:07 AM, Brian Norris wrote: > > > > No driver should be calling mtd_device_parse_register() multiple times > > on the same mtd_info. Under what context do you see this? What driver? > arch/cris/arch-v32/drivers/axisflashmap.c Looking at it another way, why should it not be allowed, if the only thing stopping it from working is Niklas' fix below? If there are multiple ways a driver can acquire partitioning information, it is handy to be able to do mtd_device_parse_register() several times, rather than collecting all the partitioning information in one place and doing the call there, which I suppose would be the alternative. I've noted that mtd/nand/diskonchip.c does it too: mtd_device_register(mtd, NULL, 0); if (!no_autopart) mtd_device_register(mtd, parts, numparts); but this driver might be essentially deprecated, I don't know. > >> Signed-off-by: Niklas Cassel <nks@flawful.org> > >> --- > >> drivers/mtd/mtdcore.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > >> index de79576..98efc1e 100644 > >> --- a/drivers/mtd/mtdcore.c > >> +++ b/drivers/mtd/mtdcore.c > >> @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > >> err = -ENODEV; > >> } > >> > >> - if (mtd->_reboot) { > >> + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) { > >> mtd->reboot_notifier.notifier_call = mtd_reboot_notifier; > >> register_reboot_notifier(&mtd->reboot_notifier); > >> } /Ricard
On Mon, Feb 02, 2015 at 10:01:51AM +0100, Ricard Wanderlof wrote: > > > On 02/02/2015 12:07 AM, Brian Norris wrote: > > > > > > No driver should be calling mtd_device_parse_register() multiple times > > > on the same mtd_info. Under what context do you see this? What driver? > > arch/cris/arch-v32/drivers/axisflashmap.c > > Looking at it another way, why should it not be allowed, if the only > thing stopping it from working is Niklas' fix below? It severely breaks the way that normal MTD partition parsers work, for one. cmdlineparts could never be sanely applied here, for instance. > If there are multiple > ways a driver can acquire partitioning information, it is handy to be able > to do mtd_device_parse_register() several times, rather than collecting > all the partitioning information in one place and doing the call there, > which I suppose would be the alternative. It may be handy, but it breaks the abstraction that is established in most every other case, that partitions are determined by exactly one source: a single list from platform data, device tree, the kernel command line, or an on-flash parser (e.g., RedBoot, bcm47xxpart, etc.). > I've noted that mtd/nand/diskonchip.c does it too: > > mtd_device_register(mtd, NULL, 0); > if (!no_autopart) > mtd_device_register(mtd, parts, numparts); > > but this driver might be essentially deprecated, I don't know. Yeah, I just noticed that one too. TBH, I'm not sure the reboot notifier patch places that code in the best place anyway. There really is no central initialization code for the 'master' mtd_info struct, as this initialization is handled ad-hoc in every single MTD driver. It just happens that the partition parsing/registration function fit nicely as the one place that (almost) all MTD master devices got registered. But this just exposed yet another instance where my assumptions are challenged. There's so much legacy crap in MTD that sometimes I wonder why I even bother... Brian
On Sun, Feb 01, 2015 at 02:08:50AM +0100, Niklas Cassel wrote: > Calling mtd_device_parse_register with the same mtd_info > (e.g. registering several partitions on a single device) > would add the same reboot notifier twice, causing an > infinte loop in notifier_chain_register during boot up. > > Signed-off-by: Niklas Cassel <nks@flawful.org> Begrudgingly applied to l2-mtd.git. It's better to fix this bug than for me just to complain about badly written board files and drivers... > --- > drivers/mtd/mtdcore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index de79576..98efc1e 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > err = -ENODEV; > } > Also added a FIXME comment here to note that some drivers are doing odd things to require this workaround. > - if (mtd->_reboot) { > + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) { > mtd->reboot_notifier.notifier_call = mtd_reboot_notifier; > register_reboot_notifier(&mtd->reboot_notifier); > } Brian
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index de79576..98efc1e 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, err = -ENODEV; } - if (mtd->_reboot) { + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) { mtd->reboot_notifier.notifier_call = mtd_reboot_notifier; register_reboot_notifier(&mtd->reboot_notifier); }
Calling mtd_device_parse_register with the same mtd_info (e.g. registering several partitions on a single device) would add the same reboot notifier twice, causing an infinte loop in notifier_chain_register during boot up. Signed-off-by: Niklas Cassel <nks@flawful.org> --- drivers/mtd/mtdcore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)