Patchwork MTD-related kobject badness during linux-next boot

login
register
mail settings
Submitter Brian Norris
Date Aug. 2, 2011, 5:40 p.m.
Message ID <CAN8TOE9mWEmS5YJqNrS9OrG=t+emeLcXNpKR1BiZ6VDJW7_c=A@mail.gmail.com>
Download mbox | patch
Permalink /patch/107979/
State New
Headers show

Comments

Brian Norris - Aug. 2, 2011, 5:40 p.m.
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(-)
Daniel Drake - Aug. 2, 2011, 5:58 p.m.
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
Brian Norris - Aug. 2, 2011, 6:05 p.m.
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
Jamie Iles - Aug. 2, 2011, 10:53 p.m.
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

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