Re: sdhci driver produces kernel oops on ejecting the card

[Russell King <rmk+lkml@arm.linux.org.uk>]

On Fri, Apr 07, 2006 at 10:47:54AM +0200, Pierre Ossman wrote:
> Mikkel Erup wrote:
> > 
> > It happens with 2.6.16-git20 as well.
> > Attached are the log file and kernel .config
> > 
> 
> Since it ooopses during umount, I'm guessing that it's a problem
> somewhere in mmc_block. I'll try to get some time to look closer at it
> during the weekend. Perhaps Russell has some idea until then?

$ grep -n driverfs_dev block/genhd.c
558:    physdev = disk->driverfs_dev;
$

Hmm, okay, genhd contains a reference to a device object, but there's
no sign of _any_ refcounting in sight.

What's happening is that the MMC card block device is setup and registered
with genhd.  We set md->disk->driverfs_dev to point at the owning device
structure.

This generates a uevent, which causes disk->driverfs_dev to be dereferenced.
All fine here.  We mount the partition, which causes another uevent to be
generated, again dereferencing disk->driverfs_dev.

If we remove the MMC card, we destroy the MMC card block device.  This
seems to generate another uevent for the block device.  At this point,
the counted references to the MMC card block device fall to zero.

But wait!  There's still uncounted disk->driverfs_dev reference waiting
for...

You unmount the partition.  This calls block_uevent, which dereferences
disk->driverfs_dev.  You know what happens now.

The levels above genhd can't do the refcounting because they don't know
when stuff has finished with driverfs_dev.  So the only place for sane
refcounting seems to be genhd.c, as per the patch below.

Comments?

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej -x .git a/block/genhd.c b/block/genhd.c
--- a/block/genhd.c	Sat Feb 18 10:31:37 2006
+++ b/block/genhd.c	Fri Apr  7 15:22:21 2006
@@ -262,6 +262,7 @@ static int exact_lock(dev_t dev, void *d
  */
 void add_disk(struct gendisk *disk)
 {
+	get_device(disk->driverfs_dev);
 	disk->flags |= GENHD_FL_UP;
 	blk_register_region(MKDEV(disk->major, disk->first_minor),
 			    disk->minors, NULL, exact_match, exact_lock, disk);
@@ -507,6 +508,7 @@ static struct attribute * default_attrs[
 static void disk_release(struct kobject * kobj)
 {
 	struct gendisk *disk = to_disk(kobj);
+	put_device(disk->driverfs_dev);
 	kfree(disk->random);
 	kfree(disk->part);
 	free_disk_stats(disk);


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core