[PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[Li Zhong]

Just noticed that after commit 919f797, it is possible that 
scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv 
returned from the above function. 

Maybe it is not possible at run time, but from the code itself, we'd better
have this check?  

This patch actually is a delta of my previous patch 
[https://lkml.org/lkml/2012/4/11/814] and commit 919f797

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2936b44..b9dd7f0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -832,7 +832,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		int old_good_bytes = good_bytes;
 		drv = scsi_cmd_to_driver(cmd);
-		if (drv->done)
+		if (drv && drv->done)
 			good_bytes = drv->done(cmd);
 		/*
 		 * USB may not give sense identifying bad sector and

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[James Bottomley]

On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote:
> Just noticed that after commit 919f797, it is possible that 
> scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv 
> returned from the above function. 
> 
> Maybe it is not possible at run time, but from the code itself, we'd better
> have this check?  

There's not much point having a check that never trips, unless it's an
assert, in which case a NULL deref does that.  All it does is add
pointless instructions to the critical path.  only REQ_TYPE_BLOCK_PC
commands can be submitted without a driver, so the check above would
seem to preclude that.

James

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[Li Zhong]

On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote:
> On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote:
> > Just noticed that after commit 919f797, it is possible that 
> > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv 
> > returned from the above function. 
> > 
> > Maybe it is not possible at run time, but from the code itself, we'd better
> > have this check?  
> 
> There's not much point having a check that never trips, unless it's an
> assert, in which case a NULL deref does that.  All it does is add
> pointless instructions to the critical path.  only REQ_TYPE_BLOCK_PC
> commands can be submitted without a driver, so the check above would
> seem to preclude that.

Hi James, 

Thank you, it sounds reasonable to me. Let's drop it. 

Thanks, Zhong

> James
> 
> 

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[James Bottomley]

On Mon, 2012-09-24 at 15:03 +0800, Li Zhong wrote:
> On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote:
> > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote:
> > > Just noticed that after commit 919f797, it is possible that 
> > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv 
> > > returned from the above function. 
> > > 
> > > Maybe it is not possible at run time, but from the code itself, we'd better
> > > have this check?  
> > 
> > There's not much point having a check that never trips, unless it's an
> > assert, in which case a NULL deref does that.  All it does is add
> > pointless instructions to the critical path.  only REQ_TYPE_BLOCK_PC
> > commands can be submitted without a driver, so the check above would
> > seem to preclude that.
> 
> Hi James, 
> 
> Thank you, it sounds reasonable to me. Let's drop it. 

Well, there is another thing you might do: The path length of
scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be
worth getting it back to what it was (this looks to be doable with the
same != REQ_TYPE_BLOCK_PC test in the error handler.  Plus, I think it
fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC
commands when a driver is and isn't attached (I've cc'd Martin to see
what he thinks).

James

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[Li Zhong]

On Mon, 2012-09-24 at 11:35 +0400, James Bottomley wrote:
> On Mon, 2012-09-24 at 15:03 +0800, Li Zhong wrote:
> > On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote:
> > > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote:
> > > > Just noticed that after commit 919f797, it is possible that 
> > > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv 
> > > > returned from the above function. 
> > > > 
> > > > Maybe it is not possible at run time, but from the code itself, we'd better
> > > > have this check?  
> > > 
> > > There's not much point having a check that never trips, unless it's an
> > > assert, in which case a NULL deref does that.  All it does is add
> > > pointless instructions to the critical path.  only REQ_TYPE_BLOCK_PC
> > > commands can be submitted without a driver, so the check above would
> > > seem to preclude that.
> > 
> > Hi James, 
> > 
> > Thank you, it sounds reasonable to me. Let's drop it. 
> 
> Well, there is another thing you might do: The path length of
> scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be
> worth getting it back to what it was (this looks to be doable with the
> same != REQ_TYPE_BLOCK_PC test in the error handler.  Plus, I think it
> fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC
> commands when a driver is and isn't attached (I've cc'd Martin to see
> what he thinks).

Hi James, 

Do you mean something like this: 

=======

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index de2337f..c1b05a8 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index ac06cc5..377df4a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -134,16 +134,7 @@ struct scsi_cmnd {
 
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[James Bottomley]

On Mon, 2012-09-24 at 17:25 +0800, Li Zhong wrote:
> On Mon, 2012-09-24 at 11:35 +0400, James Bottomley wrote:
> > On Mon, 2012-09-24 at 15:03 +0800, Li Zhong wrote:
> > > On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote:
> > > > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote:
> > > > > Just noticed that after commit 919f797, it is possible that 
> > > > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv 
> > > > > returned from the above function. 
> > > > > 
> > > > > Maybe it is not possible at run time, but from the code itself, we'd better
> > > > > have this check?  
> > > > 
> > > > There's not much point having a check that never trips, unless it's an
> > > > assert, in which case a NULL deref does that.  All it does is add
> > > > pointless instructions to the critical path.  only REQ_TYPE_BLOCK_PC
> > > > commands can be submitted without a driver, so the check above would
> > > > seem to preclude that.
> > > 
> > > Hi James, 
> > > 
> > > Thank you, it sounds reasonable to me. Let's drop it. 
> > 
> > Well, there is another thing you might do: The path length of
> > scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be
> > worth getting it back to what it was (this looks to be doable with the
> > same != REQ_TYPE_BLOCK_PC test in the error handler.  Plus, I think it
> > fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC
> > commands when a driver is and isn't attached (I've cc'd Martin to see
> > what he thinks).
> 
> Hi James, 
> 
> Do you mean something like this: 

Pretty much, yes.

James

> =======
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index de2337f..c1b05a8 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
>  			     int cmnd_size, int timeout, unsigned sense_bytes)
>  {
>  	struct scsi_device *sdev = scmd->device;
> -	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
>  	struct Scsi_Host *shost = sdev->host;
>  	DECLARE_COMPLETION_ONSTACK(done);
>  	unsigned long timeleft;
> @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
>  
>  	scsi_eh_restore_cmnd(scmd, &ses);
>  
> -	if (sdrv && sdrv->eh_action)
> -		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> +	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> +		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
> +		if (sdrv->eh_action)
> +			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> +	}
>  
>  	return rtn;
>  }
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index ac06cc5..377df4a 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -134,16 +134,7 @@ struct scsi_cmnd {
>  
>  static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
>  {
> -	struct scsi_driver **sdp;
> -
> -	if (!cmd->request->rq_disk)
> -		return NULL;
> -
> -	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
> -	if (!sdp)
> -		return NULL;
> -
> -	return *sdp;
> +	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
>  }
>  
>  extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
> 
> 
> --
> 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

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

James> Plus, I think it fixes a bug where you get different behaviours
James> from REQ_TYPE_BLOCK_PC commands when a driver is and isn't
James> attached (I've cc'd Martin to see what he thinks).

I'm fine with having the eh action be triggered for FS requests only, if
that's what you're asking?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[James Bottomley]

On Wed, 2012-09-26 at 22:02 -0400, Martin K. Petersen wrote:
> >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> James> Plus, I think it fixes a bug where you get different behaviours
> James> from REQ_TYPE_BLOCK_PC commands when a driver is and isn't
> James> attached (I've cc'd Martin to see what he thinks).
> 
> I'm fine with having the eh action be triggered for FS requests only, if
> that's what you're asking?

Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC commands
(which includes flush and other strange types), but if you think it
should only be for REQ_TYPE_FS, that's fine too.

James

[PATCH scsi] Short the path length of scsi_cmd_to_driver()

[Li Zhong]

Hi James, Martin,  

Here is the updated version, please help to review.

Thanks, Zhong

========
As suggested by James: this patch tries to short the path length of
scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be
submitted without a driver, so we could avoid the related NULL 
checking, as long as we make sure we don't use it for REQ_TYPE_BLOCK_PC
type commands. Plus, this fixes a bug where you get different behaviors
from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached. 

And Martin pointed out that we could have eh action be triggered for 
REQ_TYPE_FS type only. 

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 drivers/scsi/scsi_error.c |    8 +++++---
 include/scsi/scsi_cmnd.h  |   12 ++----------
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index de2337f..4001559 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index ac06cc5..de5f5d8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -132,18 +132,10 @@ struct scsi_cmnd {
 	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
+/* make sure not to use it with REQ_TYPE_BLOCK_PC commands */
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
-- 
1.7.7.6

Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver()

[Martin K. Petersen]

>>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes:

> @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
> 
> 	scsi_eh_restore_cmnd(scmd, &ses);
> 
>-	if (sdrv && sdrv->eh_action)
>-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
>+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
>+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
>+		if (sdrv->eh_action)
>+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
>+	}
> 
> 	return rtn;
> }

My only concern is whether our device lifetime rules guarantee that the
ULD is always attached when we service an error handling command?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver()

[Li Zhong]

On Thu, 2012-09-27 at 13:43 -0400, Martin K. Petersen wrote:
> >>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes:
> 
> > @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
> > 
> > 	scsi_eh_restore_cmnd(scmd, &ses);
> > 
> >-	if (sdrv && sdrv->eh_action)
> >-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> >+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
> >+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
> >+		if (sdrv->eh_action)
> >+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> >+	}
> > 
> > 	return rtn;
> > }
> 
> My only concern is whether our device lifetime rules guarantee that the
> ULD is always attached when we service an error handling command?

Thank you, Martin, for the review. 

I don't know much about scsi, it might take me some more time to have an
answer to the above question. 

For now, if I understand correctly, maybe we could only do the
not-consistent behaviours bug fix?  Or could we provide two versions of
scsi_cmd_to_driver(), one with NULL checking for scsi_send_eh_cmnd(),
one without the checking for scsi_finish_command()? 

Thanks, Zhong

Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver()

[James Bottomley]

On Thu, 2012-09-27 at 13:43 -0400, Martin K. Petersen wrote:
> >>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes:
> 
> > @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
> > 
> > 	scsi_eh_restore_cmnd(scmd, &ses);
> > 
> >-	if (sdrv && sdrv->eh_action)
> >-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> >+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
> >+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
> >+		if (sdrv->eh_action)
> >+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> >+	}
> > 
> > 	return rtn;
> > }
> 
> My only concern is whether our device lifetime rules guarantee that the
> ULD is always attached when we service an error handling command?

Yes, they are.  We can only get REQ_TYPE_FS through a filesystem, which
must be mounted on a block device, which is provided by the ULD.  You
can't unmount with outstanding I/O (which people complain about when it
goes into error handling, I'll admit), so the ULD has to stay bound.

James

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

>> I'm fine with having the eh action be triggered for FS requests only,
>> if that's what you're asking?

James> Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC
James> commands (which includes flush and other strange types), but if
James> you think it should only be for REQ_TYPE_FS, that's fine too.

I'm ok either way.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver()

[James Bottomley]

On Thu, 2012-09-27 at 13:41 -0400, Martin K. Petersen wrote:
> >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> >> I'm fine with having the eh action be triggered for FS requests only,
> >> if that's what you're asking?
> 
> James> Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC
> James> commands (which includes flush and other strange types), but if
> James> you think it should only be for REQ_TYPE_FS, that's fine too.
> 
> I'm ok either way.

OK, let's mirror the code in scsi.c then, so != REQ_TYPE_BLOCK_PC.

James

[PATCH v2 scsi] Short the path length of scsi_cmd_to_driver()

[Li Zhong]

As suggested by James: this patch tries to short the path length of
scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be
submitted without a driver, so we could avoid the related NULL
checking, as long as we make sure we don't use it for REQ_TYPE_BLOCK_PC
type commands. Plus, this fixes a bug where you get different behaviors
from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached.

v2: mirror code != REQ_TYPE_BLOCK_PC in scsi.c, rather than == REQ_TYPE_FS

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 drivers/scsi/scsi_error.c |    8 +++++---
 include/scsi/scsi_cmnd.h  |   12 ++----------
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index de2337f..c1b05a8 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index ac06cc5..de5f5d8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -132,18 +132,10 @@ struct scsi_cmnd {
 	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
+/* make sure not to use it with REQ_TYPE_BLOCK_PC commands */
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
-- 
1.7.7.6

Re: [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver()

[Martin K. Petersen]

>>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes:

Li> As suggested by James: this patch tries to short the path length of
Li> scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be
Li> submitted without a driver, so we could avoid the related NULL
Li> checking, as long as we make sure we don't use it for
Li> REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you
Li> get different behaviors from REQ_TYPE_BLOCK_PC commands when a
Li> driver is and isn't attached.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering