Skip to contentSkip to conversation

Fix ide unregister vs. driver model

PATCH In review Benjamin Herrenschmidt 5 participants 9 messages Aug 24, 2003 – Aug 25, 2003

lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/1 Fix ide unregister vs. driver model

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 =====

drivers/ide/ide-probe.c +18 -0
@@ -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 =====

drivers/ide/ide.c +5 -0
@@ -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 =====

include/linux/ide.h +3 -0
@@ -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 */
#1 lore raw
Quoted 1 line · Benjamin Herrenschmidt

Hi Bart !

Hi,

Quoted 6 lines

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

author
#2 lore raw
Quoted 12 lines · Bartlomiej Zolnierkiewicz
Quoted 1 line · Benjamin Herrenschmidt

Hi Bart !

Hi,

Quoted 6 lines

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.

Quoted 2 lines

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.

#3 lore raw
Quoted 16 lines · Benjamin Herrenschmidt
Quoted 12 lines · Bartlomiej Zolnierkiewicz
Quoted 1 line · Benjamin Herrenschmidt

Hi Bart !

Hi,

Quoted 6 lines

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.

Quoted 2 lines

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

Quoted 6 lines

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

#4 lore raw
Quoted 10 lines
Quoted 7 lines

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

#5 lore raw

Ok, thanks.

Quoted 13 lines · Patrick Mochel
Quoted 9 lines
Quoted 6 lines

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.

#6 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 2 lines · Bartlomiej Zolnierkiewicz

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.

#7 lore raw
In reply to Benjamin Herrenschmidt

drivers/ide/ide-probe.c line 664

Quoted 11 lines · Benjamin Herrenschmidt

static void hwif_register (ide_hwif_t *hwif)
{

drivers/ide/ide-probe.c +2 -0
 	/* 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

author
#8 lore raw
Quoted 14 lines · insecure

drivers/ide/ide-probe.c line 664

Quoted 11 lines · Benjamin Herrenschmidt

static void hwif_register (ide_hwif_t *hwif)
{

drivers/ide/ide-probe.c +2 -0
 	/* 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.