Patchwork MTD-related kobject badness during linux-next boot

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

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

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

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