Messages in this thread | | | Date | Mon, 2 Nov 2015 12:09:31 -0800 | From | Guenter Roeck <> | Subject | Re: Problems with 'mtd: warn when registering the same master many times' |
| |
On Mon, Nov 02, 2015 at 09:43:56AM -0800, Brian Norris wrote: > On Mon, Nov 02, 2015 at 07:41:48AM -0800, Guenter Roeck wrote: > > Brian, > > > > I see the following warnings in recent mips qemu tests on linux-next. > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:619 mtd_device_parse_register+0x160/0x16c() > > MTD already registered > > > > Looking into the code, this is the result of your patch 'mtd: warn when registering > > the same master many times'. > > > > This patch is supposed to warn if mtd_device_parse_register is called multiple times > > with the same mtd_info structure pointer as parameter. > > > > At the surface, the check appears to make sense. However, it has a problem. > > The output of "git grep reboot_notifier.notifier_call" in drivers/mtd results in > > > > chips/cfi_cmdset_0001.c: mtd->reboot_notifier.notifier_call = cfi_intelext_reboot; > > chips/cfi_cmdset_0002.c: mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot; > > mtdcore.c: WARN_ONCE(mtd->reboot_notifier.notifier_call, "MTD already registered\n"); > > mtdcore.c: if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) { > > mtdcore.c: mtd->reboot_notifier.notifier_call = mtd_reboot_notifier; > > > > As it turns out, the observed warning is not seen because mtd_device_parse_register() > > is called multiple times. It is seen because mtd->reboot_notifier.notifier_call > > is already set to cfi_intelext_reboot by the time it is checked. > > Hmm, you're right. I overlooked this one. FWIW, this would be > ameliorated by this patch [1] but I never got around to testing it > properly, so I didn't merge it. (Could you test it? If so, I might just > apply it as a fix.) > Seems to be working, or at least my test passes without problems after I applied it on top of -next. I sent you a Tested-by: as response to that patch.
> > I don't know if this is a bug in the mtd code, or if this is a valid use case. > > Whatever it is, the warning does not warn about the driver, it warns about a potential > > inconsistency in the mtd code itself (and if it is is a valid use case, the warning > > is inappropriate). Maybe it would make sense to change the warning as follows ? > > > > WARN_ONCE(mtd->_reboot && mtd->reboot_notifier.notifier_call, "MTD already registered\n"); > > That would be an OK fix, in the event that the above patch isn't taken. > > > I am not sure, though, if that is correct, since even that might be a valid use case > > (a caller registering a cfi based mtd device with a _reboot callback). > > No, those two are supposed to be mutually exclusive before MTD > registration; either the MTD driver provides a _reboot() callback and > MTD core registers the notifier (which fills out .notifier_call), or the > driver itself steals that notifier structure but never registers a > _reboot() callback. So no driver should actually need both. > > I'd really prefer it if we could just transition to the CFI drivers to > let MTD register the notifier for us, but I'm not 100% confident without > testing. > > > I also see the warning in crisv32 runtime tests. This is because the code in > > arch/cris/arch-v32/drivers/axisflashmap.c calls mtd_device_register() multiple times > > with the same mtd_info argument, each time to register a different partition. > > I am not sure if the check is appropriate for this case either, since the code calls > > mtd_device_register(), both 'type' and 'parser_data' are NULL, parse_mtd_partitions() > > does not do anything, and the problem you are concerned about does not apply. > > Actually, that platform is probably one of the main reasons for the > warning patch. It is not kosher to call mtd_device_register() as many > times as it does. So, you get a warning until somebody can be bothered > to fix that ugly code. > > > How about changing the warning to something like the following ? > > > > WARN_ONCE(types && mtd->_reboot && mtd->reboot_notifier.notifier_call, "MTD already registered\n"); > > No, that doesn't make much sense. We might as well just be removing the > check entirely at that point, since this just looks like you're shooting > at a random/fragile hack. > > > This would eliminate (what I think are) false positives and only warn if there > > is a real problem. > > I think we have the option of either taking patch [1] or taking your > first suggestion. But the axisflashmap.c is not a false positive, and it > should be fixed. Or just live with the warning, if it's unmaintained. > We'll see if Jesper has any comments. I didn't see an easy way to fix that driver myself, so I guess we may have to live with the warning for now.
Thanks, Guenter
| |