[PATCH] rationalize allocation and freeing of struct scsi_device

[PATCH] rationalize allocation and freeing of struct scsi_device

[Christoph Hellwig]

Currently allocation and freeing of struct scsi_device is a mess.
We have two nice functions in scsi_scan.c (scsi_allocate_sdev/
scsi_free_sdev) that are the right interfaces to deal with it, so I moved
them to scsi and made them non-static.  I've changed all functions
allocation freeing them to use it.  There's also a new helper
scsi_release_sdev that deals with getting rid of a currently
attached sdev.


--- 1.23/drivers/scsi/hosts.c	Tue Nov  5 12:18:22 2002
+++ edited/drivers/scsi/hosts.c	Fri Nov  8 20:11:00 2002
@@ -182,33 +182,13 @@
 		}
 	}
 
-	/*
-	 * Next we detach the high level drivers from the Scsi_Device
-	 * structures
-	 */
-	for (sdev = shost->host_queue; sdev; sdev = sdev->next) {
-		scsi_detach_device(sdev);
-
+	for (sdev = shost->host_queue; sdev; sdev = shost->host_queue) {
 		/* If something still attached, punt */
-		if (sdev->attached) {
+		if (scsi_release_sdev(sdev)) {
 			printk(KERN_ERR "Attached usage count = %d\n",
 			       sdev->attached);
 			return 1;
 		}
-		devfs_unregister(sdev->de);
-		device_unregister(&sdev->sdev_driverfs_dev);
-	}
-
-	/* Next we free up the Scsi_Cmnd structures for this host */
-
-	for (sdev = shost->host_queue; sdev;
-	     sdev = shost->host_queue) {
-		blk_cleanup_queue(&sdev->request_queue);
-		/* Next free up the Scsi_Device structures for this host */
-		shost->host_queue = sdev->next;
-		if (sdev->inquiry)
-			kfree(sdev->inquiry);
-		kfree(sdev);
 	}
 
 	return 0;
===== drivers/scsi/scsi.c 1.59 vs edited =====
--- 1.59/drivers/scsi/scsi.c	Wed Nov  6 21:10:15 2002
+++ edited/drivers/scsi/scsi.c	Fri Nov  8 20:11:43 2002
@@ -1926,31 +1926,9 @@
 			goto out;	/* there is no such device attached */
 
 		err = -EBUSY;
-		if (scd->access_count)
+		if (scsi_release_sdev(scd))
 			goto out;
 
-		scsi_detach_device(scd);
-
-		if (scd->attached == 0) {
-			devfs_unregister (scd->de);
-
-			/* Now we can remove the device structure */
-			if (scd->next != NULL)
-				scd->next->prev = scd->prev;
-
-			if (scd->prev != NULL)
-				scd->prev->next = scd->next;
-
-			if (HBA_ptr->host_queue == scd) {
-				HBA_ptr->host_queue = scd->next;
-			}
-			blk_cleanup_queue(&scd->request_queue);
-			if (scd->inquiry)
-				kfree(scd->inquiry);
-			kfree((char *) scd);
-		} else {
-			goto out;
-		}
 		err = 0;
 	}
 out:
@@ -1960,6 +1938,138 @@
 }
 #endif
 
+/**
+ * scsi_initialize_merge_fn() -Æ£initialize merge function for a host
+ * @sd:		host descriptor
+ */
+static void scsi_initialize_merge_fn(struct scsi_device *sd)
+{
+	request_queue_t *q = &sd->request_queue;
+	struct Scsi_Host *sh = sd->host;
+	u64 bounce_limit;
+
+	if (sh->highmem_io) {
+		if (sh->pci_dev && PCI_DMA_BUS_IS_PHYS) {
+			bounce_limit = sh->pci_dev->dma_mask;
+		} else {
+			/*
+			 * Platforms with virtual-DMA translation
+ 			 * hardware have no practical limit.
+			 */
+			bounce_limit = BLK_BOUNCE_ANY;
+		}
+	} else if (sh->unchecked_isa_dma) {
+		bounce_limit = BLK_BOUNCE_ISA;
+	} else {
+		bounce_limit = BLK_BOUNCE_HIGH;
+	}
+
+	blk_queue_bounce_limit(q, bounce_limit);
+}
+
+/**
+ * scsi_alloc_sdev - allocate and setup a Scsi_Device
+ *
+ * Description:
+ *     Allocate, initialize for io, and return a pointer to a Scsi_Device.
+ *     Stores the @shost, @channel, @id, and @lun in the Scsi_Device, and
+ *     adds Scsi_Device to the appropriate list.
+ *
+ * Return value:
+ *     Scsi_Device pointer, or NULL on failure.
+ **/
+struct scsi_device *scsi_alloc_sdev(struct Scsi_Host *shost,
+				    uint channel, uint id, uint lun)
+{
+	struct scsi_device *sdev;
+
+	sdev = kmalloc(sizeof(Scsi_Device), GFP_ATOMIC);
+	if (!sdev) {
+		printk(KERN_ERR "%s: Allocation failure during SCSI "
+				"scanning, some SCSI devices might not "
+				"be configured\n", __FUNCTION__);
+		return NULL;
+	}
+
+	memset(sdev, 0, sizeof(Scsi_Device));
+	sdev->vendor = scsi_null_device_strs;
+	sdev->model = scsi_null_device_strs;
+	sdev->rev = scsi_null_device_strs;
+	sdev->host = shost;
+	sdev->id = id;
+	sdev->lun = lun;
+	sdev->channel = channel;
+	sdev->online = TRUE;
+
+	/*
+	 * Some low level driver could use device->type
+	 */
+	sdev->type = -1;
+
+	/*
+	 * Assume that the device will have handshaking problems,
+	 * and then fix this field later if it turns out it
+	 * doesn't
+	 */
+	sdev->borken = 1;
+	scsi_initialize_queue(sdev, shost);
+	sdev->request_queue.queuedata = sdev;
+
+	scsi_initialize_merge_fn(sdev);
+	init_waitqueue_head(&sdev->scpnt_wait);
+
+	/*
+	 * Add it to the end of the shost->host_queue list.
+	 */
+	if (shost->host_queue != NULL) {
+		sdev->prev = shost->host_queue;
+		while (sdev->prev->next != NULL)
+			sdev->prev = sdev->prev->next;
+		sdev->prev->next = sdev;
+	} else
+		shost->host_queue = sdev;
+
+	return sdev;
+}
+
+/**
+ * scsi_free_sdev - cleanup and free a Scsi_Device
+ * @sdev:	cleanup and free this Scsi_Device
+ *
+ * Description:
+ *	Undo the actions in scsi_alloc_sdev, including removing @sdev from
+ *	the list, and freeing @sdev.
+ **/
+void scsi_free_sdev(struct scsi_device *sdev)
+{
+	if (sdev->next)
+		sdev->next->prev = sdev->prev;
+	if (sdev->prev)
+		sdev->prev->next = sdev->next;
+	else
+		sdev->host->host_queue = sdev->next;
+
+	blk_cleanup_queue(&sdev->request_queue);
+	kfree(sdev->inquiry);
+	kfree(sdev);
+}
+
+int scsi_release_sdev(struct scsi_device *sdev)
+{
+	if (sdev->access_count)
+		return -EBUSY;
+
+	scsi_detach_device(sdev);
+	if (sdev->attached)
+		return -EBUSY;
+
+	devfs_unregister(sdev->de);
+	device_unregister(&sdev->sdev_driverfs_dev);
+
+	scsi_free_sdev(sdev);
+	return 0;
+}
+
 int scsi_attach_device(struct scsi_device *sdev)
 {
 	struct Scsi_Device_Template *sdt;
@@ -2429,58 +2539,40 @@
  * Returns:     The Scsi_Device or NULL
  *
  * Notes:
+ *	Attach a single Scsi_Device to the Scsi_Host - this should
+ *	be made to look like a "pseudo-device" that points to the
+ *	HA itself.  For the moment, we include it at the head of
+ *	the host_queue itself - I don't think we want to show this
+ *	to the HA in select_queue_depths(), as this would probably confuse
+ *	matters.
+ *
+ *	Note - this device is not accessible from any high-level
+ *	drivers (including generics), which is probably not
+ *	optimal.  We can add hooks later to attach 
  */
-Scsi_Device * scsi_get_host_dev(struct Scsi_Host * SHpnt)
+struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
 {
-        Scsi_Device * SDpnt;
+	struct scsi_device *sdev;
 
-        /*
-         * Attach a single Scsi_Device to the Scsi_Host - this should
-         * be made to look like a "pseudo-device" that points to the
-         * HA itself.  For the moment, we include it at the head of
-         * the host_queue itself - I don't think we want to show this
-         * to the HA in select_queue_depths(), as this would probably confuse
-         * matters.
-         * Note - this device is not accessible from any high-level
-         * drivers (including generics), which is probably not
-         * optimal.  We can add hooks later to attach 
-         */
-        SDpnt = (Scsi_Device *) kmalloc(sizeof(Scsi_Device),
-                                        GFP_ATOMIC);
-        if(SDpnt == NULL)
-        	return NULL;
-        	
-        memset(SDpnt, 0, sizeof(Scsi_Device));
-	SDpnt->vendor = scsi_null_device_strs;
-	SDpnt->model = scsi_null_device_strs;
-	SDpnt->rev = scsi_null_device_strs;
-
-        SDpnt->host = SHpnt;
-        SDpnt->id = SHpnt->this_id;
-        SDpnt->type = -1;
-	SDpnt->new_queue_depth = 1;
-        
-	scsi_build_commandblocks(SDpnt);
-	if(SDpnt->current_queue_depth == 0) {
-		kfree(SDpnt);
-		return NULL;
+	sdev = scsi_alloc_sdev(shost, 0, shost->this_id, 0);
+	if (sdev) {
+		scsi_build_commandblocks(sdev);
+		if (sdev->current_queue_depth == 0)
+			goto fail;
+		sdev->borken = 0;
 	}
 
-	scsi_initialize_queue(SDpnt, SHpnt);
+	return sdev;
 
-	SDpnt->online = TRUE;
-
-        /*
-         * Initialize the object that we will use to wait for command blocks.
-         */
-	init_waitqueue_head(&SDpnt->scpnt_wait);
-        return SDpnt;
+fail:
+	kfree(sdev);
+	return NULL;
 }
 
 /*
  * Function:    scsi_free_host_dev()
  *
- * Purpose:     Create a Scsi_Device that points to the host adapter itself.
+ * Purpose:     Free a scsi_device that points to the host adapter itself.
  *
  * Arguments:   SHpnt   - Host that needs a Scsi_Device
  *
@@ -2490,23 +2582,10 @@
  *
  * Notes:
  */
-void scsi_free_host_dev(Scsi_Device * SDpnt)
+void scsi_free_host_dev(struct scsi_device *sdev)
 {
-        if( (unsigned char) SDpnt->id != (unsigned char) SDpnt->host->this_id )
-        {
-                panic("Attempt to delete wrong device\n");
-        }
-
-        blk_cleanup_queue(&SDpnt->request_queue);
-
-        /*
-         * We only have a single SCpnt attached to this device.  Free
-         * it now.
-         */
-	scsi_release_commandblocks(SDpnt);
-	if (SDpnt->inquiry)
-		kfree(SDpnt->inquiry);
-        kfree(SDpnt);
+	BUG_ON(sdev->id != sdev->host->this_id);
+	scsi_free_sdev(sdev);
 }
 
 /*
===== drivers/scsi/scsi.h 1.36 vs edited =====
--- 1.36/drivers/scsi/scsi.h	Wed Nov  6 21:08:24 2002
+++ edited/drivers/scsi/scsi.h	Fri Nov  8 20:11:52 2002
@@ -397,6 +397,8 @@
 typedef struct scsi_cmnd Scsi_Cmnd;
 typedef struct scsi_request Scsi_Request;
 
+struct Scsi_Host;
+
 #define SCSI_CMND_MAGIC 0xE25C23A5
 #define SCSI_REQ_MAGIC  0x75F6D354
 
@@ -483,6 +485,10 @@
 extern int scsi_mlqueue_insert(struct scsi_cmnd *, int);
 extern int scsi_attach_device(struct scsi_device *);
 extern void scsi_detach_device(struct scsi_device *);
+extern struct scsi_device *scsi_alloc_sdev(struct Scsi_Host *,
+			uint, uint, uint);
+extern void scsi_free_sdev(struct scsi_device *);
+extern int scsi_release_sdev(struct scsi_device *);
 
 /*
  * Newer request-based interfaces.
===== drivers/scsi/scsi_scan.c 1.34 vs edited =====
--- 1.34/drivers/scsi/scsi_scan.c	Wed Nov  6 21:08:41 2002
+++ edited/drivers/scsi/scsi_scan.c	Fri Nov  8 04:57:45 2002
@@ -210,8 +210,6 @@
 #define SCSI_SCAN_TARGET_PRESENT	1
 #define SCSI_SCAN_LUN_PRESENT		2
 
-static char *scsi_null_device_strs = "nullnullnullnull";
-
 #define MAX_SCSI_LUNS	512
 
 #ifdef CONFIG_SCSI_MULTI_LUN
@@ -480,118 +478,6 @@
 		return device_list[i].flags;
 	}
 	return 0;
-}
-
-/**
- * scsi_initialize_merge_fn() -Æ£initialize merge function for a host
- * @sd:		host descriptor
- */
-static void scsi_initialize_merge_fn(struct scsi_device *sd)
-{
-	request_queue_t *q = &sd->request_queue;
-	struct Scsi_Host *sh = sd->host;
-	u64 bounce_limit;
-
-	if (sh->highmem_io) {
-		if (sh->pci_dev && PCI_DMA_BUS_IS_PHYS) {
-			bounce_limit = sh->pci_dev->dma_mask;
-		} else {
-			/*
-			 * Platforms with virtual-DMA translation
- 			 * hardware have no practical limit.
-			 */
-			bounce_limit = BLK_BOUNCE_ANY;
-		}
-	} else if (sh->unchecked_isa_dma) {
-		bounce_limit = BLK_BOUNCE_ISA;
-	} else {
-		bounce_limit = BLK_BOUNCE_HIGH;
-	}
-
-	blk_queue_bounce_limit(q, bounce_limit);
-}
-
-/**
- * scsi_alloc_sdev - allocate and setup a Scsi_Device
- *
- * Description:
- *     Allocate, initialize for io, and return a pointer to a Scsi_Device.
- *     Stores the @shost, @channel, @id, and @lun in the Scsi_Device, and
- *     adds Scsi_Device to the appropriate list.
- *
- * Return value:
- *     Scsi_Device pointer, or NULL on failure.
- **/
-static Scsi_Device *scsi_alloc_sdev(struct Scsi_Host *shost, uint channel,
-				    uint id, uint lun)
-{
-	Scsi_Device *sdev;
-
-	sdev = (Scsi_Device *) kmalloc(sizeof(Scsi_Device), GFP_ATOMIC);
-	if (sdev == NULL)
-		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
-	else {
-		memset(sdev, 0, sizeof(Scsi_Device));
-		sdev->vendor = scsi_null_device_strs;
-		sdev->model = scsi_null_device_strs;
-		sdev->rev = scsi_null_device_strs;
-		sdev->host = shost;
-		sdev->id = id;
-		sdev->lun = lun;
-		sdev->channel = channel;
-		sdev->online = TRUE;
-		/*
-		 * Some low level driver could use device->type
-		 */
-		sdev->type = -1;
-		/*
-		 * Assume that the device will have handshaking problems,
-		 * and then fix this field later if it turns out it
-		 * doesn't
-		 */
-		sdev->borken = 1;
-		scsi_initialize_queue(sdev, shost);
-		sdev->request_queue.queuedata = (void *) sdev;
-
-		scsi_initialize_merge_fn(sdev);
-		init_waitqueue_head(&sdev->scpnt_wait);
-
-		/*
-		 * Add it to the end of the shost->host_queue list.
-		 */
-		if (shost->host_queue != NULL) {
-			sdev->prev = shost->host_queue;
-			while (sdev->prev->next != NULL)
-				sdev->prev = sdev->prev->next;
-			sdev->prev->next = sdev;
-		} else
-			shost->host_queue = sdev;
-
-	}
-	return (sdev);
-}
-
-/**
- * scsi_free_sdev - cleanup and free a Scsi_Device
- * @sdev:	cleanup and free this Scsi_Device
- *
- * Description:
- *     Undo the actions in scsi_alloc_sdev, including removing @sdev from
- *     the list, and freeing @sdev.
- **/
-static void scsi_free_sdev(Scsi_Device *sdev)
-{
-	if (sdev->prev != NULL)
-		sdev->prev->next = sdev->next;
-	else
-		sdev->host->host_queue = sdev->next;
-	if (sdev->next != NULL)
-		sdev->next->prev = sdev->prev;
-
-	blk_cleanup_queue(&sdev->request_queue);
-	if (sdev->inquiry != NULL)
-		kfree(sdev->inquiry);
-	kfree(sdev);
 }
 
 /**
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rationalize allocation and freeing of struct scsi_device

[Christoph Hellwig]

Currently allocation and freeing of struct scsi_device is a mess.
We have two nice functions in scsi_scan.c (scsi_allocate_sdev/
scsi_free_sdev) that are the right interfaces to deal with it, so I moved
them to scsi and made them non-static.  I've changed all functions
allocation freeing them to use it.


--- 1.29/drivers/scsi/hosts.c	Sat Nov 16 20:26:02 2002
+++ edited/drivers/scsi/hosts.c	Sun Nov 17 21:47:02 2002
@ -289,17 +289,8 @@
 		device_unregister(&sdev->sdev_driverfs_dev);
 	}
 
-	/* Next we free up the Scsi_Cmnd structures for this host */
-
-	for (sdev = shost->host_queue; sdev;
-	     sdev = shost->host_queue) {
-		blk_cleanup_queue(&sdev->request_queue);
-		/* Next free up the Scsi_Device structures for this host */
-		shost->host_queue = sdev->next;
-		if (sdev->inquiry)
-			kfree(sdev->inquiry);
-		kfree(sdev);
-	}
+	for (sdev = shost->host_queue; sdev; sdev = shost->host_queue)
+		scsi_free_sdev(sdev);
 
 	return 0;
 }
@@ -677,6 +675,67 @@
 		if (shost_hn)
 			shost_hn++;
 	}
+}
+
+/*
+ * Function:    scsi_get_host_dev()
+ *
+ * Purpose:     Create a Scsi_Device that points to the host adapter itself.
+ *
+ * Arguments:   SHpnt   - Host that needs a Scsi_Device
+ *
+ * Lock status: None assumed.
+ *
+ * Returns:     The Scsi_Device or NULL
+ *
+ * Notes:
+ *	Attach a single Scsi_Device to the Scsi_Host - this should
+ *	be made to look like a "pseudo-device" that points to the
+ *	HA itself.  For the moment, we include it at the head of
+ *	the host_queue itself - I don't think we want to show this
+ *	to the HA in select_queue_depths(), as this would probably confuse
+ *	matters.
+ *
+ *	Note - this device is not accessible from any high-level
+ *	drivers (including generics), which is probably not
+ *	optimal.  We can add hooks later to attach 
+ */
+struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
+{
+	struct scsi_device *sdev;
+
+	sdev = scsi_alloc_sdev(shost, 0, shost->this_id, 0);
+	if (sdev) {
+		scsi_build_commandblocks(sdev);
+		if (sdev->current_queue_depth == 0)
+			goto fail;
+		sdev->borken = 0;
+	}
+
+	return sdev;
+
+fail:
+	kfree(sdev);
+	return NULL;
+}
+
+/*
+ * Function:    scsi_free_host_dev()
+ *
+ * Purpose:     Free a scsi_device that points to the host adapter itself.
+ *
+ * Arguments:   SHpnt   - Host that needs a Scsi_Device
+ *
+ * Lock status: None assumed.
+ *
+ * Returns:     Nothing
+ *
+ * Notes:
+ */
+void scsi_free_host_dev(struct scsi_device *sdev)
+{
+	BUG_ON(sdev->id != sdev->host->this_id);
+	scsi_free_sdev(sdev);
 }
 
 void scsi_host_busy_inc(struct Scsi_Host *shost, Scsi_Device *sdev)
--- 1.63/drivers/scsi/scsi.c	Sat Nov 16 14:53:49 2002
+++ edited/drivers/scsi/scsi.c	Sun Nov 17 22:39:03 2002
@@ -155,7 +155,6 @@
 	"Enclosure        ",
 };
 
-static char * scsi_null_device_strs = "nullnullnullnull";
 static const char * const spaces = "                "; /* 16 of them */
 
 static unsigned scsi_default_dev_flags;
@@ -2266,95 +2265,3 @@
 
 module_init(init_scsi);
 module_exit(exit_scsi);
-
-/*
- * Function:    scsi_get_host_dev()
- *
- * Purpose:     Create a Scsi_Device that points to the host adapter itself.
- *
- * Arguments:   SHpnt   - Host that needs a Scsi_Device
- *
- * Lock status: None assumed.
- *
- * Returns:     The Scsi_Device or NULL
- *
- * Notes:
- */
-Scsi_Device * scsi_get_host_dev(struct Scsi_Host * SHpnt)
-{
-        Scsi_Device * SDpnt;
-
-        /*
-         * Attach a single Scsi_Device to the Scsi_Host - this should
-         * be made to look like a "pseudo-device" that points to the
-         * HA itself.  For the moment, we include it at the head of
-         * the host_queue itself - I don't think we want to show this
-         * to the HA in select_queue_depths(), as this would probably confuse
-         * matters.
-         * Note - this device is not accessible from any high-level
-         * drivers (including generics), which is probably not
-         * optimal.  We can add hooks later to attach 
-         */
-        SDpnt = (Scsi_Device *) kmalloc(sizeof(Scsi_Device),
-                                        GFP_ATOMIC);
-        if(SDpnt == NULL)
-        	return NULL;
-        	
-        memset(SDpnt, 0, sizeof(Scsi_Device));
-	SDpnt->vendor = scsi_null_device_strs;
-	SDpnt->model = scsi_null_device_strs;
-	SDpnt->rev = scsi_null_device_strs;
-
-        SDpnt->host = SHpnt;
-        SDpnt->id = SHpnt->this_id;
-        SDpnt->type = -1;
-	SDpnt->new_queue_depth = 1;
-        
-	scsi_build_commandblocks(SDpnt);
-	if(SDpnt->current_queue_depth == 0) {
-		kfree(SDpnt);
-		return NULL;
-	}
-
-	scsi_initialize_queue(SDpnt, SHpnt);
-
-	SDpnt->online = TRUE;
-
-        /*
-         * Initialize the object that we will use to wait for command blocks.
-         */
-	init_waitqueue_head(&SDpnt->scpnt_wait);
-        return SDpnt;
-}
-
-/*
- * Function:    scsi_free_host_dev()
- *
- * Purpose:     Create a Scsi_Device that points to the host adapter itself.
- *
- * Arguments:   SHpnt   - Host that needs a Scsi_Device
- *
- * Lock status: None assumed.
- *
- * Returns:     Nothing
- *
- * Notes:
- */
-void scsi_free_host_dev(Scsi_Device * SDpnt)
-{
-        if( (unsigned char) SDpnt->id != (unsigned char) SDpnt->host->this_id )
-        {
-                panic("Attempt to delete wrong device\n");
-        }
-
-        blk_cleanup_queue(&SDpnt->request_queue);
-
-        /*
-         * We only have a single SCpnt attached to this device.  Free
-         * it now.
-         */
-	scsi_release_commandblocks(SDpnt);
-	if (SDpnt->inquiry)
-		kfree(SDpnt->inquiry);
-        kfree(SDpnt);
-}
===== drivers/scsi/scsi.h 1.41 vs edited =====
--- 1.41/drivers/scsi/scsi.h	Sat Nov 16 20:26:02 2002
+++ edited/drivers/scsi/scsi.h	Sun Nov 17 21:44:35 2002
@@ -516,6 +516,13 @@
 static inline void scsi_proc_host_add(struct Scsi_Host *);
 static inline void scsi_proc_host_rm(struct Scsi_Host *);
 #endif /* CONFIG_PROC_FS */
+
+/*
+ * Prototypes for functions in scsi_scan.c
+ */
+extern struct scsi_device *scsi_alloc_sdev(struct Scsi_Host *,
+			uint, uint, uint);
+extern void scsi_free_sdev(struct scsi_device *);
 
 /*
  * Prototypes for functions in constants.c
--- 1.8/drivers/scsi/scsi_proc.c	Sat Nov 16 20:26:02 2002
+++ edited/drivers/scsi/scsi_proc.c	Sun Nov 17 21:49:06 2002
@@ -607,25 +607,9 @@
 
 		if (sdev->attached == 0) {
 			devfs_unregister (sdev->de);
-
-			/* Now we can remove the device structure */
-			if (sdev->next != NULL)
-				sdev->next->prev = sdev->prev;
-
-			if (sdev->prev != NULL)
-				sdev->prev->next = sdev->next;
-
-			if (shost->host_queue == sdev) {
-				shost->host_queue = sdev->next;
-			}
-			blk_cleanup_queue(&sdev->request_queue);
-			if (sdev->inquiry)
-				kfree(sdev->inquiry);
-			kfree((char *) sdev);
-		} else {
-			goto out;
+			scsi_free_sdev(sdev);
+			err = 0;
 		}
-		err = 0;
 	}
 out:
 	
--- 1.36/drivers/scsi/scsi_scan.c	Sat Nov 16 18:56:15 2002
+++ edited/drivers/scsi/scsi_scan.c	Sun Nov 17 21:47:20 2002
@@ -465,12 +465,12 @@
  * Return value:
  *     Scsi_Device pointer, or NULL on failure.
  **/
-static Scsi_Device *scsi_alloc_sdev(struct Scsi_Host *shost, uint channel,
+struct scsi_device *scsi_alloc_sdev(struct Scsi_Host *shost, uint channel,
 				    uint id, uint lun)
 {
-	Scsi_Device *sdev;
+	struct scsi_device *sdev;
 
-	sdev = (Scsi_Device *) kmalloc(sizeof(Scsi_Device), GFP_ATOMIC);
+	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
 	if (sdev == NULL)
 		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 	else {
@@ -522,7 +522,7 @@
  *     Undo the actions in scsi_alloc_sdev, including removing @sdev from
  *     the list, and freeing @sdev.
  **/
-static void scsi_free_sdev(Scsi_Device *sdev)
+void scsi_free_sdev(struct scsi_device *sdev)
 {
 	if (sdev->prev != NULL)
 		sdev->prev->next = sdev->next;

Re: [PATCH] rationalize allocation and freeing of struct scsi_device

[Patrick Mansfield]

On Sun, Nov 17, 2002 at 11:49:01PM +0100, Christoph Hellwig wrote:
> Currently allocation and freeing of struct scsi_device is a mess.
> We have two nice functions in scsi_scan.c (scsi_allocate_sdev/
> scsi_free_sdev) that are the right interfaces to deal with it, so I moved
> them to scsi and made them non-static.  I've changed all functions
> allocation freeing them to use it.

What happened to the scsi_release_sdev that was in your previous version
of the patch?

That was a nice place to call device_unregister(), and would be the right
place to remove sysfs scsi_device files.

-- Patrick Mansfield

Re: [PATCH] rationalize allocation and freeing of struct scsi_device

[Christoph Hellwig]

On Mon, Nov 18, 2002 at 10:24:10AM -0800, Patrick Mansfield wrote:
> On Sun, Nov 17, 2002 at 11:49:01PM +0100, Christoph Hellwig wrote:
> > Currently allocation and freeing of struct scsi_device is a mess.
> > We have two nice functions in scsi_scan.c (scsi_allocate_sdev/
> > scsi_free_sdev) that are the right interfaces to deal with it, so I moved
> > them to scsi and made them non-static.  I've changed all functions
> > allocation freeing them to use it.
> 
> What happened to the scsi_release_sdev that was in your previous version
> of the patch?

That will follow in another patch. 

> That was a nice place to call device_unregister(), and would be the right
> place to remove sysfs scsi_device files.

Right.  But I also want to add such a wrapper for the alloc path, too
before submitting it.