scsi_add_device/scsi_remove_device oops

scsi_add_device/scsi_remove_device oops

[Brian King]

I am experiencing an oops when calling both scsi_add_device and
scsi_remove_device at the same time. Is the API supposed to be able to
handle this? I can patch around the exact oops I encountered, but it
looks like there would be more problems in this code path as well.
If I am not following the API, I can try putting some more intelligence 
in ipr so that I don't try to delete a device that hasn't fully been 
added, but I want to make sure I am fixing this the proper way.

Looking at the backtraces, it looks like slave_configure has already 
been called on the device and we are constructing the classdev and 
trying to destroy it at the same time.

Here are the backtraces:

0xc00000033fecb5c0  0xc000000000043098  .do_page_fault +0x43c
0xc00000033fecb6f0  0xc00000000000ad28  DataAccessSLB_common +0x114
--- Exception: 380:  at .sysfs_hash_and_remove +0x2c
0xc00000033fecb9e0  0xc00000000011bc4c  .sysfs_hash_and_remove +0x2c
0xc00000033fecb9e0  0xc00000000011d644 (lr) .sysfs_remove_link +0x14
0xc00000033fecba70  0xc00000000011d644  .sysfs_remove_link +0x14
0xc00000033fecbaf0  0xc00000000024d978  .class_device_del +0x1ec
0xc00000033fecbba0  0xc00000000024d9d4  .class_device_unregister +0x1c
0xc00000033fecbc30  0xd000000000095818  .scsi_remove_device +0x54
0xc00000033fecbcb0  0xd0000000000e422c  .ipr_worker_thread +0x960
0xc00000033fecbdb0  0xc0000000000746ec  .worker_thread +0x24c
0xc00000033fecbed0  0xc00000000007a584  .kthread +0x17c
0xc00000033fecbf90  0xc000000000017924  .kernel_thread +0x4c

and

0xc0000000028e7330  0xc00000000005841c  .schedule +0xbc
0xc0000000028e7450  0xc00000000005985c  .wait_for_completion +0xec
0xc0000000028e7550  0xd000000000092738  .scsi_wait_req +0x88
0xc0000000028e7600  0xd0000000000574b0  .sd_revalidate_disk +0x160
0xc0000000028e7750  0xd000000000058b6c  .sd_probe +0x238
0xc0000000028e7800  0xc00000000024c060  .bus_match +0x94
0xc0000000028e7890  0xc00000000024c130  .device_attach +0x6c
0xc0000000028e7920  0xc00000000024c2c4  .bus_add_device +0x98
0xc0000000028e79b0  0xc00000000024a710  .device_add +0xd0
0xc0000000028e7a50  0xd0000000000959b8  .scsi_sysfs_add_sdev +0x8c
0xc0000000028e7b00  0xd000000000093c54  .scsi_probe_and_add_lun +0x87c
0xc0000000028e7c00  0xd000000000094adc  .scsi_add_device +0x78
0xc0000000028e7cb0  0xd0000000000e42c0  .ipr_worker_thread +0x9f4
0xc0000000028e7db0  0xc0000000000746ec  .worker_thread +0x24c
0xc0000000028e7ed0  0xc00000000007a584  .kthread +0x17c
0xc0000000028e7f90  0xc000000000017924  .kernel_thread +0x4c


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[PATCH] scsi_remove_device locking

[Brian King]

[-- Attachment #1: Type: text/plain, Size: 64 bytes --]


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[-- Attachment #2: scsi_remove_device_locking.patch --]
[-- Type: text/plain, Size: 1467 bytes --]


The following patch fixes an oops I was seeing on a machine with
misconfigured scsi cables, but could feasibly happen in other paths.
The oops was occurring because scsi_remove_device was getting
called for a device before scsi_add_device had fully completed.
This resulted in sysfs_remove_link being called with a NULL dentry.

Signed-off-by: Brian King <brking@us.ibm.com>
---

 linux-2.6.7-bjking1/drivers/scsi/scsi_sysfs.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/scsi_sysfs.c~scsi_remove_device_locking drivers/scsi/scsi_sysfs.c
--- linux-2.6.7/drivers/scsi/scsi_sysfs.c~scsi_remove_device_locking	2004-07-02 11:34:25.000000000 -0500
+++ linux-2.6.7-bjking1/drivers/scsi/scsi_sysfs.c	2004-07-02 11:37:00.000000000 -0500
@@ -524,8 +524,13 @@ int scsi_sysfs_add_sdev(struct scsi_devi
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+	struct Scsi_Host *shost = sdev->host;
+	down(&shost->scan_mutex);
+
+	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0) {
+		up(&shost->scan_mutex);
 		return;
+	}
 
 	class_device_unregister(&sdev->sdev_classdev);
 	if (sdev->transport_classdev.class)
@@ -537,6 +542,7 @@ void scsi_remove_device(struct scsi_devi
 	if (sdev->host->transportt->cleanup)
 		sdev->host->transportt->cleanup(sdev);
 	put_device(&sdev->sdev_gendev);
+	up(&shost->scan_mutex);
 }
 
 int scsi_register_driver(struct device_driver *drv)
_

Re: [PATCH] scsi_remove_device locking

[Matthew Wilcox]

On Fri, Jul 02, 2004 at 03:18:50PM -0500, Brian King wrote:
>  {
> -	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> +	struct Scsi_Host *shost = sdev->host;
> +	down(&shost->scan_mutex);
> +
> +	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0) {
> +		up(&shost->scan_mutex);
>  		return;
> +	}
>  
>  	class_device_unregister(&sdev->sdev_classdev);
>  	if (sdev->transport_classdev.class)
> @@ -537,6 +542,7 @@ void scsi_remove_device(struct scsi_devi
>  	if (sdev->host->transportt->cleanup)
>  		sdev->host->transportt->cleanup(sdev);
>  	put_device(&sdev->sdev_gendev);
> +	up(&shost->scan_mutex);
>  }

IMO, this would be better done as:

{
	struct Scsi_Host *shost = sdev->host;
	down(&shost->scan_mutex);

	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
		goto out;

...
 out:
	up(&shost->scan_mutex);
}


-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] scsi_remove_device locking

[Brian King]

[-- Attachment #1: Type: text/plain, Size: 116 bytes --]

Here is an updated patch with Matthew's suggestion.

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[-- Attachment #2: scsi_remove_device_locking.patch --]
[-- Type: text/plain, Size: 1402 bytes --]


The following patch fixes an oops I was seeing on a machine with
misconfigured scsi cables, but could feasibly happen in other paths.
The oops was occurring because scsi_remove_device was getting
called for a device before scsi_add_device had fully completed.
This resulted in sysfs_remove_link being called with a NULL dentry.

Signed-off-by: Brian King <brking@us.ibm.com>
---

 linux-2.6.7-bjking1/drivers/scsi/scsi_sysfs.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/scsi_sysfs.c~scsi_remove_device_locking drivers/scsi/scsi_sysfs.c
--- linux-2.6.7/drivers/scsi/scsi_sysfs.c~scsi_remove_device_locking	2004-07-02 11:34:25.000000000 -0500
+++ linux-2.6.7-bjking1/drivers/scsi/scsi_sysfs.c	2004-07-02 15:29:59.000000000 -0500
@@ -524,8 +524,11 @@ int scsi_sysfs_add_sdev(struct scsi_devi
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
+	struct Scsi_Host *shost = sdev->host;
+
+	down(&shost->scan_mutex);
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-		return;
+		goto out;
 
 	class_device_unregister(&sdev->sdev_classdev);
 	if (sdev->transport_classdev.class)
@@ -537,6 +540,9 @@ void scsi_remove_device(struct scsi_devi
 	if (sdev->host->transportt->cleanup)
 		sdev->host->transportt->cleanup(sdev);
 	put_device(&sdev->sdev_gendev);
+
+out:
+	up(&shost->scan_mutex);
 }
 
 int scsi_register_driver(struct device_driver *drv)
_