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

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.

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");

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

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.

How about changing the warning to something like the following ?

WARN_ONCE(types && mtd->_reboot && mtd->reboot_notifier.notifier_call, "MTD already registered\n");

This would eliminate (what I think are) false positives and only warn if there is a real problem.

Quoted 22 lines · Guenter Roeck

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;

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 24 lines · Brian Norris

Quoted 6 lines · Guenter Roeck

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.

Quoted 3 lines

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.

Quoted 2 lines

This would eliminate (what I think are) false positives and only warn if there is a real problem.

Quoted 4 lines

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