MTD-related kobject badness during linux-next boot

MTD-related kobject badness during linux-next boot

[Daniel Drake]

Hi,

Booting linux-next 20110707 on OLPC XO-1 I get:

[    0.965646] CAFE NAND 0000:00:0c.0: enabling device (0000 -> 0002)
[    0.984187] NAND device: Manufacturer ID: 0xad, Chip ID: 0xdc
(Hynix NAND 512MiB 3,3V 8-bit)
[    1.001115] 2 NAND chips detected
[    1.144358] Searching for RedBoot partition table in cafe_nand at offset 0x0
[    1.162364] No RedBoot partition table detected in cafe_nand
[    1.172947] kobject (cd8d40dc): tried to init an initialized
object, something is seriously wrong.
[    1.184111] Pid: 1, comm: swapper Not tainted 3.0.0-rc6-next-20110707+ #100
[    1.198384] Call Trace:
[    1.207741]  [<c05301a0>] ? kobject_init+0x24/0x6b
[    1.212420]  [<c0578c49>] ? device_initialize+0x18/0x56
[    1.227060]  [<c057960a>] ? device_register+0x8/0x10
[    1.241052]  [<c059104a>] ? add_mtd_device+0x17a/0x1f1
[    1.244949]  [<c0591128>] ? mtd_device_parse_register+0x67/0x7c
[    1.259536]  [<c06c6522>] ? cafe_nand_probe+0x5c9/0x686
[    1.263228]  [<c0546697>] ? pci_device_probe+0x52/0xb0
[    1.276828]  [<c057ad8b>] ? driver_probe_device+0x8a/0x109
[    1.290960]  [<c057ae4a>] ? __driver_attach+0x40/0x5b
[    1.294236]  [<c057a370>] ? bus_for_each_dev+0x37/0x60
[    1.307430] Switching to clocksource tsc
[    1.316560]  [<c057ab33>] ? driver_attach+0x14/0x17
[    1.329149]  [<c057ae0a>] ? driver_probe_device+0x109/0x109
[    1.362236] Switched to NOHz mode on CPU #0
[    1.390945]  [<c057a867>] ? bus_add_driver+0x88/0x1b1
[    1.422422]  [<c0530945>] ? kset_find_obj_hinted+0x6f/0x94
[    1.453723]  [<c07ef640>] ? start_kernel+0x293/0x293
[    1.483790]  [<c0803af6>] ? nand_base_init+0x12/0x12
[    1.513128]  [<c057b1ca>] ? driver_register+0x73/0xc6
[    1.541936]  [<c0803af6>] ? nand_base_init+0x12/0x12
[    1.570298]  [<c07ef640>] ? start_kernel+0x293/0x293
[    1.598567]  [<c0803af6>] ? nand_base_init+0x12/0x12
[    1.626318]  [<c0546ea3>] ? __pci_register_driver+0x2c/0x81
[    1.654298]  [<c07ef640>] ? start_kernel+0x293/0x293
[    1.680913]  [<c0803af6>] ? nand_base_init+0x12/0x12
[    1.706828]  [<c0401065>] ? do_one_initcall+0x65/0x104
[    1.733304]  [<c07ef640>] ? start_kernel+0x293/0x293
[    1.758893]  [<c07ef6a5>] ? kernel_init+0x65/0xda
[    1.783437]  [<c06cd9d6>] ? kernel_thread_helper+0x6/0xd

I think this is a recent regression but I haven't been testing enough
recent kernels to know exactly when it appeared.

This seems to cause shutdown to fail as well, as device_shutdown()
gets in an infinite loop trying to shut down mtd1.

Any ideas?

Thanks,
Daniel

Re: MTD-related kobject badness during linux-next boot

[Brian Norris]

Hi,

I CC'd Dmitry and Jamie who did recent work on this area of MTD,
regarding a subsystem-wide simplification of the routines for
partitioning and registering devices. I don't personally develop with
the cafe_nand driver, but I can try to help.

On Thu, Jul 14, 2011 at 10:37 AM, Daniel Drake <dsd@laptop.org> wrote:
> Booting linux-next 20110707 on OLPC XO-1 I get:
> [    1.172947] kobject (cd8d40dc): tried to init an initialized
> object, something is seriously wrong.
> [    1.184111] Pid: 1, comm: swapper Not tainted 3.0.0-rc6-next-20110707+ #100
> [    1.198384] Call Trace:
> [    1.207741]  [<c05301a0>] ? kobject_init+0x24/0x6b
> [    1.212420]  [<c0578c49>] ? device_initialize+0x18/0x56
> [    1.227060]  [<c057960a>] ? device_register+0x8/0x10
> [    1.241052]  [<c059104a>] ? add_mtd_device+0x17a/0x1f1
> [    1.244949]  [<c0591128>] ? mtd_device_parse_register+0x67/0x7c
> [    1.259536]  [<c06c6522>] ? cafe_nand_probe+0x5c9/0x686
...
> Any ideas?

Yeah, I think it has to do with Dmitry Eremin-Solenikov's recent
changes in l2-mtd-2.6.git. Looks like the driver is trying calling
add_mtd_device() on the master MTD twice. The problem commit is (for
now):
commit 0f7451bea72c64d3f0a47850328d52f0315e2ea6
"mtd: cafe_nand.c: use mtd_device_parse_register"

Have you tried linux 3.0, which does not have the patch series that
messes with mtd partition parsing and registering?

It looks like previously, cafe_nand would always add the master
device, then it would parse and register its partitions, if found.

Since Dmitry's change, it looks like cafe_nand will add the master
device, then parse and register its partitions, if found. However, if
partitions are NOT found, then mtd_device_parse_register() falls back
to adding the master device, which was already added. In
drivers/mtd/mtdcore.c, see:

     int mtd_device_parse_register(struct mtd_info *mtd, const char **types,
     ...
     if (err > 0) {
     ...
     } else if (err == 0) {
             err = add_mtd_device(mtd);
     ...


So it looks like perhaps we can solve the problem by just killing the
"register the whole device first" and allow mtd_device_parse_register
to do it if there are no partitions. Any cafe_nand developers know if
this is a problem? i.e., is there a reason we need both the whole
device AND the partitions sent to add_mtd_device()? I'll send a full
patch with sign-off and description if there are no objections.

Brian

---
 drivers/mtd/nand/cafe_nand.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index fb1425e..72d3f23 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -798,9 +798,6 @@ static int __devinit cafe_nand_probe(struct pci_dev *pdev,

        pci_set_drvdata(pdev, mtd);

-       /* We register the whole device first, separate from the partitions */
-       mtd_device_register(mtd, NULL, 0);
-
        mtd->name = "cafe_nand";
        mtd_device_parse_register(mtd, part_probes, 0, NULL, 0);

-- 
1.7.0.4

Re: MTD-related kobject badness during linux-next boot

[Daniel Drake]

On 2 August 2011 18:40, Brian Norris <computersforpeace@gmail.com> wrote:
> Have you tried linux 3.0, which does not have the patch series that
> messes with mtd partition parsing and registering?

Yes, and the problem does not appear there.

Daniel

Re: MTD-related kobject badness during linux-next boot

[Brian Norris]

On Tue, Aug 2, 2011 at 10:58 AM, Daniel Drake <dsd@laptop.org> wrote:
> On 2 August 2011 18:40, Brian Norris <computersforpeace@gmail.com> wrote:
>> Have you tried linux 3.0, which does not have the patch series that
>> messes with mtd partition parsing and registering?
>
> Yes, and the problem does not appear there.

As expected. Then let's wait and see if anyone has comments on my patch.

Brian

Re: MTD-related kobject badness during linux-next boot

[Jamie Iles]

Hi Brian,

On Tue, Aug 02, 2011 at 10:40:29AM -0700, Brian Norris wrote:
> Since Dmitry's change, it looks like cafe_nand will add the master
> device, then parse and register its partitions, if found. However, if
> partitions are NOT found, then mtd_device_parse_register() falls back
> to adding the master device, which was already added. In
> drivers/mtd/mtdcore.c, see:
> 
>      int mtd_device_parse_register(struct mtd_info *mtd, const char **types,
>      ...
>      if (err > 0) {
>      ...
>      } else if (err == 0) {
>              err = add_mtd_device(mtd);
>      ...
> 
> 
> So it looks like perhaps we can solve the problem by just killing the
> "register the whole device first" and allow mtd_device_parse_register
> to do it if there are no partitions. Any cafe_nand developers know if
> this is a problem? i.e., is there a reason we need both the whole
> device AND the partitions sent to add_mtd_device()? I'll send a full
> patch with sign-off and description if there are no objections.

I think that's the right thing to do.  There's actually a comment in 
drivers/mtd/mtdpart.c saying:

	/* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
         * to have the same data be in two different partitions.
         */

So I do think it should be the whole device *or* the partitions.  In any 
case, your patch looks good to me.

Jamie

[PATCH] mtd: cafe_nand: register master MTD at most once

[Brian Norris]

This patch addresses a regression where a master MTD object might be
registered twice if no partitions are found. The initial bug report can
be seen here:
    http://lists.infradead.org/pipermail/linux-mtd/2011-July/037086.html

The regression is caused by:
    commit 0f7451bea72c64d3f0a47850328d52f0315e2ea6
    "mtd: cafe_nand.c: use mtd_device_parse_register"

The aforementioned commit was intended to basically just refactor the
codebase; however, it had unintentional side effects. The cafe_nand probe
behavior over time is as follows.

Previous functionality (before commit 0f7451be):
 A) register the master device with add_mtd_device()
 B) if partitions are found, register them as well

New functionality (at commit 0f7451be):
 A) register the master device with add_mtd_device()
 B) if partitions are found, register them as well
 C) if partitions are not found, register the master device (again)

Correct functionality (this patch):
 (remove step A)
 B) if partitions are found, register them
 C) if partitions are not found, register the master device

Thus, this fix means that we will never register both the master MTD and
its partitions, as we did before commit 0f7451be. This is probably the
expected behavior and should be a welcome change.

Reported-by: Daniel Drake <dsd@laptop.org>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/cafe_nand.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index 661e5dc..11a56df 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -797,9 +797,6 @@ static int __devinit cafe_nand_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, mtd);
 
-	/* We register the whole device first, separate from the partitions */
-	mtd_device_register(mtd, NULL, 0);
-
 	mtd->name = "cafe_nand";
 	mtd_device_parse_register(mtd, part_probes, 0, NULL, 0);
 
-- 
1.7.0.4

Re: [PATCH] mtd: cafe_nand: register master MTD at most once

[Artem Bityutskiy]

On Tue, 2011-08-02 at 16:38 -0700, Brian Norris wrote:
> This patch addresses a regression where a master MTD object might be
> registered twice if no partitions are found. The initial bug report can
> be seen here:
>     http://lists.infradead.org/pipermail/linux-mtd/2011-July/037086.html
> 
> The regression is caused by:
>     commit 0f7451bea72c64d3f0a47850328d52f0315e2ea6
>     "mtd: cafe_nand.c: use mtd_device_parse_register"

Thanks for the fix! For some reason David missed the last merge window
and did not merge the MTD stuff. So we still have a possibility to fix
the original commit, so I folded this into commit 0f7451bea.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] mtd: cafe_nand: register master MTD at most once

[Brian Norris]

On Mon, Aug 15, 2011 at 8:31 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Thanks for the fix! For some reason David missed the last merge window
> and did not merge the MTD stuff. So we still have a possibility to fix
> the original commit, so I folded this into commit 0f7451bea.

OK, good.

Speaking of the folding, missing merge window, rebasing, etc., I think
you now have a commit in l2-mtd-2.6.git that refers to a hash that
doesn't exist in your tree anymore. For fear of continuing the trend
of having archived messages referring to holes in history, I will
simply list the name (not hash) of the commit whose description
probably should be edited:

"mtd: plat-nand: Fixup kerneldoc for struct platform_nand_chip"

Brian