Correct some mistakes in drivers using the scsi hotplug model

PATCH In review Matthew Wilcox 5 participants 9 messages Jul 31, 2007

Subsystems (4)

SCSI IBM ServeRAID RAID driver Ips SCSI RAID driver AHA152X SCSI driver

lore

Share & tools

lore

Matthew Wilcox

author

2007-07-31 12:55 UTC #0 lore raw

PATCH 1/1 Correct some mistakes in drivers using the scsi hotplug model

A few drivers are doing things like releasing IRQs before calling scsi_remove_host(), which can lead to some ugly error messages. Many more drivers are simply forgetting to call scsi_host_put() after they've finished tearing down their driver structures.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/scsi/NCR_D700.c +1 -0

index 3a80897..6c2d4bb 100644
@@ -357,6 +357,7 @@ NCR_D700_remove_one(struct Scsi_Host *host)
 	kfree((struct NCR_700_Host_Parameters *)host->hostdata[0]);
 	free_irq(host->irq, host);
 	release_region(host->base, 64);
+	scsi_host_put(host);
 }
 
 static int __devexit

drivers/scsi/NCR_Q720.c +1 -0

index a8bbdc2..6f03574 100644
@@ -323,6 +323,7 @@ NCR_Q720_remove_one(struct Scsi_Host *host)
 {
 	scsi_remove_host(host);
 	ncr53c8xx_release(host);
+	scsi_host_put(host);
 }
 
 static int __exit

drivers/scsi/a4000t.c +1 -0

index 0c758d1..8211225 100644
@@ -105,6 +105,7 @@ static __devexit int a4000t_device_remove(struct device *dev)
 	kfree(hostdata);
 	free_irq(host->irq, host);
 	release_mem_region(A4000T_SCSI_ADDR, 0x1000);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/aha152x.c +1 -1

index 85f2394..27acef4 100644
@@ -907,6 +907,7 @@ void aha152x_release(struct Scsi_Host *shpnt)
 	if(!shpnt)
 		return;
 
+	scsi_remove_host(shpnt);
 	if (shpnt->irq)
 		free_irq(shpnt->irq, shpnt);

@@ -920,7 +921,6 @@ void aha152x_release(struct Scsi_Host *shpnt)
 		pnp_device_detach(HOSTDATA(shpnt)->pnpdev);
 #endif
 
-	scsi_remove_host(shpnt);
 	list_del(&HOSTDATA(shpnt)->host_list);
 	scsi_host_put(shpnt);
 }

drivers/scsi/bvme6000_scsi.c +1 -0

index cac3540..4865b6a 100644
@@ -97,6 +97,7 @@ bvme6000_device_remove(struct device *dev)
 	NCR_700_release(host);
 	kfree(hostdata);
 	free_irq(host->irq, host);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/ibmmca.c +1 -0

index 4275d1b..82f116a 100644
@@ -1693,6 +1693,7 @@ static int __devexit ibmmca_remove(struct device *dev)
 	scsi_remove_host(shpnt);
 	release_region(shpnt->io_port, shpnt->n_io_port);
 	free_irq(shpnt->irq, dev);
+	scsi_host_put(shpnt);
 	return 0;
 }

drivers/scsi/ide-scsi.c +1 -1

index bb90df8..818459f 100644
@@ -760,6 +760,7 @@ static void ide_scsi_remove(ide_drive_t *drive)
 	struct ide_scsi_obj *scsi = scsihost_to_idescsi(scsihost);
 	struct gendisk *g = scsi->disk;
 
+	scsi_remove_host(scsihost);
 	ide_proc_unregister_driver(drive, scsi->driver);
 
 	ide_unregister_region(g);

@@ -768,7 +769,6 @@ static void ide_scsi_remove(ide_drive_t *drive)
 	g->private_data = NULL;
 	put_disk(g);
 
-	scsi_remove_host(scsihost);
 	ide_scsi_put(scsi);
 }

drivers/scsi/ips.c +2 -1

index 492a51b..eacd666 100644
@@ -656,6 +656,8 @@ ips_release(struct Scsi_Host *sh)
 
 	METHOD_TRACE("ips_release", 1);
 
+	scsi_remove_host(sh);
+
 	for (i = 0; i < IPS_MAX_ADAPTERS && ips_sh[i] != sh; i++) ;
 
 	if (i == IPS_MAX_ADAPTERS) {

@@ -707,7 +709,6 @@ ips_release(struct Scsi_Host *sh)
 	/* free IRQ */
 	free_irq(ha->irq, ha);
 
-	scsi_remove_host(sh);
 	scsi_host_put(sh);
 
 	ips_released_controllers++;

drivers/scsi/lasi700.c +1 -0

index 3126824..4ad3a18 100644
@@ -160,6 +160,7 @@ lasi700_driver_remove(struct parisc_device *dev)
 	free_irq(host->irq, host);
 	iounmap(hostdata->base);
 	kfree(hostdata);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/mvme16x_scsi.c +2 -1

index 1bdddad..131eb4e 100644
@@ -108,6 +108,7 @@ mvme16x_device_remove(struct device *dev)
 	struct Scsi_Host *host = dev_get_drvdata(dev);
 	struct NCR_700_Host_Parameters *hostdata = shost_priv(host);
 
+	scsi_remove_host(host);
 	/* Disable scsi chip ints */
 	{
 		volatile unsigned long v;

@@ -116,10 +117,10 @@ mvme16x_device_remove(struct device *dev)
 		v &= ~0x10;
 		out_be32(0xfff4202c, v);
 	}
-	scsi_remove_host(host);
 	NCR_700_release(host);
 	kfree(hostdata);
 	free_irq(host->irq, host);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/qlogicfas.c +1 -1

index 94baca8..1e874f1 100644
@@ -166,6 +166,7 @@ static int qlogicfas_release(struct Scsi_Host *shost)
 {
 	struct qlogicfas408_priv *priv = get_priv_by_host(shost);
 
+	scsi_remove_host(shost);
 	if (shost->irq) {
 		qlogicfas408_disable_ints(priv);	
 		free_irq(shost->irq, shost);

@@ -174,7 +175,6 @@ static int qlogicfas_release(struct Scsi_Host *shost)
 		free_dma(shost->dma_channel);
 	if (shost->io_port && shost->n_io_port)
 		release_region(shost->io_port, shost->n_io_port);
-	scsi_remove_host(shost);
 	scsi_host_put(shost);
 
 	return 0;

drivers/scsi/sim710.c +1 -0

index d63d229..cb75468 100644
@@ -165,6 +165,7 @@ sim710_device_remove(struct device *dev)
 	kfree(hostdata);
 	free_irq(host->irq, host);
 	release_region(host->base, 64);
+	scsi_host_put(host);
 	return 0;
 }

drivers/scsi/sni_53c710.c +1 -0

index 0a6b45b..cc1bcd2 100644
@@ -127,6 +127,7 @@ static int __exit snirm710_driver_remove(struct platform_device *dev)
 	free_irq(host->irq, host);
 	iounmap(hostdata->base);
 	kfree(hostdata);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/zalon.c +1 -0

index 4b5f908..7465b38 100644
@@ -174,6 +174,7 @@ static int __exit zalon_remove(struct parisc_device *dev)
 	scsi_remove_host(host);
 	ncr53c8xx_release(host);
 	free_irq(dev->irq, host);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/zorro7xx.c +1 -0

index c822deb..e3cab87 100644
@@ -157,6 +157,7 @@ static __devexit void zorro7xx_remove_one(struct zorro_dev *z)
 	NCR_700_release(host);
 	kfree(hostdata);
 	free_irq(host->irq, host);
+	scsi_host_put(host);
 	zorro_release_device(z);
 }

Signature · 3 lines

"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Jeff Garzik

2007-07-31 13:27 UTC #1 lore raw

Quoted 6 lines · Matthew Wilcox

A few drivers are doing things like releasing IRQs before calling scsi_remove_host(), which can lead to some ugly error messages. Many more drivers are simply forgetting to call scsi_host_put() after they've finished tearing down their driver structures.
Signed-off-by Matthew Wilcox <[email protected]>

ACK

BTW a diffstat with patches that touch more than one file are useful

James Bottomley

2007-07-31 14:42 UTC #2 lore raw

In reply to Matthew Wilcox

Quoted 12 lines · Matthew Wilcox

A few drivers are doing things like releasing IRQs before calling scsi_remove_host(), which can lead to some ugly error messages. Many more drivers are simply forgetting to call scsi_host_put() after they've finished tearing down their driver structures.
Signed-off-by Matthew Wilcox <[email protected]>
drivers/scsi/NCR_D700.c +0 -0
index 3a80897..6c2d4bb 100644

drivers/scsi/NCR_D700.c NCR_D700_remove_one(struct Scsi_Host *host) · line 361

Quoted 5 lines

drivers/scsi/NCR_D700.c +1 -0

@@ -357,6 +357,7 @@ NCR_D700_remove_one(struct Scsi_Host *host)
 	kfree((struct NCR_700_Host_Parameters *)host->hostdata[0]);
 	free_irq(host->irq, host);
 	release_region(host->base, 64);
+	scsi_host_put(host);
 }

Rather than changing this in every 53c700 based driver, shouldn't it just be added to NCR_700_release()?

drivers/scsi/NCR_Q720.c NCR_Q720_remove_one(struct Scsi_Host *host) · line 327

Quoted 11 lines

drivers/scsi/NCR_D700.c +0 -0

 static int __devexit

drivers/scsi/NCR_Q720.c +1 -0

index a8bbdc2..6f03574 100644
@@ -323,6 +323,7 @@ NCR_Q720_remove_one(struct Scsi_Host *host)
 {
 	scsi_remove_host(host);
 	ncr53c8xx_release(host);
+	scsi_host_put(host);
 }

Ditto for this (since zalon also has the problem).

Quoted 18 lines

drivers/scsi/NCR_Q720.c +0 -0

 static int __exit

drivers/scsi/a4000t.c +1 -0

index 0c758d1..8211225 100644
@@ -105,6 +105,7 @@ static __devexit int a4000t_device_remove(struct device *dev)
 	kfree(hostdata);
 	free_irq(host->irq, host);
 	release_mem_region(A4000T_SCSI_ADDR, 0x1000);
+	scsi_host_put(host);
 
 	return 0;
 }

drivers/scsi/aha152x.c +0 -0

index 85f2394..27acef4 100644

drivers/scsi/aha152x.c void aha152x_release(struct Scsi_Host *shpnt) · line 912

Quoted 6 lines

drivers/scsi/aha152x.c +1 -0

@@ -907,6 +907,7 @@ void aha152x_release(struct Scsi_Host *shpnt)
 	if(!shpnt)
 		return;
 
+	scsi_remove_host(shpnt);
 	if (shpnt->irq)
 		free_irq(shpnt->irq, shpnt);

and this one needs to go via Boaz ... he's currently rewriting this driver.

James

Matthew Wilcox

author

2007-07-31 14:52 UTC #3 lore raw

Quoted 2 lines · James Bottomley

Rather than changing this in every 53c700 based driver, shouldn't it just be added to NCR_700_release()?

Then I'd have to change the few drivers that weren't leaking scsi_hosts ;-) But I can, if that's what you prefer.

Quoted 8 lines

drivers/scsi/aha152x.c +0 -0

index 85f2394..27acef4 100644
@@ -907,6 +907,7 @@ void aha152x_release(struct Scsi_Host *shpnt)
 	if(!shpnt)
 		return;

Quoted 6 lines

drivers/scsi/aha152x.c void aha152x_release(struct Scsi_Host *shpnt) · line 912
Quoted 3 lines
drivers/scsi/aha152x.c +1 -0
+	scsi_remove_host(shpnt);
 	if (shpnt->irq)
 		free_irq(shpnt->irq, shpnt);
and this one needs to go via Boaz ... he's currently rewriting this driver.

OK.

Signature · 3 lines

"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Christoph Hellwig

2007-07-31 14:57 UTC #4 lore raw

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · James Bottomley
Rather than changing this in every 53c700 based driver, shouldn't it just be added to NCR_700_release()?
Then I'd have to change the few drivers that weren't leaking scsi_hosts ;-) But I can, if that's what you prefer.

I don't think it's a good idea. scsi_hhost_put should be the very last thing in the release method and all the drivers do some kind of cleanup after detaching from the generic ncr53c00 driver.

James Bottomley

2007-07-31 15:05 UTC #5 lore raw

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Matthew Wilcox
Quoted 2 lines · James Bottomley
Rather than changing this in every 53c700 based driver, shouldn't it just be added to NCR_700_release()?
Then I'd have to change the few drivers that weren't leaking scsi_hosts ;-) But I can, if that's what you prefer.
I don't think it's a good idea. scsi_hhost_put should be the very last thing in the release method and all the drivers do some kind of cleanup after detaching from the generic ncr53c00 driver.

I agree the NCR_700_release should be the last method called. However, the NCR_700_detect() API calls scsi_host_alloc/scsi_add_host() ... the API will be horribly assymetric (and thus prone to misuse) with regard to the host lifetime management if the NCR_700_release() method doesn't tidy up the host.

I can go either way, but either the alloc/add has to come out of the detect method or the put has to go into the release method.

James

Matthew Wilcox

author

2007-07-31 16:00 UTC #6 lore raw

Quoted 8 lines · James Bottomley

I agree the NCR_700_release should be the last method called. However, the NCR_700_detect() API calls scsi_host_alloc/scsi_add_host() ... the API will be horribly assymetric (and thus prone to misuse) with regard to the host lifetime management if the NCR_700_release() method doesn't tidy up the host.
I can go either way, but either the alloc/add has to come out of the detect method or the put has to go into the release method.

It's already asymmetric -- scsi_remove_host is called by the front end driver, and that's the opposite to scsi_add_host, which is done in NCR_700_detect.

Signature · 3 lines

"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

James Bottomley

2007-07-31 16:17 UTC #7 lore raw

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · James Bottomley
I agree the NCR_700_release should be the last method called. However, the NCR_700_detect() API calls scsi_host_alloc/scsi_add_host() ... the API will be horribly assymetric (and thus prone to misuse) with regard to the host lifetime management if the NCR_700_release() method doesn't tidy up the host.

Quoted 6 lines

Quoted 2 lines
I can go either way, but either the alloc/add has to come out of the detect method or the put has to go into the release method.
It's already asymmetric -- scsi_remove_host is called by the front end driver, and that's the opposite to scsi_add_host, which is done in NCR_700_detect.

In many ways that's paired with the external scsi_scan_host() ... although they're not true pairs in the model. I don't really see a reason to separate NCR_700_release() and scsi_host_put() unless there's something I'm missing?

James

Boaz Harrosh

2007-07-31 15:18 UTC #8 lore raw

In reply to Matthew Wilcox

Quoted 12 lines · Matthew Wilcox

Quoted 11 lines · James Bottomley
Rather than changing this in every 53c700 based driver, shouldn't it just be added to NCR_700_release()?
Quoted 8 lines
drivers/scsi/aha152x.c +0 -0
index 85f2394..27acef4 100644
@@ -907,6 +907,7 @@ void aha152x_release(struct Scsi_Host *shpnt)
    if(!shpnt)
            return;
 

Quoted 9 lines

Quoted 6 lines
drivers/scsi/aha152x.c void aha152x_release(struct Scsi_Host *shpnt) · line 912
Quoted 3 lines
drivers/scsi/aha152x.c +1 -0
+   scsi_remove_host(shpnt);
    if (shpnt->irq)
            free_irq(shpnt->irq, shpnt);
and this one needs to go via Boaz ... he's currently rewriting this driver.
OK.

It applies cleanly. I have not touched this area of code. But maybe it's related to Randy's last problem of device driver not unloading do to ref-count = 2. so please wait a bit with this patch and we'll test it

And Thanks maybe it's the problem we where looking for

Boaz