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.
Thanks,
Guenter
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/
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
On Mon, Nov 02, 2015 at 12:09:31PM -0800, Guenter Roeck wrote:
> 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:
> > > 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.
Yeah, I'm sorry to say that we're aware of the problem but haven't
got to fixing it... At one time (3.20?), the code actually went into
an infinite loop on startup. :-(
I'll boost this to the head of the queue to be fixed, but I hope you can live
with the warning until we can find a mythical "Round Tuit".
> Thanks,
> Guenter
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]