Re: Bad module reference counter

[Stanislaw Gruszka <stf_xl@wp.pl>]

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

Regards
Stanislaw Gruszka
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html