[PATCH 1/4 ver 3] SCSI: add construction flags to scsi_device structure

[PATCH 1/4 ver 3] SCSI: add construction flags to scsi_device structure

[Alan Stern]

This patch (as1250c) adds extra flags to the scsi_device structure, to
keep track of what parts have been constructed.  These flags are used
in the destructor routines to determine which fields need to be torn
down.  This allows all the slave_destroy() and
transport_destroy_device() calls to be moved to a single place in
scsi_device_dev_release_user_context() instead of appearing in
multiple locations throughout the source.

This also simplifies the destructors.  There's no need for a separate
scsi_destroy_sdev() routine; __scsi_remove_device() can handle
everything.  The error paths are more robust because now it's easy to
verify that all the destructors are called along every pathway.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/include/scsi/scsi_device.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_device.h
+++ usb-2.6/include/scsi/scsi_device.h
@@ -146,6 +146,13 @@ struct scsi_device {
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
 
+			/* Keep track of how much has been constructed */
+	unsigned did_slave_alloc:1;
+	unsigned did_device_add:1;
+	unsigned did_add_sdev_dev:1;
+	unsigned did_bsg_register_queue:1;
+	unsigned did_transport_add_device:1;
+
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
 	struct work_struct event_work;
Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -311,13 +311,12 @@ static struct scsi_device *scsi_alloc_sd
 			goto out_device_destroy;
 		}
 	}
+	sdev->did_slave_alloc = 1;
 
 	return sdev;
 
 out_device_destroy:
-	scsi_device_set_state(sdev, SDEV_DEL);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
+	__scsi_remove_device(sdev);
 out:
 	if (display_failure_msg)
 		printk(ALLOC_FAILURE_MSG, __func__);
@@ -951,15 +950,6 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
-static inline void scsi_destroy_sdev(struct scsi_device *sdev)
-{
-	scsi_device_set_state(sdev, SDEV_DEL);
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
-}
-
 #ifdef CONFIG_SCSI_LOGGING
 /** 
  * scsi_inq_str - print INQUIRY data from min to max index, strip trailing whitespace
@@ -1137,7 +1127,7 @@ static int scsi_probe_and_add_lun(struct
 			}
 		}
 	} else
-		scsi_destroy_sdev(sdev);
+		__scsi_remove_device(sdev);
  out:
 	return res;
 }
@@ -1498,7 +1488,7 @@ static int scsi_report_lun_scan(struct s
 		/*
 		 * the sdev we used didn't appear in the report luns scan
 		 */
-		scsi_destroy_sdev(sdev);
+		__scsi_remove_device(sdev);
 	return ret;
 }
 
@@ -1708,7 +1698,7 @@ static void scsi_sysfs_add_devices(struc
 	shost_for_each_device(sdev, shost) {
 		if (!scsi_host_scan_allowed(shost) ||
 		    scsi_sysfs_add_sdev(sdev) != 0)
-			scsi_destroy_sdev(sdev);
+			__scsi_remove_device(sdev);
 	}
 }
 
@@ -1941,7 +1931,7 @@ void scsi_free_host_dev(struct scsi_devi
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
 
-	scsi_destroy_sdev(sdev);
+	scsi_remove_device(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
 
Index: usb-2.6/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_sysfs.c
+++ usb-2.6/drivers/scsi/scsi_sysfs.c
@@ -318,6 +318,11 @@ static void scsi_device_dev_release_user
 		kfree(evt);
 	}
 
+	if (sdev->did_slave_alloc && sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+
+	transport_destroy_device(&sdev->sdev_gendev);
+
 	if (sdev->request_queue) {
 		sdev->request_queue->queuedata = NULL;
 		/* user context needed to free queue */
@@ -858,11 +863,15 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 		printk(KERN_INFO "error 1\n");
 		return error;
 	}
+	sdev->did_device_add = 1;
+
 	error = device_add(&sdev->sdev_dev);
 	if (error) {
 		printk(KERN_INFO "error 2\n");
-		goto clean_device;
+		__scsi_remove_device(sdev);
+		return error;
 	}
+	sdev->did_add_sdev_dev = 1;
 
 	/* take a reference for the sdev_dev; this is
 	 * released by the sdev_class .release */
@@ -895,6 +904,7 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	/* we're treating error on bsg register as non-fatal, so pretend
 	 * nothing went wrong */
 	error = 0;
+	sdev->did_bsg_register_queue = 1;
 
 	/* add additional host specific attributes */
 	if (sdev->host->hostt->sdev_attrs) {
@@ -909,17 +919,9 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	}
 
 	transport_add_device(&sdev->sdev_gendev);
+	sdev->did_transport_add_device = 1;
  out:
 	return error;
-
- clean_device:
-	scsi_device_set_state(sdev, SDEV_CANCEL);
-
-	device_del(&sdev->sdev_gendev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
-
-	return error;
 }
 
 void __scsi_remove_device(struct scsi_device *sdev)
@@ -929,14 +931,15 @@ void __scsi_remove_device(struct scsi_de
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
 		return;
 
-	bsg_unregister_queue(sdev->request_queue);
-	device_unregister(&sdev->sdev_dev);
-	transport_remove_device(dev);
-	device_del(dev);
+	if (sdev->did_bsg_register_queue)
+		bsg_unregister_queue(sdev->request_queue);
+	if (sdev->did_add_sdev_dev)
+		device_unregister(&sdev->sdev_dev);
+	if (sdev->did_transport_add_device)
+		transport_remove_device(dev);
+	if (sdev->did_device_add)
+		device_del(dev);
 	scsi_device_set_state(sdev, SDEV_DEL);
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(dev);
 	put_device(dev);
 }
 

Re: [PATCH 1/4 ver 3] SCSI: add construction flags to scsi_device structure

[James Bottomley]

On Wed, 2009-10-28 at 15:09 -0400, Alan Stern wrote:
> This patch (as1250c) adds extra flags to the scsi_device structure, to
> keep track of what parts have been constructed.  These flags are used
> in the destructor routines to determine which fields need to be torn
> down.  This allows all the slave_destroy() and
> transport_destroy_device() calls to be moved to a single place in
> scsi_device_dev_release_user_context() instead of appearing in
> multiple locations throughout the source.
> 
> This also simplifies the destructors.  There's no need for a separate
> scsi_destroy_sdev() routine; __scsi_remove_device() can handle
> everything.  The error paths are more robust because now it's easy to
> verify that all the destructors are called along every pathway.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

The diffstats really say this is a wash on simplification:

 drivers/scsi/scsi_scan.c   |   22 ++++++----------------
 drivers/scsi/scsi_sysfs.c  |   37 ++++++++++++++++++++-----------------
 include/scsi/scsi_device.h |    7 +++++++
 3 files changed, 33 insertions(+), 33 deletions(-)

Your 5 constructor flags are what makes this so complex ... plus it now
doesn't actually work with the memory leak fix commit:

commit 37e6ba00720c2786330dec2a9a5081e9e049422f
Author: James Bottomley <James.Bottomley@suse.de>
Date:   Fri Oct 2 13:30:08 2009 -0500

    [SCSI] fix memory leak in initialization
 
I think you can make this much simpler just by having one visible flag
(i.e. was scsi_sysfs_sdev_add() called or not) and making everything
unwind back to either the alloc or the add state on failure.  Like the
attached.  It also cleanly fixes the USB scan crash in an easily
backportable manner.

James

---

 drivers/scsi/scsi_scan.c   |   18 +++----------
 drivers/scsi/scsi_sysfs.c  |   63 +++++++++++++++++++-------------------------
 include/scsi/scsi_device.h |    1 +
 3 files changed, 32 insertions(+), 50 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index a0e0632..ded615d 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -955,16 +955,6 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
-static inline void scsi_destroy_sdev(struct scsi_device *sdev)
-{
-	scsi_device_set_state(sdev, SDEV_DEL);
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_dev);
-	put_device(&sdev->sdev_gendev);
-}
-
 #ifdef CONFIG_SCSI_LOGGING
 /** 
  * scsi_inq_str - print INQUIRY data from min to max index, strip trailing whitespace
@@ -1142,7 +1132,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 			}
 		}
 	} else
-		scsi_destroy_sdev(sdev);
+		__scsi_remove_device(sdev);
  out:
 	return res;
 }
@@ -1503,7 +1493,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		/*
 		 * the sdev we used didn't appear in the report luns scan
 		 */
-		scsi_destroy_sdev(sdev);
+		__scsi_remove_device(sdev);
 	return ret;
 }
 
@@ -1713,7 +1703,7 @@ static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
 	shost_for_each_device(sdev, shost) {
 		if (!scsi_host_scan_allowed(shost) ||
 		    scsi_sysfs_add_sdev(sdev) != 0)
-			scsi_destroy_sdev(sdev);
+			__scsi_remove_device(sdev);
 	}
 }
 
@@ -1946,7 +1936,7 @@ void scsi_free_host_dev(struct scsi_device *sdev)
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
 
-	scsi_destroy_sdev(sdev);
+	__scsi_remove_device(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 758598f..5a06505 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -888,15 +888,17 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	transport_configure_device(&starget->dev);
 	error = device_add(&sdev->sdev_gendev);
 	if (error) {
-		put_device(sdev->sdev_gendev.parent);
 		printk(KERN_INFO "error 1\n");
-		return error;
+		goto out_remove;
 	}
 	error = device_add(&sdev->sdev_dev);
 	if (error) {
 		printk(KERN_INFO "error 2\n");
-		goto clean_device;
+		device_del(&sdev->sdev_gendev);
+		goto out_remove;
 	}
+	transport_add_device(&sdev->sdev_gendev);
+	sdev->is_visible = 1;
 
 	/* create queue files, which may be writable, depending on the host */
 	if (sdev->host->hostt->change_queue_depth) {
@@ -907,67 +909,56 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	}
 	else
 		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_depth);
-	if (error) {
-		__scsi_remove_device(sdev);
-		goto out;
-	}
+	if (error)
+		goto out_remove;
+
 	if (sdev->host->hostt->change_queue_type)
 		error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_type_rw);
 	else
 		error = device_create_file(&sdev->sdev_gendev, &dev_attr_queue_type);
-	if (error) {
-		__scsi_remove_device(sdev);
-		goto out;
-	}
+	if (error)
+		goto out_remove;
 
 	error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
 
 	if (error)
+		/* we're treating error on bsg register as non-fatal,
+		 * so pretend nothing went wrong */
 		sdev_printk(KERN_INFO, sdev,
 			    "Failed to register bsg queue, errno=%d\n", error);
 
-	/* we're treating error on bsg register as non-fatal, so pretend
-	 * nothing went wrong */
-	error = 0;
-
 	/* add additional host specific attributes */
 	if (sdev->host->hostt->sdev_attrs) {
 		for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
 			error = device_create_file(&sdev->sdev_gendev,
 					sdev->host->hostt->sdev_attrs[i]);
-			if (error) {
-				__scsi_remove_device(sdev);
-				goto out;
-			}
+			if (error)
+				goto out_remove;
 		}
 	}
 
-	transport_add_device(&sdev->sdev_gendev);
- out:
-	return error;
-
- clean_device:
-	scsi_device_set_state(sdev, SDEV_CANCEL);
-
-	device_del(&sdev->sdev_gendev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_dev);
-	put_device(&sdev->sdev_gendev);
+	return 0;
 
+ out_remove:
+	__scsi_remove_device(sdev);
 	return error;
+
 }
 
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
 
-	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-		return;
+	if (sdev->is_visible) {
+		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+			return;
 
-	bsg_unregister_queue(sdev->request_queue);
-	device_unregister(&sdev->sdev_dev);
-	transport_remove_device(dev);
-	device_del(dev);
+		bsg_unregister_queue(sdev->request_queue);
+		device_unregister(&sdev->sdev_dev);
+		transport_remove_device(dev);
+		device_del(dev);
+	} else
+		put_device(&sdev->sdev_dev);
 	scsi_device_set_state(sdev, SDEV_DEL);
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 68d185c..7c44499 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -148,6 +148,7 @@ struct scsi_device {
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
+	unsigned is_visible:1;	/* is the device visible in sysfs */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Re: [PATCH 1/4 ver 3] SCSI: add construction flags to scsi_device structure

[Alan Stern]

On Thu, 19 Nov 2009, James Bottomley wrote:

> Your 5 constructor flags are what makes this so complex ... plus it now
> doesn't actually work with the memory leak fix commit:
> 
> commit 37e6ba00720c2786330dec2a9a5081e9e049422f
> Author: James Bottomley <James.Bottomley@suse.de>
> Date:   Fri Oct 2 13:30:08 2009 -0500
> 
>     [SCSI] fix memory leak in initialization

Yes, okay.  I'll rebase on top of that patch.

> I think you can make this much simpler just by having one visible flag
> (i.e. was scsi_sysfs_sdev_add() called or not) and making everything
> unwind back to either the alloc or the add state on failure.  Like the
> attached.  It also cleanly fixes the USB scan crash in an easily
> backportable manner.

That makes sense.  But there's still a little awkwardness caused by the
fact that the creation and destruction paths aren't clean mirror
images.  In broad outline and ignoring the state transitions, we have:

scsi_alloc_sdev():
	kzalloc and init sdev
	get_device(&starget->dev)
	scsi_alloc_queue()
	scsi_sysfs_device_initialize():
		device_initialize(&sdev->sdev_gendev)
		device_initialize(&sdev->sdev_dev)
		get_device(&sdev->sdev_gendev) for sdev_dev
		transport_setup_device()
		add sdev to host's and target's lists
	shost->hostt->slave_alloc()

scsi_add_lun():
	transport_configure_device()
	sdev->host->hostt->slave_configure()
	scsi_sysfs_add_sdev():
		device_add(&sdev->sdev_gendev)
		device_add(&sdev->sdev_dev)
		transport_add_device()
		bsg_register_queue()
		add extra host attributes

The removal routines look like this:

__scsi_remove_device():
	bsg_unregister_queue()
	device_unregister(&sdev->sdev_dev)
	transport_remove_device()
	device_del(&sdev->sdev_gendev)
	sdev->host->hostt->slave_destroy()
	transport_destroy_device()
	put_device(&sdev->sdev_gendev)

scsi_device_cls_release():
	put_device(&sdev->sdev_gendev)

scsi_device_dev_release_usercontext():
	remove sdev from host's and target's lists
	scsi_free_queue()
	kfree(sdev)
	put_device(&starget->dev)

Sorry for the large amount of detail.  But this does reveal some
oddities.

     1. __scsi_remove_device() is awkward because it encompasses both
	removal from visibility (its first four lines above) and
	deinitialization (the last three lines).  Adding the
	is_visible flag helps clarify this, of course.

     2. bsg_unregister_queue() is called even when
	bsg_register_queue() fails.  Is this legitimate?

     3. transport_setup_device() and adding sdev to the host's and 
	target's lists don't have anything to do with sysfs.  Hence
	they don't really belong in scsi_sysfs_device_initialize().
	Is there any reason not to move them up into scsi_alloc_lun()?

     4. bsg_register_queue() comes before transport_add_device() but
	bug_unregister_queue() doesn't come after
	transport_remove_device().  Maybe the order doesn't matter and
	maybe it does; but it isn't commented and it looks unclean.
	Should the second pair be reversed?

     5. scsi_sysfs_device_initialize() can't fail.  Hence it should
	come after the slave_alloc() call, which can, in order to
	simplify unwinding.  Unless there is an ordering restriction
	which prevents this?

Would it be okay to take your patch and modify it in accordance with
points 3 - 5?

Alan Stern

Re: [PATCH 1/4 ver 3] SCSI: add construction flags to scsi_device structure

[James Bottomley]

On Fri, 2009-11-20 at 15:55 -0500, Alan Stern wrote:
> On Thu, 19 Nov 2009, James Bottomley wrote:
> 
> > Your 5 constructor flags are what makes this so complex ... plus it now
> > doesn't actually work with the memory leak fix commit:
> > 
> > commit 37e6ba00720c2786330dec2a9a5081e9e049422f
> > Author: James Bottomley <James.Bottomley@suse.de>
> > Date:   Fri Oct 2 13:30:08 2009 -0500
> > 
> >     [SCSI] fix memory leak in initialization
> 
> Yes, okay.  I'll rebase on top of that patch.
> 
> > I think you can make this much simpler just by having one visible flag
> > (i.e. was scsi_sysfs_sdev_add() called or not) and making everything
> > unwind back to either the alloc or the add state on failure.  Like the
> > attached.  It also cleanly fixes the USB scan crash in an easily
> > backportable manner.
> 
> That makes sense.  But there's still a little awkwardness caused by the
> fact that the creation and destruction paths aren't clean mirror
> images.  In broad outline and ignoring the state transitions, we have:
> 
> scsi_alloc_sdev():
> 	kzalloc and init sdev
> 	get_device(&starget->dev)
> 	scsi_alloc_queue()
> 	scsi_sysfs_device_initialize():
> 		device_initialize(&sdev->sdev_gendev)
> 		device_initialize(&sdev->sdev_dev)
> 		get_device(&sdev->sdev_gendev) for sdev_dev
> 		transport_setup_device()
> 		add sdev to host's and target's lists
> 	shost->hostt->slave_alloc()
> 
> scsi_add_lun():
> 	transport_configure_device()
> 	sdev->host->hostt->slave_configure()
> 	scsi_sysfs_add_sdev():
> 		device_add(&sdev->sdev_gendev)
> 		device_add(&sdev->sdev_dev)
> 		transport_add_device()
> 		bsg_register_queue()
> 		add extra host attributes
> 
> The removal routines look like this:
> 
> __scsi_remove_device():
> 	bsg_unregister_queue()
> 	device_unregister(&sdev->sdev_dev)
> 	transport_remove_device()
> 	device_del(&sdev->sdev_gendev)
> 	sdev->host->hostt->slave_destroy()
> 	transport_destroy_device()
> 	put_device(&sdev->sdev_gendev)
> 
> scsi_device_cls_release():
> 	put_device(&sdev->sdev_gendev)
> 
> scsi_device_dev_release_usercontext():
> 	remove sdev from host's and target's lists
> 	scsi_free_queue()
> 	kfree(sdev)
> 	put_device(&starget->dev)
> 
> Sorry for the large amount of detail.  But this does reveal some
> oddities.
> 
>      1. __scsi_remove_device() is awkward because it encompasses both
> 	removal from visibility (its first four lines above) and
> 	deinitialization (the last three lines).  Adding the
> 	is_visible flag helps clarify this, of course.

Right, the point I was looking at is what are the persistent states.
Pretty much that's running not visible and running visible ... most
other transactions can simply be wound backwards or forwards to those
points.

>      2. bsg_unregister_queue() is called even when
> 	bsg_register_queue() fails.  Is this legitimate?

Yes, there's a check in bsg_unregister for the initial registration.

>      3. transport_setup_device() and adding sdev to the host's and 
> 	target's lists don't have anything to do with sysfs.  Hence
> 	they don't really belong in scsi_sysfs_device_initialize().
> 	Is there any reason not to move them up into scsi_alloc_lun()?

For a patch that needs to fix a bug, yes ... it needs to be the minimum
change.  But on the broader principle, no, it doesn't matter.  The
scsi_alloc_sdev() must contain the list adds because at some point if
the scan mutex is removed, the act of checking the lists and allocating
and adding them will have to become atomic so they have to be somwhere
in scsi_alloc_sdev() ... but it might not matter where.

>      4. bsg_register_queue() comes before transport_add_device() but
> 	bug_unregister_queue() doesn't come after
> 	transport_remove_device().  Maybe the order doesn't matter and
> 	maybe it does; but it isn't commented and it looks unclean.
> 	Should the second pair be reversed?

The ordering shouldn't matter.  The bsg registration is an independent
operation.

>      5. scsi_sysfs_device_initialize() can't fail.  Hence it should
> 	come after the slave_alloc() call, which can, in order to
> 	simplify unwinding.  Unless there is an ordering restriction
> 	which prevents this?

Well, we'd have to unwind what was done on slave alloc failure if you do
that ... so it just adds more to the unwind path.

> Would it be okay to take your patch and modify it in accordance with
> points 3 - 5?

I really just need the minimum patch that can be backported, so if you
can shorten it, go for it.

James

Re: [PATCH 1/4 ver 3] SCSI: add construction flags to scsi_device structure

[Alan Stern]

On Fri, 20 Nov 2009, James Bottomley wrote:

> >      3. transport_setup_device() and adding sdev to the host's and 
> > 	target's lists don't have anything to do with sysfs.  Hence
> > 	they don't really belong in scsi_sysfs_device_initialize().
> > 	Is there any reason not to move them up into scsi_alloc_lun()?
> 
> For a patch that needs to fix a bug, yes ... it needs to be the minimum
> change.  But on the broader principle, no, it doesn't matter.  The
> scsi_alloc_sdev() must contain the list adds because at some point if
> the scan mutex is removed, the act of checking the lists and allocating
> and adding them will have to become atomic so they have to be somwhere
> in scsi_alloc_sdev() ... but it might not matter where.
> 
> >      4. bsg_register_queue() comes before transport_add_device() but
> > 	bug_unregister_queue() doesn't come after
> > 	transport_remove_device().  Maybe the order doesn't matter and
> > 	maybe it does; but it isn't commented and it looks unclean.
> > 	Should the second pair be reversed?
> 
> The ordering shouldn't matter.  The bsg registration is an independent
> operation.
> 
> >      5. scsi_sysfs_device_initialize() can't fail.  Hence it should
> > 	come after the slave_alloc() call, which can, in order to
> > 	simplify unwinding.  Unless there is an ordering restriction
> > 	which prevents this?
> 
> Well, we'd have to unwind what was done on slave alloc failure if you do
> that ... so it just adds more to the unwind path.

You misunderstood...  In what I'm suggesting, if the slave_alloc fails
there will be less to unwind, because scsi_sysfs_device_initialize()
won't have run yet.  See the patch below, which applies on top of
yours.

> > Would it be okay to take your patch and modify it in accordance with
> > points 3 - 5?
> 
> I really just need the minimum patch that can be backported, so if you
> can shorten it, go for it.

Tell you what: Go ahead and merge your patch.  Add in the one below if
you want, but it isn't necessary.  Then I'll write the suggested
changes and submit them for 2.6.33-rc1.

Alan Stern


Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -286,19 +286,12 @@ static struct scsi_device *scsi_alloc_sd
 	sdev->borken = 1;
 
 	sdev->request_queue = scsi_alloc_queue(sdev);
-	if (!sdev->request_queue) {
-		/* release fn is set up in scsi_sysfs_device_initialise, so
-		 * have to free and put manually here */
-		put_device(&starget->dev);
-		kfree(sdev);
-		goto out;
-	}
+	if (!sdev->request_queue)
+		goto out_put_target;
 
 	sdev->request_queue->queuedata = sdev;
 	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
 
-	scsi_sysfs_device_initialize(sdev);
-
 	if (shost->hostt->slave_alloc) {
 		ret = shost->hostt->slave_alloc(sdev);
 		if (ret) {
@@ -308,18 +301,19 @@ static struct scsi_device *scsi_alloc_sd
 			 */
 			if (ret == -ENXIO)
 				display_failure_msg = 0;
-			goto out_device_destroy;
+			goto out_free_queue;
 		}
 	}
 
+	scsi_sysfs_device_initialize(sdev);
 	return sdev;
 
-out_device_destroy:
-	scsi_device_set_state(sdev, SDEV_DEL);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_dev);
-	put_device(&sdev->sdev_gendev);
-out:
+ out_free_queue:
+	scsi_free_queue(sdev->request_queue);
+ out_put_target:
+	put_device(&starget->dev);
+	kfree(sdev);
+ out:
 	if (display_failure_msg)
 		printk(ALLOC_FAILURE_MSG, __func__);
 	return NULL;