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 <mwi...@suse.com>

Ack-ed by: Douglas Gilbert <dgilb...@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 = {