| Message ID | 20180214100557.30717-1-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 = { >
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!
Doug,
> Ack-ed by: Douglas Gilbert <dgilbert@interlog.com>
s/Ack-ed by:/Acked-by:/
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 = {
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(-)