Re: Problems with 'mtd: warn when registering the same master many times'

[Brian Norris <computersforpeace@gmail.com>]

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.)

> 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.

Brian

[1] http://patchwork.ozlabs.org/patch/417981/