[PATCH v2] scsi_debug: call resp_*() function after setting host_scribble

[PATCH v2] scsi_debug: call resp_*() function after setting host_scribble

[Martin Wilck]

Error injection in scsi_debug (e.g. opts=16, SDEBUG_OPT_TRANSPORT_ERR)
currently doesn't work correctly because the test for sqcp in
resp_read_dt0() and similar resp_*() functions always fails.
sqcp is set from cmnd->host_scribble, which is set in schedule_resp(), which
is called from scsi_debug_queuecommand() after calling the resp_* function.

Defer calling resp_*() until after cmnd->host_scribble is
set in schedule_resp().

Fixes: c483739430f1 "scsi_debug: add multiple queue support"

Changes in v2: Adapted to code changes after 80c49563e250
	"scsi: scsi_debug: implement IMMED bit"

Notes about this adaptation:

The "flags &= ~F_LONG_DELAY" statement in scsi_debug_queuecommand()
from 80c49563e250 had no effect. Dropped it.
Because we call the resp_*() function later now, the code flow in
schedule_resp() is slightly different now for the IMMED case - instead of
falling through to the "respond_in_thread" label immediately, the command will
be put in the work queue with zero delay.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_debug.c | 53 +++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 905501075ec6..26ce022dd6f4 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4315,7 +4315,10 @@ static void setup_inject(struct sdebug_queue *sqp,
  * SCSI_MLQUEUE_HOST_BUSY if temporarily out of resources.
  */
 static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
-			 int scsi_result, int delta_jiff, int ndelay)
+			 int scsi_result,
+			 int (*pfp)(struct scsi_cmnd *,
+				    struct sdebug_dev_info *),
+			 int delta_jiff, int ndelay)
 {
 	unsigned long iflags;
 	int k, num_in_q, qdepth, inject;
@@ -4331,9 +4334,6 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 	}
 	sdp = cmnd->device;
 
-	if (unlikely(sdebug_verbose && scsi_result))
-		sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n",
-			    __func__, scsi_result);
 	if (delta_jiff == 0)
 		goto respond_in_thread;
 
@@ -4388,7 +4388,6 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 	sqcp = &sqp->qc_arr[k];
 	sqcp->a_cmnd = cmnd;
 	cmnd->host_scribble = (unsigned char *)sqcp;
-	cmnd->result = scsi_result;
 	sd_dp = sqcp->sd_dp;
 	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
 	if (unlikely(sdebug_every_nth && sdebug_any_injecting_opt))
@@ -4398,6 +4397,22 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 		if (sd_dp == NULL)
 			return SCSI_MLQUEUE_HOST_BUSY;
 	}
+
+	cmnd->result = pfp != NULL ? pfp(cmnd, devip) : 0;
+	if (cmnd->result & SDEG_RES_IMMED_MASK) {
+		/*
+		 * This is the F_DELAY_OVERR case. No delay.
+		 */
+		cmnd->result &= ~SDEG_RES_IMMED_MASK;
+		delta_jiff = ndelay = 0;
+	}
+	if (cmnd->result == 0 && scsi_result != 0)
+		cmnd->result = scsi_result;
+
+	if (unlikely(sdebug_verbose && cmnd->result))
+		sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n",
+			    __func__, cmnd->result);
+
 	if (delta_jiff > 0 || ndelay > 0) {
 		ktime_t kt;
 
@@ -4440,7 +4455,10 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 	return 0;
 
 respond_in_thread:	/* call back to mid-layer using invocation thread */
-	cmnd->result = scsi_result;
+	cmnd->result = pfp != NULL ? pfp(cmnd, devip) : 0;
+	cmnd->result &= ~SDEG_RES_IMMED_MASK;
+	if (cmnd->result == 0 && scsi_result != 0)
+		cmnd->result = scsi_result;
 	cmnd->scsi_done(cmnd);
 	return 0;
 }
@@ -5628,6 +5646,7 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 	struct sdebug_dev_info *devip;
 	u8 *cmd = scp->cmnd;
 	int (*r_pfp)(struct scsi_cmnd *, struct sdebug_dev_info *);
+	int (*pfp)(struct scsi_cmnd *, struct sdebug_dev_info *) = NULL;
 	int k, na;
 	int errsts = 0;
 	u32 flags;
@@ -5749,19 +5768,13 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 			return 0;	/* ignore command: make trouble */
 	}
 	if (likely(oip->pfp))
-		errsts = oip->pfp(scp, devip);	/* calls a resp_* function */
-	else if (r_pfp)	/* if leaf function ptr NULL, try the root's */
-		errsts = r_pfp(scp, devip);
-	if (errsts & SDEG_RES_IMMED_MASK) {
-		errsts &= ~SDEG_RES_IMMED_MASK;
-		flags |= F_DELAY_OVERR;
-		flags &= ~F_LONG_DELAY;
-	}
-
+		pfp = oip->pfp;	/* calls a resp_* function */
+	else
+		pfp = r_pfp;    /* if leaf function ptr NULL, try the root's */
 
 fini:
 	if (F_DELAY_OVERR & flags)
-		return schedule_resp(scp, devip, errsts, 0, 0);
+		return schedule_resp(scp, devip, errsts, pfp, 0, 0);
 	else if ((sdebug_jdelay || sdebug_ndelay) && (flags & F_LONG_DELAY)) {
 		/*
 		 * If any delay is active, want F_LONG_DELAY to be at least 1
@@ -5771,14 +5784,14 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 		int jdelay = (sdebug_jdelay < 2) ? 1 : sdebug_jdelay;
 
 		jdelay = mult_frac(USER_HZ * jdelay, HZ, USER_HZ);
-		return schedule_resp(scp, devip, errsts, jdelay, 0);
+		return schedule_resp(scp, devip, errsts, pfp, jdelay, 0);
 	} else
-		return schedule_resp(scp, devip, errsts, sdebug_jdelay,
+		return schedule_resp(scp, devip, errsts, pfp, sdebug_jdelay,
 				     sdebug_ndelay);
 check_cond:
-	return schedule_resp(scp, devip, check_condition_result, 0, 0);
+	return schedule_resp(scp, devip, check_condition_result, NULL, 0, 0);
 err_out:
-	return schedule_resp(scp, NULL, DID_NO_CONNECT << 16, 0, 0);
+	return schedule_resp(scp, NULL, DID_NO_CONNECT << 16, NULL, 0, 0);
 }
 
 static struct scsi_host_template sdebug_driver_template = {
-- 
2.16.1

Re: [PATCH v2] scsi_debug: call resp_*() function after setting host_scribble

[Douglas Gilbert]

On 2018-02-14 05:05 AM, Martin Wilck wrote:
> Error injection in scsi_debug (e.g. opts=16, SDEBUG_OPT_TRANSPORT_ERR)
> currently doesn't work correctly because the test for sqcp in
> resp_read_dt0() and similar resp_*() functions always fails.
> sqcp is set from cmnd->host_scribble, which is set in schedule_resp(), which
> is called from scsi_debug_queuecommand() after calling the resp_* function.
> 
> Defer calling resp_*() until after cmnd->host_scribble is
> set in schedule_resp().
> 
> Fixes: c483739430f1 "scsi_debug: add multiple queue support"
> 
> Changes in v2: Adapted to code changes after 80c49563e250
> 	"scsi: scsi_debug: implement IMMED bit"
> 
> Notes about this adaptation:
> 
> The "flags &= ~F_LONG_DELAY" statement in scsi_debug_queuecommand()
> from 80c49563e250 had no effect. Dropped it.
> Because we call the resp_*() function later now, the code flow in
> schedule_resp() is slightly different now for the IMMED case - instead of
> falling through to the "respond_in_thread" label immediately, the command will
> be put in the work queue with zero delay.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Ack-ed by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/scsi_debug.c | 53 +++++++++++++++++++++++++++++------------------
>   1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 905501075ec6..26ce022dd6f4 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4315,7 +4315,10 @@ static void setup_inject(struct sdebug_queue *sqp,
>    * SCSI_MLQUEUE_HOST_BUSY if temporarily out of resources.
>    */
>   static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
> -			 int scsi_result, int delta_jiff, int ndelay)
> +			 int scsi_result,
> +			 int (*pfp)(struct scsi_cmnd *,
> +				    struct sdebug_dev_info *),
> +			 int delta_jiff, int ndelay)
>   {
>   	unsigned long iflags;
>   	int k, num_in_q, qdepth, inject;
> @@ -4331,9 +4334,6 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
>   	}
>   	sdp = cmnd->device;
>   
> -	if (unlikely(sdebug_verbose && scsi_result))
> -		sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n",
> -			    __func__, scsi_result);
>   	if (delta_jiff == 0)
>   		goto respond_in_thread;
>   
> @@ -4388,7 +4388,6 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
>   	sqcp = &sqp->qc_arr[k];
>   	sqcp->a_cmnd = cmnd;
>   	cmnd->host_scribble = (unsigned char *)sqcp;
> -	cmnd->result = scsi_result;
>   	sd_dp = sqcp->sd_dp;
>   	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>   	if (unlikely(sdebug_every_nth && sdebug_any_injecting_opt))
> @@ -4398,6 +4397,22 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
>   		if (sd_dp == NULL)
>   			return SCSI_MLQUEUE_HOST_BUSY;
>   	}
> +
> +	cmnd->result = pfp != NULL ? pfp(cmnd, devip) : 0;
> +	if (cmnd->result & SDEG_RES_IMMED_MASK) {
> +		/*
> +		 * This is the F_DELAY_OVERR case. No delay.
> +		 */
> +		cmnd->result &= ~SDEG_RES_IMMED_MASK;
> +		delta_jiff = ndelay = 0;
> +	}
> +	if (cmnd->result == 0 && scsi_result != 0)
> +		cmnd->result = scsi_result;
> +
> +	if (unlikely(sdebug_verbose && cmnd->result))
> +		sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n",
> +			    __func__, cmnd->result);
> +
>   	if (delta_jiff > 0 || ndelay > 0) {
>   		ktime_t kt;
>   
> @@ -4440,7 +4455,10 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
>   	return 0;
>   
>   respond_in_thread:	/* call back to mid-layer using invocation thread */
> -	cmnd->result = scsi_result;
> +	cmnd->result = pfp != NULL ? pfp(cmnd, devip) : 0;
> +	cmnd->result &= ~SDEG_RES_IMMED_MASK;
> +	if (cmnd->result == 0 && scsi_result != 0)
> +		cmnd->result = scsi_result;
>   	cmnd->scsi_done(cmnd);
>   	return 0;
>   }
> @@ -5628,6 +5646,7 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
>   	struct sdebug_dev_info *devip;
>   	u8 *cmd = scp->cmnd;
>   	int (*r_pfp)(struct scsi_cmnd *, struct sdebug_dev_info *);
> +	int (*pfp)(struct scsi_cmnd *, struct sdebug_dev_info *) = NULL;
>   	int k, na;
>   	int errsts = 0;
>   	u32 flags;
> @@ -5749,19 +5768,13 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
>   			return 0;	/* ignore command: make trouble */
>   	}
>   	if (likely(oip->pfp))
> -		errsts = oip->pfp(scp, devip);	/* calls a resp_* function */
> -	else if (r_pfp)	/* if leaf function ptr NULL, try the root's */
> -		errsts = r_pfp(scp, devip);
> -	if (errsts & SDEG_RES_IMMED_MASK) {
> -		errsts &= ~SDEG_RES_IMMED_MASK;
> -		flags |= F_DELAY_OVERR;
> -		flags &= ~F_LONG_DELAY;
> -	}
> -
> +		pfp = oip->pfp;	/* calls a resp_* function */
> +	else
> +		pfp = r_pfp;    /* if leaf function ptr NULL, try the root's */
>   
>   fini:
>   	if (F_DELAY_OVERR & flags)
> -		return schedule_resp(scp, devip, errsts, 0, 0);
> +		return schedule_resp(scp, devip, errsts, pfp, 0, 0);
>   	else if ((sdebug_jdelay || sdebug_ndelay) && (flags & F_LONG_DELAY)) {
>   		/*
>   		 * If any delay is active, want F_LONG_DELAY to be at least 1
> @@ -5771,14 +5784,14 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
>   		int jdelay = (sdebug_jdelay < 2) ? 1 : sdebug_jdelay;
>   
>   		jdelay = mult_frac(USER_HZ * jdelay, HZ, USER_HZ);
> -		return schedule_resp(scp, devip, errsts, jdelay, 0);
> +		return schedule_resp(scp, devip, errsts, pfp, jdelay, 0);
>   	} else
> -		return schedule_resp(scp, devip, errsts, sdebug_jdelay,
> +		return schedule_resp(scp, devip, errsts, pfp, sdebug_jdelay,
>   				     sdebug_ndelay);
>   check_cond:
> -	return schedule_resp(scp, devip, check_condition_result, 0, 0);
> +	return schedule_resp(scp, devip, check_condition_result, NULL, 0, 0);
>   err_out:
> -	return schedule_resp(scp, NULL, DID_NO_CONNECT << 16, 0, 0);
> +	return schedule_resp(scp, NULL, DID_NO_CONNECT << 16, NULL, 0, 0);
>   }
>   
>   static struct scsi_host_template sdebug_driver_template = {
> 

Re: [PATCH v2] scsi_debug: call resp_*() function after setting host_scribble

[Martin K. Petersen]

Martin,

Applied to 4.17/scsi-queue.

Minor patch submission nits below (for next time, I fixed them up).

> Error injection in scsi_debug (e.g. opts=16, SDEBUG_OPT_TRANSPORT_ERR)
> currently doesn't work correctly because the test for sqcp in
> resp_read_dt0() and similar resp_*() functions always fails.  sqcp is
> set from cmnd->host_scribble, which is set in schedule_resp(), which
> is called from scsi_debug_queuecommand() after calling the resp_*
> function.
>
> Defer calling resp_*() until after cmnd->host_scribble is
> set in schedule_resp().
>
> Fixes: c483739430f1 "scsi_debug: add multiple queue support"

Your Signed-off-by: needs to go here. And then you need a "---"
separator before the change log.

> Changes in v2: Adapted to code changes after 80c49563e250
> 	"scsi: scsi_debug: implement IMMED bit"
>
> Notes about this adaptation:
>
> The "flags &= ~F_LONG_DELAY" statement in scsi_debug_queuecommand()
> from 80c49563e250 had no effect. Dropped it.
> Because we call the resp_*() function later now, the code flow in
> schedule_resp() is slightly different now for the IMMED case - instead of
> falling through to the "respond_in_thread" label immediately, the command will
> be put in the work queue with zero delay.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi_debug: call resp_*() function after setting host_scribble

[Martin K. Petersen]

Doug,

> Ack-ed by: Douglas Gilbert <dgilbert@interlog.com>

s/Ack-ed by:/Acked-by:/

-- 
Martin K. Petersen	Oracle Linux Engineering