Skip to contentSkip to conversation

Correct order for device unregistration in aic7xxx

PATCH In review Hannes Reinecke 2 participants 2 messages May 24, 2005

Subsystems (2)
SCSI AIC7XXX / AIC79XX SCSI driver
lore
Share & tools
lore

Hannes Reinecke

author
#0 lore raw

PATCH 1/1 Correct order for device unregistration in aic7xxx

Hi James,

calling 'rmmod aic79xx' (with my previous patch) results in an interesting internal state dump of the adapter itself. Problem is that at the time of calling sd_shutdown() the dv_thread for this adapter is already stopped so that no command processing will be done. This confuses the adapter resulting in the state dump.

Instead we should unregister any disks first and then shutting down the adapter.

Please apply.

Btw, any plans to convert aic79xx to spi_transport?

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke			[email protected]
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

From: Hannes Reinecke <[email protected]>
Subject: Correct order for device unregistration in aic7xxx

When issuing a 'rmmod aic79xx' the driver crashes with a rather nasty internal state dump.
Problem is that all attached devices are shutdown during a call to ahc_platform_free after the dv_thread is already deleted. This disables processing of any SCSI commands which any ULD might want to send (eg SYNCHRONIZE CACHE). So any commands sent to the adaper will not be processed but result in the state dump mentioned above.

Device unregistration from scsi-ml should be handled before the adapter itself is disabled to allow for any commands to be handled properly.

drivers/scsi/aic7xxx/aic7xxx_osm.c renamed from drivers/scsi/aic7xxx/aic7xxx_osm.c.orig +14 -6
@@ -1748,12 +1748,6 @@ ahc_platform_free(struct ahc_softc *ahc)
 	if (ahc->platform_data != NULL) {
 		del_timer_sync(&ahc->platform_data->completeq_timer);
 		tasklet_kill(&ahc->platform_data->runq_tasklet);
-		if (ahc->platform_data->host != NULL) {
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
-			scsi_remove_host(ahc->platform_data->host);
-#endif
-			scsi_host_put(ahc->platform_data->host);
-		}
 
 		/* destroy all of the device and target objects */
 		for (i = 0; i < AHC_NUM_TARGETS; i++) {

@@ -3623,6 +3617,20 @@ ahc_linux_init(void)
 static void
 ahc_linux_exit(void)
 {
+	struct ahd_softc *ahc;
+
+	/*
+	 * Tear down midlayer first
+	 */
+	TAILQ_FOREACH(ahc, &ahd_tailq, links) {
+		if (ahc->platform_data->host != NULL) {
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
+			scsi_remove_host(ahc->platform_data->host);
+#endif
+			scsi_host_put(ahc->platform_data->host);
+		}
+	}
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
 	/*
 	 * In 2.4 we have to unregister from the PCI core _after_
drivers/scsi/aic7xxx/aic79xx_osm.c renamed from drivers/scsi/aic7xxx/aic79xx_osm.c.orig +7 -12
@@ -2191,12 +2191,6 @@ ahd_platform_free(struct ahd_softc *ahd)
 		del_timer_sync(&ahd->platform_data->completeq_timer);
 		ahd_linux_kill_dv_thread(ahd);
 		ahd_teardown_runq_tasklet(ahd);
-		if (ahd->platform_data->host != NULL) {
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
-			scsi_remove_host(ahd->platform_data->host);
-#endif
-			scsi_host_put(ahd->platform_data->host);
-		}
 
 		/* destroy all of the device and target objects */
 		for (i = 0; i < AHD_NUM_TARGETS; i++) {

@@ -4992,14 +4986,15 @@ ahd_linux_exit(void)
 	struct ahd_softc *ahd;
 
 	/*
-	 * Shutdown DV threads before going into the SCSI mid-layer.
-	 * This avoids situations where the mid-layer locks the entire
-	 * kernel so that waiting for our DV threads to exit leads
-	 * to deadlock.
+	 * Tear down midlayer first
 	 */
 	TAILQ_FOREACH(ahd, &ahd_tailq, links) {
-
-		ahd_linux_kill_dv_thread(ahd);
+		if (ahd->platform_data->host != NULL) {
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
+			scsi_remove_host(ahd->platform_data->host);
+#endif
+			scsi_host_put(ahd->platform_data->host);
+		}
 	}
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
#1 lore raw
Quoted 23 lines · Hannes Reinecke
drivers/scsi/aic7xxx/aic7xxx_osm.c renamed from drivers/scsi/aic7xxx/aic7xxx_osm.c.orig +4 -6
@@ -1748,12 +1748,6 @@ ahc_platform_free(struct ahc_softc *ahc)
 	if (ahc->platform_data != NULL) {
 		del_timer_sync(&ahc->platform_data->completeq_timer);
 		tasklet_kill(&ahc->platform_data->runq_tasklet);
-		if (ahc->platform_data->host != NULL) {
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
-			scsi_remove_host(ahc->platform_data->host);
-#endif
-			scsi_host_put(ahc->platform_data->host);
-		}
 
 		/* destroy all of the device and target objects */
 		for (i = 0; i < AHC_NUM_TARGETS; i++) {

@@ -3623,6 +3617,20 @@ ahc_linux_init(void)
 static void
 ahc_linux_exit(void)
 {
+	struct ahd_softc *ahc;
+
+	/*
+	 * Tear down midlayer first

drivers/scsi/aic7xxx/aic7xxx_osm.c ahc_linux_init(void) · line 3633

Quoted 10 lines
drivers/scsi/aic7xxx/aic7xxx_osm.c renamed from drivers/scsi/aic7xxx/aic7xxx_osm.c.orig +10 -0
+	 */
+	TAILQ_FOREACH(ahc, &ahd_tailq, links) {
+		if (ahc->platform_data->host != NULL) {
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
+			scsi_remove_host(ahc->platform_data->host);
+#endif
+			scsi_host_put(ahc->platform_data->host);
+		}
+	}
+

Sorrym this is not acceptable at all. Please leave the host unregistration in ->remove (and while we're at it the scsi_host_put happens far too early in there) and move tearing down the DV thread later in ->remove.

You're current patch completely breaks hot removal. Also please look at aic7xxx in current Linus' tree - it's using the SPI transport class and doesn't have it's own dv mechanisms anymore, only aic79xx needs fixes or preferably someone porting over the recent aic7xxx changes.