On Friday 20 February 2009, Stanislaw Gruszka wrote:
> Thursday 19 February 2009 17:49:52 Bartlomiej Zolnierkiewicz napisał(a):
> > > > Seems like ide_device_put() needs the same module_refcount() check that
> > > > is present in scsi_device_put() so removal of device driver won't trigger
> > > > a spurious module_put() on a host driver?
> > >
> > > I little surprise about scsi code (linux-scsi ML CC). Is comment inside
> > > scsi_device_put() function correct? Why scsi_device_get() not check
> > > try_module_get() return value? And most importand: there is reference
> > > counter check before put, so it can be 0, but data does it protect is in
> > > use ?
>
> Any comments?
>
> > Uh... we will need some more intrusive changes to the reference counting
> > to fix it -- like to replace idkp->kref by idkp->dev and make drive->gendev
> > a parent of it (so only after the final put on ->dev ->gendev can go away).
> >
> > [ IOW we need to have some changes similar to those done in sd.c by:
> > commit 6bdaa1f17dd32ec62345c7b57842f53e6278a2fa
> > and later by:
> > commit ee959b00c335d7780136c5abda37809191fe52c3 ]
> >
> > > There is no oops with my workaround, when I just remove ide_disk_put() from
> >
> > I suppose that after ide_disk_put() removal ide_disk_release() is simply
> > never called... ;)
> >
> > > ide_gd_remove(). It's strange why there is lack of symmetrical _put/_get calls,
> > > ide_gd_probe() has no call to ide_disk_get().
> >
> > We have kref_init() in ide_disk_probe(), so there is no need for it
> > and we also don't want to hold an extra reference on host driver...
>
> Looks that using ->dev insted of ->kref will do the work. But perhaps less
> intrusive fix, like check kref in ide_disk_put() would be better solution.
> I tested below patch and everythings is fine.
>
> diff --git a/drivers/ide/ide-gd.c b/drivers/ide/ide-gd.c
> index 7857b20..598f21b 100644
> --- a/drivers/ide/ide-gd.c
> +++ b/drivers/ide/ide-gd.c
> @@ -48,8 +48,8 @@ static void ide_disk_put(struct ide_disk_obj *idkp)
> ide_drive_t *drive = idkp->drive;
>
> mutex_lock(&ide_disk_ref_mutex);
> - kref_put(&idkp->kref, ide_disk_release);
> - ide_device_put(drive);
> + if (!kref_put(&idkp->kref, ide_disk_release))
> + ide_device_put(drive);
I worry that this just masks the problem as according to your previous
mail drive still can be already gone before ->flush in ide_gd_remove().
> mutex_unlock(&ide_disk_ref_mutex);
> }
>
> If this patch is ok and dropping kref to dev is not planed currently, maybe
> I'll send "official" patch with ide-gd fix and for other devices types.
Lets fix it fully. The below patch together with previous ide_device_put()
fix and drive_release_dev() one (from another mail) should make all problems
go away...
From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
Subject: [PATCH] ide: fix refcounting in device drivers
During host driver module removal del_gendisk() results in a final
put on drive->gendev and freeing the drive by drive_release_dev().
Convert device drivers from using struct kref to use struct device
so device driver's object holds reference on ->gendev and prevents
drive from prematurely going away.
Also fix ->remove methods to not erroneously drop reference on a
host driver by using only put_device() instead of ide*_put().
Reported-by: Stanislaw Gruszka <stf_xl@xxxxx>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
---
drivers/ide/ide-cd.c | 27 ++++++++++++++++++---------
drivers/ide/ide-cd.h | 2 +-
drivers/ide/ide-gd.c | 26 +++++++++++++++++---------
drivers/ide/ide-gd.h | 2 +-
drivers/ide/ide-tape.c | 29 +++++++++++++++++++----------
include/linux/ide.h | 2 +-
6 files changed, 57 insertions(+), 31 deletions(-)
Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -55,7 +55,7 @@
static DEFINE_MUTEX(idecd_ref_mutex);
-static void ide_cd_release(struct kref *);
+static void ide_cd_release(struct device *);
static struct cdrom_info *ide_cd_get(struct gendisk *disk)
{
@@ -67,7 +67,7 @@ static struct cdrom_info *ide_cd_get(str
if (ide_device_get(cd->drive))
cd = NULL;
else
- kref_get(&cd->kref);
+ get_device(&cd->dev);
}
mutex_unlock(&idecd_ref_mutex);
@@ -79,7 +79,7 @@ static void ide_cd_put(struct cdrom_info
ide_drive_t *drive = cd->drive;
mutex_lock(&idecd_ref_mutex);
- kref_put(&cd->kref, ide_cd_release);
+ put_device(&cd->dev);
ide_device_put(drive);
mutex_unlock(&idecd_ref_mutex);
}
@@ -1790,15 +1790,17 @@ static void ide_cd_remove(ide_drive_t *d
ide_debug_log(IDE_DBG_FUNC, "Call %s\n", __func__);
ide_proc_unregister_driver(drive, info->driver);
-
+ device_del(&info->dev);
del_gendisk(info->disk);
- ide_cd_put(info);
+ mutex_lock(&idecd_ref_mutex);
+ put_device(&info->dev);
+ mutex_unlock(&idecd_ref_mutex);
}
-static void ide_cd_release(struct kref *kref)
+static void ide_cd_release(struct device *dev)
{
- struct cdrom_info *info = to_ide_drv(kref, cdrom_info);
+ struct cdrom_info *info = to_ide_drv(dev, cdrom_info);
struct cdrom_device_info *devinfo = &info->devinfo;
ide_drive_t *drive = info->drive;
struct gendisk *g = info->disk;
@@ -1997,7 +1999,12 @@ static int ide_cd_probe(ide_drive_t *dri
ide_init_disk(g, drive);
- kref_init(&info->kref);
+ info->dev.parent = &drive->gendev;
+ info->dev.release = ide_cd_release;
+ dev_set_name(&info->dev, dev_name(&drive->gendev));
+
+ if (device_register(&info->dev))
+ goto out_free_disk;
info->drive = drive;
info->driver = &ide_cdrom_driver;
@@ -2011,7 +2018,7 @@ static int ide_cd_probe(ide_drive_t *dri
g->driverfs_dev = &drive->gendev;
g->flags = GENHD_FL_CD | GENHD_FL_REMOVABLE;
if (ide_cdrom_setup(drive)) {
- ide_cd_release(&info->kref);
+ put_device(&info->dev);
goto failed;
}
@@ -2021,6 +2028,8 @@ static int ide_cd_probe(ide_drive_t *dri
add_disk(g);
return 0;
+out_free_disk:
+ put_disk(g);
out_free_cd:
kfree(info);
failed:
Index: b/drivers/ide/ide-cd.h
===================================================================
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -80,7 +80,7 @@ struct cdrom_info {
ide_drive_t *drive;
struct ide_driver *driver;
struct gendisk *disk;
- struct kref kref;
+ struct device dev;
/* Buffer for table of contents. NULL if we haven't allocated
a TOC buffer for this device yet. */
Index: b/drivers/ide/ide-gd.c
===================================================================
--- a/drivers/ide/ide-gd.c
+++ b/drivers/ide/ide-gd.c
@@ -25,7 +25,7 @@ module_param(debug_mask, ulong, 0644);
static DEFINE_MUTEX(ide_disk_ref_mutex);
-static void ide_disk_release(struct kref *);
+static void ide_disk_release(struct device *);
static struct ide_disk_obj *ide_disk_get(struct gendisk *disk)
{
@@ -37,7 +37,7 @@ static struct ide_disk_obj *ide_disk_get
if (ide_device_get(idkp->drive))
idkp = NULL;
else
- kref_get(&idkp->kref);
+ get_device(&idkp->dev);
}
mutex_unlock(&ide_disk_ref_mutex);
return idkp;
@@ -48,7 +48,7 @@ static void ide_disk_put(struct ide_disk
ide_drive_t *drive = idkp->drive;
mutex_lock(&ide_disk_ref_mutex);
- kref_put(&idkp->kref, ide_disk_release);
+ put_device(&idkp->dev);
ide_device_put(drive);
mutex_unlock(&ide_disk_ref_mutex);
}
@@ -66,17 +66,18 @@ static void ide_gd_remove(ide_drive_t *d
struct gendisk *g = idkp->disk;
ide_proc_unregister_driver(drive, idkp->driver);
-
+ device_del(&idkp->dev);
del_gendisk(g);
-
drive->disk_ops->flush(drive);
- ide_disk_put(idkp);
+ mutex_lock(&ide_disk_ref_mutex);
+ put_device(&idkp->dev);
+ mutex_unlock(&ide_disk_ref_mutex);
}
-static void ide_disk_release(struct kref *kref)
+static void ide_disk_release(struct device *dev)
{
- struct ide_disk_obj *idkp = to_ide_drv(kref, ide_disk_obj);
+ struct ide_disk_obj *idkp = to_ide_drv(dev, ide_disk_obj);
ide_drive_t *drive = idkp->drive;
struct gendisk *g = idkp->disk;
@@ -348,7 +349,12 @@ static int ide_gd_probe(ide_drive_t *dri
ide_init_disk(g, drive);
- kref_init(&idkp->kref);
+ idkp->dev.parent = &drive->gendev;
+ idkp->dev.release = ide_disk_release;
+ dev_set_name(&idkp->dev, dev_name(&drive->gendev));
+
+ if (device_register(&idkp->dev))
+ goto out_free_disk;
idkp->drive = drive;
idkp->driver = &ide_gd_driver;
@@ -373,6 +379,8 @@ static int ide_gd_probe(ide_drive_t *dri
add_disk(g);
return 0;
+out_free_disk:
+ put_disk(g);
out_free_idkp:
kfree(idkp);
failed:
Index: b/drivers/ide/ide-gd.h
===================================================================
--- a/drivers/ide/ide-gd.h
+++ b/drivers/ide/ide-gd.h
@@ -17,7 +17,7 @@ struct ide_disk_obj {
ide_drive_t *drive;
struct ide_driver *driver;
struct gendisk *disk;
- struct kref kref;
+ struct device dev;
unsigned int openers; /* protected by BKL for now */
/* Last failed packet command */
Index: b/drivers/ide/ide-tape.c
===================================================================
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -169,7 +169,7 @@ typedef struct ide_tape_obj {
ide_drive_t *drive;
struct ide_driver *driver;
struct gendisk *disk;
- struct kref kref;
+ struct device dev;
/*
* failed_pc points to the last failed packet command, or contains
@@ -267,7 +267,7 @@ static DEFINE_MUTEX(idetape_ref_mutex);
static struct class *idetape_sysfs_class;
-static void ide_tape_release(struct kref *);
+static void ide_tape_release(struct device *);
static struct ide_tape_obj *ide_tape_get(struct gendisk *disk)
{
@@ -279,7 +279,7 @@ static struct ide_tape_obj *ide_tape_get
if (ide_device_get(tape->drive))
tape = NULL;
else
- kref_get(&tape->kref);
+ get_device(&tape->dev);
}
mutex_unlock(&idetape_ref_mutex);
return tape;
@@ -290,7 +290,7 @@ static void ide_tape_put(struct ide_tape
ide_drive_t *drive = tape->drive;
mutex_lock(&idetape_ref_mutex);
- kref_put(&tape->kref, ide_tape_release);
+ put_device(&tape->dev);
ide_device_put(drive);
mutex_unlock(&idetape_ref_mutex);
}
@@ -308,7 +308,7 @@ static struct ide_tape_obj *ide_tape_chr
mutex_lock(&idetape_ref_mutex);
tape = idetape_devs[i];
if (tape)
- kref_get(&tape->kref);
+ get_device(&tape->dev);
mutex_unlock(&idetape_ref_mutex);
return tape;
}
@@ -2256,15 +2256,17 @@ static void ide_tape_remove(ide_drive_t
idetape_tape_t *tape = drive->driver_data;
ide_proc_unregister_driver(drive, tape->driver);
-
+ device_del(&tape->dev);
ide_unregister_region(tape->disk);
- ide_tape_put(tape);
+ mutex_lock(&idetape_ref_mutex);
+ put_device(&tape->dev);
+ mutex_unlock(&idetape_ref_mutex);
}
-static void ide_tape_release(struct kref *kref)
+static void ide_tape_release(struct device *dev)
{
- struct ide_tape_obj *tape = to_ide_drv(kref, ide_tape_obj);
+ struct ide_tape_obj *tape = to_ide_drv(dev, ide_tape_obj);
ide_drive_t *drive = tape->drive;
struct gendisk *g = tape->disk;
@@ -2407,7 +2409,12 @@ static int ide_tape_probe(ide_drive_t *d
ide_init_disk(g, drive);
- kref_init(&tape->kref);
+ tape->dev.parent = &drive->gendev;
+ tape->dev.release = ide_tape_release;
+ dev_set_name(&tape->dev, dev_name(&drive->gendev));
+
+ if (device_register(&tape->dev))
+ goto out_free_disk;
tape->drive = drive;
tape->driver = &idetape_driver;
@@ -2436,6 +2443,8 @@ static int ide_tape_probe(ide_drive_t *d
return 0;
+out_free_disk:
+ put_disk(g);
out_free_tape:
kfree(tape);
failed:
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -663,7 +663,7 @@ typedef struct ide_drive_s ide_drive_t;
#define to_ide_device(dev) container_of(dev, ide_drive_t, gendev)
#define to_ide_drv(obj, cont_type) \
- container_of(obj, struct cont_type, kref)
+ container_of(obj, struct cont_type, dev)
#define ide_drv_g(disk, cont_type) \
container_of((disk)->private_data, struct cont_type, driver)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
