[PATCH 1/1] scsi: Add added_to_sysfs sdev flag to fix device scanning oops

[PATCH 1/1] scsi: Add added_to_sysfs sdev flag to fix device scanning oops

[Brian King]

The following patch fixes an oops which was observed during device scanning.
If LUN 0 does not exist for a valid target, but non-zero LUNs do exist, a struct
scsi_device exists only when probing the target and is deleted by the scanning
code, by checking to see if the device's state is SDEV_CREATED. If, while scanning,
the rport is deleted, such that target is placed into the blocked state, and
then later the rport is added again, the LUN 0 sdev finds its way into the SDEV_RUNNING
state. This causes the scanning code to NOT delete the sdev when scanning is complete.
Later on, if the target is deleted, we will oops when we try to free up everything
that gets allocated in scsi_sysfs_add_sdev, since it was never added. This patch
fixes this issue, by adding a added_to_sysfs flag to struct scsi_device, which is
then checked to determine what to do, both in the device probe exit code and the
device remove code.

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

 drivers/scsi/scsi_scan.c   |    4 ++--
 drivers/scsi/scsi_sysfs.c  |   11 +++++++----
 include/scsi/scsi_device.h |    1 +
 3 files changed, 10 insertions(+), 6 deletions(-)

diff -puN include/scsi/scsi_device.h~scsi_add_sysfs_flag include/scsi/scsi_device.h
--- linux-2.6/include/scsi/scsi_device.h~scsi_add_sysfs_flag	2008-08-11 10:49:06.000000000 -0500
+++ linux-2.6-bjking1/include/scsi/scsi_device.h	2008-08-11 10:49:06.000000000 -0500
@@ -142,6 +142,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 added_to_sysfs:1;	/* Device added to sysfs */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
diff -puN drivers/scsi/scsi_sysfs.c~scsi_add_sysfs_flag drivers/scsi/scsi_sysfs.c
--- linux-2.6/drivers/scsi/scsi_sysfs.c~scsi_add_sysfs_flag	2008-08-11 10:49:06.000000000 -0500
+++ linux-2.6-bjking1/drivers/scsi/scsi_sysfs.c	2008-08-11 10:49:06.000000000 -0500
@@ -922,6 +922,7 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	}
 
 	transport_add_device(&sdev->sdev_gendev);
+	sdev->added_to_sysfs = 1;
  out:
 	return error;
 
@@ -942,10 +943,12 @@ 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->added_to_sysfs) {
+		bsg_unregister_queue(sdev->request_queue);
+		device_unregister(&sdev->sdev_dev);
+		transport_remove_device(dev);
+		device_del(dev);
+	}
 	scsi_device_set_state(sdev, SDEV_DEL);
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
diff -puN drivers/scsi/scsi_scan.c~scsi_add_sysfs_flag drivers/scsi/scsi_scan.c
--- linux-2.6/drivers/scsi/scsi_scan.c~scsi_add_sysfs_flag	2008-08-11 10:49:06.000000000 -0500
+++ linux-2.6-bjking1/drivers/scsi/scsi_scan.c	2008-08-11 10:49:06.000000000 -0500
@@ -994,7 +994,7 @@ static int scsi_probe_and_add_lun(struct
 	 */
 	sdev = scsi_device_lookup_by_target(starget, lun);
 	if (sdev) {
-		if (rescan || sdev->sdev_state != SDEV_CREATED) {
+		if (rescan || sdev->added_to_sysfs) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -1466,7 +1466,7 @@ static int scsi_report_lun_scan(struct s
 	kfree(lun_data);
  out:
 	scsi_device_put(sdev);
-	if (sdev->sdev_state == SDEV_CREATED)
+	if (!sdev->added_to_sysfs)
 		/*
 		 * the sdev we used didn't appear in the report luns scan
 		 */
_

Re: [PATCH 1/1] scsi: Add added_to_sysfs sdev flag to fix device scanning oops

[James Bottomley]

On Mon, 2008-08-11 at 13:03 -0500, Brian King wrote:
> The following patch fixes an oops which was observed during device scanning.
> If LUN 0 does not exist for a valid target, but non-zero LUNs do exist, a struct
> scsi_device exists only when probing the target and is deleted by the scanning
> code, by checking to see if the device's state is SDEV_CREATED. If, while scanning,
> the rport is deleted, such that target is placed into the blocked state, and
> then later the rport is added again, the LUN 0 sdev finds its way into the SDEV_RUNNING
> state. This causes the scanning code to NOT delete the sdev when scanning is complete.
> Later on, if the target is deleted, we will oops when we try to free up everything
> that gets allocated in scsi_sysfs_add_sdev, since it was never added. This patch
> fixes this issue, by adding a added_to_sysfs flag to struct scsi_device, which is
> then checked to determine what to do, both in the device probe exit code and the
> device remove code.

What this seems to be exposing is a bug in the state model around the
blocked state.

The transition:

CREATED -> BLOCK -> RUNNING

shouldn't be legal.  My initial reaction is just to forbid the CREATED
-> BLOCK transition, but it looks like the fc transport code never
checks return values from scsi_target_block() (sigh!)

So an alternate fix should be to correct the state model rather than try
and work around the deficiencies with additional flags.

It looks like, to allow the CREATED -> BLOCK -> CREATED transition we
need an extra state (CREATED_BLOCK) and we forbid the CREATED -> BLOCK
in favour of it.

The model also now allows an online transition to do CREATED_BLOCK ->
BLOCK

This is a rough code of that, does it work for you?

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ff5d56b..42b33a3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1250,6 +1250,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 			break;
 		case SDEV_QUIESCE:
 		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:
 			/*
 			 * If the devices is blocked we defer normal commands.
 			 */
@@ -2063,10 +2064,13 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 
 	switch (state) {
 	case SDEV_CREATED:
-		/* There are no legal states that come back to
-		 * created.  This is the manually initialised start
-		 * state */
-		goto illegal;
+		switch (oldstate) {
+		case SDEV_CREATED_BLOCK:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
 			
 	case SDEV_RUNNING:
 		switch (oldstate) {
@@ -2104,8 +2108,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 
 	case SDEV_BLOCK:
 		switch (oldstate) {
-		case SDEV_CREATED:
 		case SDEV_RUNNING:
+		case SDEV_CREATED_BLOCK:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
+	case SDEV_CREATED_BLOCK:
+		switch (oldstate) {
+		case SDEV_CREATED:
 			break;
 		default:
 			goto illegal;
@@ -2393,8 +2406,12 @@ scsi_internal_device_block(struct scsi_device *sdev)
 	int err = 0;
 
 	err = scsi_device_set_state(sdev, SDEV_BLOCK);
-	if (err)
-		return err;
+	if (err) {
+		err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
+
+		if (err)
+			return err;
+	}
 
 	/* 
 	 * The device has transitioned to SDEV_BLOCK.  Stop the
@@ -2437,8 +2454,12 @@ scsi_internal_device_unblock(struct scsi_device *sdev)
 	 * and goose the device queue if successful.  
 	 */
 	err = scsi_device_set_state(sdev, SDEV_RUNNING);
-	if (err)
-		return err;
+	if (err) {
+		err = scsi_device_set_state(sdev, SDEV_CREATED);
+
+		if (err)
+			return err;
+	}
 
 	spin_lock_irqsave(q->queue_lock, flags);
 	blk_start_queue(q);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 84b4879..c6791a7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -730,6 +730,8 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		int *bflags, int async)
 {
+	int ret;
+
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,
 	 * save just vendor/model/rev.
@@ -885,7 +887,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
 	/* set the device running here so that slave configure
 	 * may do I/O */
-	scsi_device_set_state(sdev, SDEV_RUNNING);
+	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
+	if (ret) {
+		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
+
+		if (ret) {
+			sdev_printk(KERN_ERR, sdev,
+				    "in wrong state %s to complete scan\n",
+				    scsi_device_state_name(sdev->sdev_state));
+			return SCSI_SCAN_NO_RESPONSE;
+		}
+	}
 
 	if (*bflags & BLIST_MS_192_BYTES_FOR_3F)
 		sdev->use_192_bytes_for_3f = 1;
@@ -899,7 +911,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	transport_configure_device(&sdev->sdev_gendev);
 
 	if (sdev->host->hostt->slave_configure) {
-		int ret = sdev->host->hostt->slave_configure(sdev);
+		ret = sdev->host->hostt->slave_configure(sdev);
 		if (ret) {
 			/*
 			 * if LLDD reports slave not present, don't clutter
@@ -994,7 +1006,8 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 	 */
 	sdev = scsi_device_lookup_by_target(starget, lun);
 	if (sdev) {
-		if (rescan || sdev->sdev_state != SDEV_CREATED) {
+		if (rescan || (sdev->sdev_state != SDEV_CREATED &&
+			       sdev->sdev_state != SDEV_CREATED_BLOCK)) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -1466,7 +1479,8 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	kfree(lun_data);
  out:
 	scsi_device_put(sdev);
-	if (sdev->sdev_state == SDEV_CREATED)
+	if (sdev->sdev_state == SDEV_CREATED ||
+	    sdev->sdev_state == SDEV_CREATED_BLOCK)
 		/*
 		 * the sdev we used didn't appear in the report luns scan
 		 */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ab3c718..09d311d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -34,6 +34,7 @@ static const struct {
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
 	{ SDEV_BLOCK,	"blocked" },
+	{ SDEV_CREATED_BLOCK, "created-blocked" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 80b2e93..ad3d42d 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -42,9 +42,11 @@ enum scsi_device_state {
 				 * originate in the mid-layer) */
 	SDEV_OFFLINE,		/* Device offlined (by error handling or
 				 * user request */
-	SDEV_BLOCK,		/* Device blocked by scsi lld.  No scsi 
-				 * commands from user or midlayer should be issued
-				 * to the scsi lld. */
+	SDEV_BLOCK,		/* Device blocked by scsi lld.  No
+				 * scsi commands from user or midlayer
+				 * should be issued to the scsi
+				 * lld. */
+	SDEV_CREATED_BLOCK,	/* same as above but for created devices */
 };
 
 enum scsi_device_event {

Re: [PATCH 1/1] scsi: Add added_to_sysfs sdev flag to fix device scanning oops

[Brian King]

James Bottomley wrote:
> What this seems to be exposing is a bug in the state model around the
> blocked state.
> 
> The transition:
> 
> CREATED -> BLOCK -> RUNNING
> 
> shouldn't be legal.  My initial reaction is just to forbid the CREATED
> -> BLOCK transition, but it looks like the fc transport code never
> checks return values from scsi_target_block() (sigh!)
> 
> So an alternate fix should be to correct the state model rather than try
> and work around the deficiencies with additional flags.
> 
> It looks like, to allow the CREATED -> BLOCK -> CREATED transition we
> need an extra state (CREATED_BLOCK) and we forbid the CREATED -> BLOCK
> in favour of it.
> 
> The model also now allows an online transition to do CREATED_BLOCK ->
> BLOCK
> 
> This is a rough code of that, does it work for you?

I loaded up your patch and this fixes my issue as well. Do we need to also
add a check for SDEV_CREATED_BLOCK in scsi_dispatch_cmd?

Thanks,

Brian

> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ff5d56b..42b33a3 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1250,6 +1250,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
>  			break;
>  		case SDEV_QUIESCE:
>  		case SDEV_BLOCK:
> +		case SDEV_CREATED_BLOCK:
>  			/*
>  			 * If the devices is blocked we defer normal commands.
>  			 */
> @@ -2063,10 +2064,13 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
> 
>  	switch (state) {
>  	case SDEV_CREATED:
> -		/* There are no legal states that come back to
> -		 * created.  This is the manually initialised start
> -		 * state */
> -		goto illegal;
> +		switch (oldstate) {
> +		case SDEV_CREATED_BLOCK:
> +			break;
> +		default:
> +			goto illegal;
> +		}
> +		break;
>  			
>  	case SDEV_RUNNING:
>  		switch (oldstate) {
> @@ -2104,8 +2108,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
> 
>  	case SDEV_BLOCK:
>  		switch (oldstate) {
> -		case SDEV_CREATED:
>  		case SDEV_RUNNING:
> +		case SDEV_CREATED_BLOCK:
> +			break;
> +		default:
> +			goto illegal;
> +		}
> +		break;
> +
> +	case SDEV_CREATED_BLOCK:
> +		switch (oldstate) {
> +		case SDEV_CREATED:
>  			break;
>  		default:
>  			goto illegal;
> @@ -2393,8 +2406,12 @@ scsi_internal_device_block(struct scsi_device *sdev)
>  	int err = 0;
> 
>  	err = scsi_device_set_state(sdev, SDEV_BLOCK);
> -	if (err)
> -		return err;
> +	if (err) {
> +		err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
> +
> +		if (err)
> +			return err;
> +	}
> 
>  	/* 
>  	 * The device has transitioned to SDEV_BLOCK.  Stop the
> @@ -2437,8 +2454,12 @@ scsi_internal_device_unblock(struct scsi_device *sdev)
>  	 * and goose the device queue if successful.  
>  	 */
>  	err = scsi_device_set_state(sdev, SDEV_RUNNING);
> -	if (err)
> -		return err;
> +	if (err) {
> +		err = scsi_device_set_state(sdev, SDEV_CREATED);
> +
> +		if (err)
> +			return err;
> +	}
> 
>  	spin_lock_irqsave(q->queue_lock, flags);
>  	blk_start_queue(q);
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 84b4879..c6791a7 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -730,6 +730,8 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  		int *bflags, int async)
>  {
> +	int ret;
> +
>  	/*
>  	 * XXX do not save the inquiry, since it can change underneath us,
>  	 * save just vendor/model/rev.
> @@ -885,7 +887,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
> 
>  	/* set the device running here so that slave configure
>  	 * may do I/O */
> -	scsi_device_set_state(sdev, SDEV_RUNNING);
> +	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
> +	if (ret) {
> +		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
> +
> +		if (ret) {
> +			sdev_printk(KERN_ERR, sdev,
> +				    "in wrong state %s to complete scan\n",
> +				    scsi_device_state_name(sdev->sdev_state));
> +			return SCSI_SCAN_NO_RESPONSE;
> +		}
> +	}
> 
>  	if (*bflags & BLIST_MS_192_BYTES_FOR_3F)
>  		sdev->use_192_bytes_for_3f = 1;
> @@ -899,7 +911,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	transport_configure_device(&sdev->sdev_gendev);
> 
>  	if (sdev->host->hostt->slave_configure) {
> -		int ret = sdev->host->hostt->slave_configure(sdev);
> +		ret = sdev->host->hostt->slave_configure(sdev);
>  		if (ret) {
>  			/*
>  			 * if LLDD reports slave not present, don't clutter
> @@ -994,7 +1006,8 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  	 */
>  	sdev = scsi_device_lookup_by_target(starget, lun);
>  	if (sdev) {
> -		if (rescan || sdev->sdev_state != SDEV_CREATED) {
> +		if (rescan || (sdev->sdev_state != SDEV_CREATED &&
> +			       sdev->sdev_state != SDEV_CREATED_BLOCK)) {
>  			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
>  				"scsi scan: device exists on %s\n",
>  				sdev->sdev_gendev.bus_id));
> @@ -1466,7 +1479,8 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
>  	kfree(lun_data);
>   out:
>  	scsi_device_put(sdev);
> -	if (sdev->sdev_state == SDEV_CREATED)
> +	if (sdev->sdev_state == SDEV_CREATED ||
> +	    sdev->sdev_state == SDEV_CREATED_BLOCK)
>  		/*
>  		 * the sdev we used didn't appear in the report luns scan
>  		 */
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ab3c718..09d311d 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -34,6 +34,7 @@ static const struct {
>  	{ SDEV_QUIESCE, "quiesce" },
>  	{ SDEV_OFFLINE,	"offline" },
>  	{ SDEV_BLOCK,	"blocked" },
> +	{ SDEV_CREATED_BLOCK, "created-blocked" },
>  };
> 
>  const char *scsi_device_state_name(enum scsi_device_state state)
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 80b2e93..ad3d42d 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -42,9 +42,11 @@ enum scsi_device_state {
>  				 * originate in the mid-layer) */
>  	SDEV_OFFLINE,		/* Device offlined (by error handling or
>  				 * user request */
> -	SDEV_BLOCK,		/* Device blocked by scsi lld.  No scsi 
> -				 * commands from user or midlayer should be issued
> -				 * to the scsi lld. */
> +	SDEV_BLOCK,		/* Device blocked by scsi lld.  No
> +				 * scsi commands from user or midlayer
> +				 * should be issued to the scsi
> +				 * lld. */
> +	SDEV_CREATED_BLOCK,	/* same as above but for created devices */
>  };
> 
>  enum scsi_device_event {
> 
> 
> --
> 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


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] scsi: Add added_to_sysfs sdev flag to fix device scanning oops

[James Bottomley]

On Mon, 2008-08-18 at 09:06 -0500, Brian King wrote:
> James Bottomley wrote:
> > What this seems to be exposing is a bug in the state model around the
> > blocked state.
> > 
> > The transition:
> > 
> > CREATED -> BLOCK -> RUNNING
> > 
> > shouldn't be legal.  My initial reaction is just to forbid the CREATED
> > -> BLOCK transition, but it looks like the fc transport code never
> > checks return values from scsi_target_block() (sigh!)
> > 
> > So an alternate fix should be to correct the state model rather than try
> > and work around the deficiencies with additional flags.
> > 
> > It looks like, to allow the CREATED -> BLOCK -> CREATED transition we
> > need an extra state (CREATED_BLOCK) and we forbid the CREATED -> BLOCK
> > in favour of it.
> > 
> > The model also now allows an online transition to do CREATED_BLOCK ->
> > BLOCK
> > 
> > This is a rough code of that, does it work for you?
> 
> I loaded up your patch and this fixes my issue as well. Do we need to also
> add a check for SDEV_CREATED_BLOCK in scsi_dispatch_cmd?

Yes ... everywhere we previously checked for SDEV_BLOCK ... really, that
check now needs to be made into an inline function.  I'll reroll the
patch properly with that included.

James

[PATCH 1/2] add inline functions for recognising created and blocked states

[James Bottomley]

The created and blocked states are very shortly going to correspond to
mixed sdev_state states.

James


---
 drivers/scsi/scsi.c        |   13 +++++++------
 drivers/scsi/scsi_scan.c   |    4 ++--
 include/scsi/scsi_device.h |   11 +++++++++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ee6be59..762a879 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -664,13 +664,14 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 		goto out;
 	}
 
-	/* Check to see if the scsi lld put this device into state SDEV_BLOCK. */
-	if (unlikely(cmd->device->sdev_state == SDEV_BLOCK)) {
+	/* Check to see if the scsi lld made this device blocked. */
+	if (unlikely(scsi_device_blocked(cmd->device))) {
 		/* 
-		 * in SDEV_BLOCK, the command is just put back on the device
-		 * queue.  The suspend state has already blocked the queue so
-		 * future requests should not occur until the device 
-		 * transitions out of the suspend state.
+		 * in blocked state, the command is just put back on
+		 * the device queue.  The suspend state has already
+		 * blocked the queue so future requests should not
+		 * occur until the device transitions out of the
+		 * suspend state.
 		 */
 		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 84b4879..55e76c0 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -994,7 +994,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 	 */
 	sdev = scsi_device_lookup_by_target(starget, lun);
 	if (sdev) {
-		if (rescan || sdev->sdev_state != SDEV_CREATED) {
+		if (rescan || !scsi_device_created(sdev)) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -1466,7 +1466,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	kfree(lun_data);
  out:
 	scsi_device_put(sdev);
-	if (sdev->sdev_state == SDEV_CREATED)
+	if (scsi_device_created(sdev))
 		/*
 		 * the sdev we used didn't appear in the report luns scan
 		 */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 80b2e93..cc46652 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -384,10 +384,21 @@ static inline unsigned int sdev_id(struct scsi_device *sdev)
 #define scmd_id(scmd) sdev_id((scmd)->device)
 #define scmd_channel(scmd) sdev_channel((scmd)->device)
 
+/*
+ * checks for positions of the SCSI state machine
+ */
 static inline int scsi_device_online(struct scsi_device *sdev)
 {
 	return sdev->sdev_state != SDEV_OFFLINE;
 }
+static inline int scsi_device_blocked(struct scsi_device *sdev)
+{
+	return sdev->sdev_state == SDEV_BLOCK;
+}
+static inline int scsi_device_created(struct scsi_device *sdev)
+{
+	return sdev->sdev_state == SDEV_CREATED;
+}
 
 /* accessor functions for the SCSI parameters */
 static inline int scsi_device_sync(struct scsi_device *sdev)
-- 
1.5.6.3

[PATCH 2/2] Update the SCSI state model to allow blocking in the created state

[James Bottomley]

Brian King <brking@linux.vnet.ibm.com> reported that fibre channel
devices can oops during scanning if their ports block (because the
device goes from CREATED -> BLOCK -> RUNNING rather than CREATED ->
BLOCK -> CREATED).

Fix this by adding a new state: CREATED_BLOCK which can only transition
back to CREATED and disallow the CREATED -> BLOCK transition.  Now both
the created and blocked states that the mid-layer recognises can include
CREATED_BLOCK.

James

---

 drivers/scsi/scsi_lib.c    |   39 ++++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_scan.c   |   16 ++++++++++++++--
 drivers/scsi/scsi_sysfs.c  |    1 +
 include/scsi/scsi_device.h |   14 +++++++++-----
 4 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ff5d56b..42b33a3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1250,6 +1250,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 			break;
 		case SDEV_QUIESCE:
 		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:
 			/*
 			 * If the devices is blocked we defer normal commands.
 			 */
@@ -2063,10 +2064,13 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 
 	switch (state) {
 	case SDEV_CREATED:
-		/* There are no legal states that come back to
-		 * created.  This is the manually initialised start
-		 * state */
-		goto illegal;
+		switch (oldstate) {
+		case SDEV_CREATED_BLOCK:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
 			
 	case SDEV_RUNNING:
 		switch (oldstate) {
@@ -2104,8 +2108,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 
 	case SDEV_BLOCK:
 		switch (oldstate) {
-		case SDEV_CREATED:
 		case SDEV_RUNNING:
+		case SDEV_CREATED_BLOCK:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
+	case SDEV_CREATED_BLOCK:
+		switch (oldstate) {
+		case SDEV_CREATED:
 			break;
 		default:
 			goto illegal;
@@ -2393,8 +2406,12 @@ scsi_internal_device_block(struct scsi_device *sdev)
 	int err = 0;
 
 	err = scsi_device_set_state(sdev, SDEV_BLOCK);
-	if (err)
-		return err;
+	if (err) {
+		err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
+
+		if (err)
+			return err;
+	}
 
 	/* 
 	 * The device has transitioned to SDEV_BLOCK.  Stop the
@@ -2437,8 +2454,12 @@ scsi_internal_device_unblock(struct scsi_device *sdev)
 	 * and goose the device queue if successful.  
 	 */
 	err = scsi_device_set_state(sdev, SDEV_RUNNING);
-	if (err)
-		return err;
+	if (err) {
+		err = scsi_device_set_state(sdev, SDEV_CREATED);
+
+		if (err)
+			return err;
+	}
 
 	spin_lock_irqsave(q->queue_lock, flags);
 	blk_start_queue(q);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 55e76c0..c64bf13 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -730,6 +730,8 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		int *bflags, int async)
 {
+	int ret;
+
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,
 	 * save just vendor/model/rev.
@@ -885,7 +887,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
 	/* set the device running here so that slave configure
 	 * may do I/O */
-	scsi_device_set_state(sdev, SDEV_RUNNING);
+	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
+	if (ret) {
+		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
+
+		if (ret) {
+			sdev_printk(KERN_ERR, sdev,
+				    "in wrong state %s to complete scan\n",
+				    scsi_device_state_name(sdev->sdev_state));
+			return SCSI_SCAN_NO_RESPONSE;
+		}
+	}
 
 	if (*bflags & BLIST_MS_192_BYTES_FOR_3F)
 		sdev->use_192_bytes_for_3f = 1;
@@ -899,7 +911,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	transport_configure_device(&sdev->sdev_gendev);
 
 	if (sdev->host->hostt->slave_configure) {
-		int ret = sdev->host->hostt->slave_configure(sdev);
+		ret = sdev->host->hostt->slave_configure(sdev);
 		if (ret) {
 			/*
 			 * if LLDD reports slave not present, don't clutter
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ab3c718..09d311d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -34,6 +34,7 @@ static const struct {
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
 	{ SDEV_BLOCK,	"blocked" },
+	{ SDEV_CREATED_BLOCK, "created-blocked" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cc46652..b49e725 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -42,9 +42,11 @@ enum scsi_device_state {
 				 * originate in the mid-layer) */
 	SDEV_OFFLINE,		/* Device offlined (by error handling or
 				 * user request */
-	SDEV_BLOCK,		/* Device blocked by scsi lld.  No scsi 
-				 * commands from user or midlayer should be issued
-				 * to the scsi lld. */
+	SDEV_BLOCK,		/* Device blocked by scsi lld.  No
+				 * scsi commands from user or midlayer
+				 * should be issued to the scsi
+				 * lld. */
+	SDEV_CREATED_BLOCK,	/* same as above but for created devices */
 };
 
 enum scsi_device_event {
@@ -393,11 +395,13 @@ static inline int scsi_device_online(struct scsi_device *sdev)
 }
 static inline int scsi_device_blocked(struct scsi_device *sdev)
 {
-	return sdev->sdev_state == SDEV_BLOCK;
+	return sdev->sdev_state == SDEV_BLOCK ||
+		sdev->sdev_state == SDEV_CREATED_BLOCK;
 }
 static inline int scsi_device_created(struct scsi_device *sdev)
 {
-	return sdev->sdev_state == SDEV_CREATED;
+	return sdev->sdev_state == SDEV_CREATED ||
+		sdev->sdev_state == SDEV_CREATED_BLOCK;
 }
 
 /* accessor functions for the SCSI parameters */
-- 
1.5.6.3