Skip to contentSkip to conversation

rationalize allocation and freeing of struct scsi_device

PATCH In review Christoph Hellwig 2 participants 3 messages Nov 17, 2002 – Nov 18, 2002 SCSI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rationalize allocation and freeing of struct scsi_device

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)

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

drivers/scsi/scsi.h +7 -0
@@ -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
drivers/scsi/scsi_proc.c +2 -18
@@ -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:
 	
drivers/scsi/scsi_scan.c +4 -4
@@ -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;
#1 lore raw
Quoted 5 lines · 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.

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

#2 lore raw
Quoted 9 lines · Patrick Mansfield
Quoted 5 lines · 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.

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

That will follow in another patch.

Quoted 2 lines

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.