[PATCH] Fix ide unregister vs. driver model

[PATCH] Fix ide unregister vs. driver model

[Benjamin Herrenschmidt]

Hi Bart !

This patch seem to have been lost, so here it is again. It fixes
an Ooops on unregistering hwifs due to the device model now having
mandatory release() functions. It also close the possible race we
had on release if the entry was in use (by or /sys typically) by
using a semaphore waiting for the release() to be called after
doing an unregister.

It also include the fix for the gendev parent that wasn't merged
neither.

Please submit to Linus,

Regards,
Ben.

===== drivers/ide/ide-probe.c 1.58 vs edited =====
--- 1.58/drivers/ide/ide-probe.c	Fri Aug 15 01:52:06 2003
+++ edited/drivers/ide/ide-probe.c	Sun Aug 24 15:00:35 2003
@@ -644,15 +644,25 @@
 	return drive->present;
 }
 
+static void hwif_release_dev (struct device *dev)
+{
+	ide_hwif_t *hwif = container_of(dev, ide_hwif_t, gendev);
+
+	up(&hwif->gendev_rel_sem);
+}
+
 static void hwif_register (ide_hwif_t *hwif)
 {
 	/* register with global device tree */
 	strlcpy(hwif->gendev.bus_id,hwif->name,BUS_ID_SIZE);
 	hwif->gendev.driver_data = hwif;
+	if (hwif->gendev.parent == NULL) {
 	if (hwif->pci_dev)
 		hwif->gendev.parent = &hwif->pci_dev->dev;
 	else
 		hwif->gendev.parent = NULL; /* Would like to do = &device_legacy */
+	}
+	hwif->gendev.release = hwif_release_dev;
 	device_register(&hwif->gendev);
 }
 
@@ -1201,6 +1211,13 @@
 	return -ENOMEM;
 }
 
+static void drive_release_dev (struct device *dev)
+{
+	ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
+
+	up(&drive->gendev_rel_sem);
+}
+
 /*
  * init_gendisk() (as opposed to ide_geninit) is called for each major device,
  * after probing for drives, to allocate partition tables and other data
@@ -1219,6 +1236,7 @@
 		drive->gendev.parent = &hwif->gendev;
 		drive->gendev.bus = &ide_bus_type;
 		drive->gendev.driver_data = drive;
+		drive->gendev.release = drive_release_dev;
 		if (drive->present) {
 			device_register(&drive->gendev);
 			sprintf(drive->devfs_name, "ide/host%d/bus%d/target%d/lun%d",
===== drivers/ide/ide.c 1.87 vs edited =====
--- 1.87/drivers/ide/ide.c	Wed Aug 20 18:01:03 2003
+++ edited/drivers/ide/ide.c	Sun Aug 24 15:00:54 2003
@@ -255,6 +255,8 @@
 	hwif->mwdma_mask = 0x80;	/* disable all mwdma */
 	hwif->swdma_mask = 0x80;	/* disable all swdma */
 
+	sema_init(&hwif->gendev_rel_sem, 0);
+
 	default_hwif_iops(hwif);
 	default_hwif_transport(hwif);
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
@@ -277,6 +279,7 @@
 		drive->driver			= &idedefault_driver;
 		drive->vdma			= 0;
 		INIT_LIST_HEAD(&drive->list);
+		sema_init(&drive->gendev_rel_sem, 0);
 	}
 }
 
@@ -749,6 +752,7 @@
 		spin_unlock_irq(&ide_lock);
 		blk_cleanup_queue(drive->queue);
 		device_unregister(&drive->gendev);
+		down(&drive->gendev_rel_sem);
 		spin_lock_irq(&ide_lock);
 		drive->queue = NULL;
 	}
@@ -778,6 +782,7 @@
 	/* More messed up locking ... */
 	spin_unlock_irq(&ide_lock);
 	device_unregister(&hwif->gendev);
+	down(&hwif->gendev_rel_sem);
 
 	/*
 	 * Remove us from the kernel's knowledge
===== include/linux/ide.h 1.67 vs edited =====
--- 1.67/include/linux/ide.h	Wed Aug 20 18:01:03 2003
+++ edited/include/linux/ide.h	Sun Aug 24 15:01:22 2003
@@ -22,6 +22,7 @@
 #include <asm/system.h>
 #include <asm/hdreg.h>
 #include <asm/io.h>
+#include <asm/semaphore.h>
 
 #define DEBUG_PM
 
@@ -774,6 +775,7 @@
 	int		crc_count;	/* crc counter to reduce drive speed */
 	struct list_head list;
 	struct device	gendev;
+	struct semaphore gendev_rel_sem;	/* to deal with device release() */
 	struct gendisk *disk;
 } ide_drive_t;
 
@@ -1040,6 +1042,7 @@
 	unsigned	auto_poll  : 1; /* supports nop auto-poll */
 
 	struct device	gendev;
+	struct semaphore gendev_rel_sem; /* To deal with device release() */
 
 	void		*hwif_data;	/* extra hwif data */
 

Re: [PATCH] Fix ide unregister vs. driver model

[Bartlomiej Zolnierkiewicz]

On Sunday 24 of August 2003 15:05, Benjamin Herrenschmidt wrote:
> Hi Bart !

Hi,

> This patch seem to have been lost, so here it is again. It fixes
> an Ooops on unregistering hwifs due to the device model now having
> mandatory release() functions. It also close the possible race we
> had on release if the entry was in use (by or /sys typically) by
> using a semaphore waiting for the release() to be called after
> doing an unregister.

I can't see the race - all references to struct device should be dropped
by driver model before finally calling ->release() function...

<...>

--bartlomiej

Re: [PATCH] Fix ide unregister vs. driver model

[Benjamin Herrenschmidt]

On Mon, 2003-08-25 at 00:23, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 24 of August 2003 15:05, Benjamin Herrenschmidt wrote:
> > Hi Bart !
> 
> Hi,
> 
> > This patch seem to have been lost, so here it is again. It fixes
> > an Ooops on unregistering hwifs due to the device model now having
> > mandatory release() functions. It also close the possible race we
> > had on release if the entry was in use (by or /sys typically) by
> > using a semaphore waiting for the release() to be called after
> > doing an unregister.
> 
> I can't see the race - all references to struct device should be dropped
> by driver model before finally calling ->release() function...

We have no race with the patch, that is we have no race when we wait
for the semaphore after calling unregister(). We have a race if we
don't as unregister() will drop a reference, but we may have pending
ones from sysfs still... so if we don't wait for release() to be
called, we may overwrite a struct device currently beeing used by
sysfs.

Ben.

Re: [PATCH] Fix ide unregister vs. driver model

[insecure]

On Sunday 24 August 2003 16:05, Benjamin Herrenschmidt wrote:
>  static void hwif_register (ide_hwif_t *hwif)
>  {
>  	/* register with global device tree */
>  	strlcpy(hwif->gendev.bus_id,hwif->name,BUS_ID_SIZE);
>  	hwif->gendev.driver_data = hwif;
> +	if (hwif->gendev.parent == NULL) {
>  	if (hwif->pci_dev)
>  		hwif->gendev.parent = &hwif->pci_dev->dev;
>  	else
>  		hwif->gendev.parent = NULL; /* Would like to do = &device_legacy */
> +	}

inner if() should be indented
--
vda

Re: [PATCH] Fix ide unregister vs. driver model

[Benjamin Herrenschmidt]

On Mon, 2003-08-25 at 12:01, insecure wrote:
> On Sunday 24 August 2003 16:05, Benjamin Herrenschmidt wrote:
> >  static void hwif_register (ide_hwif_t *hwif)
> >  {
> >  	/* register with global device tree */
> >  	strlcpy(hwif->gendev.bus_id,hwif->name,BUS_ID_SIZE);
> >  	hwif->gendev.driver_data = hwif;
> > +	if (hwif->gendev.parent == NULL) {
> >  	if (hwif->pci_dev)
> >  		hwif->gendev.parent = &hwif->pci_dev->dev;
> >  	else
> >  		hwif->gendev.parent = NULL; /* Would like to do = &device_legacy */
> > +	}
> 
> inner if() should be indented

Ah shit, I keep fixing that and for some reason, it keeps re-breaking...
I must have confused bitkeeper someway

Anyway, Bart, you know how to fix that ;)

Ben.

Re: [PATCH] Fix ide unregister vs. driver model

[Bartlomiej Zolnierkiewicz]

On Monday 25 of August 2003 08:34, Benjamin Herrenschmidt wrote:
> On Mon, 2003-08-25 at 00:23, Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 24 of August 2003 15:05, Benjamin Herrenschmidt wrote:
> > > Hi Bart !
> >
> > Hi,
> >
> > > This patch seem to have been lost, so here it is again. It fixes
> > > an Ooops on unregistering hwifs due to the device model now having
> > > mandatory release() functions. It also close the possible race we
> > > had on release if the entry was in use (by or /sys typically) by
> > > using a semaphore waiting for the release() to be called after
> > > doing an unregister.
> >
> > I can't see the race - all references to struct device should be dropped
> > by driver model before finally calling ->release() function...
>
> We have no race with the patch, that is we have no race when we wait
> for the semaphore after calling unregister(). We have a race if we
> don't as unregister() will drop a reference, but we may have pending
> ones from sysfs still... so if we don't wait for release() to be
> called, we may overwrite a struct device currently beeing used by
> sysfs.

Nope, I don't think struct device can be used by sysfs after execution
of device_unregister() (I've checked driver model and sysfs code).

--bartlomiej

Re: [PATCH] Fix ide unregister vs. driver model

[viro]

On Mon, Aug 25, 2003 at 10:50:27PM +0200, Bartlomiej Zolnierkiewicz wrote:
 
> Nope, I don't think struct device can be used by sysfs after execution
> of device_unregister() (I've checked driver model and sysfs code).

Yes, it can.  Have the sucker held by open file on sysfs.  Keep it open
past the device_unregister().  Now close it and watch the struct device
being modified.

Re: [PATCH] Fix ide unregister vs. driver model

[Patrick Mochel]

> > We have no race with the patch, that is we have no race when we wait
> > for the semaphore after calling unregister(). We have a race if we
> > don't as unregister() will drop a reference, but we may have pending
> > ones from sysfs still... so if we don't wait for release() to be
> > called, we may overwrite a struct device currently beeing used by
> > sysfs.
> 
> Nope, I don't think struct device can be used by sysfs after execution
> of device_unregister() (I've checked driver model and sysfs code).

It can, if the sysfs file for the device was held open while, at the same
time, the device was unregistered. You cannot, however, obtain new
references to the device.


	Pat

Re: [PATCH] Fix ide unregister vs. driver model

[Bartlomiej Zolnierkiewicz]

Ok, thanks.

On Monday 25 of August 2003 22:59, Patrick Mochel wrote:
> > > We have no race with the patch, that is we have no race when we wait
> > > for the semaphore after calling unregister(). We have a race if we
> > > don't as unregister() will drop a reference, but we may have pending
> > > ones from sysfs still... so if we don't wait for release() to be
> > > called, we may overwrite a struct device currently beeing used by
> > > sysfs.
> >
> > Nope, I don't think struct device can be used by sysfs after execution
> > of device_unregister() (I've checked driver model and sysfs code).
>
> It can, if the sysfs file for the device was held open while, at the same
> time, the device was unregistered. You cannot, however, obtain new
> references to the device.