Hi Bart,
Thanks for your suggestions.
Hi Sang Hyun,
Bart Van Assche <bvanassche@xxxxxxx> 於 2020年7月22日 週三 下午11:11寫道：
>
> On 2020-07-22 02:14, Stanley Chu wrote:
> > Hi Bart,
> >
> > On Sat, 2020-07-18 at 13:30 -0700, Bart Van Assche wrote:
> >> On 2020-07-17 00:39, Lee Sang Hyun wrote:
> >>> - ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
> >>> - START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
> >>> - if (ret) {
> >>> - sdev_printk(KERN_WARNING, sdp,
> >>> - "START_STOP failed for power mode: %d, result %x\n",
> >>> - pwr_mode, ret);
> >>> - if (driver_byte(ret) == DRIVER_SENSE)
> >>> - scsi_print_sense_hdr(sdp, NULL, &sshdr);
> >>> + for (retries = 0; retries < SSU_RETRIES; retries++) {
> >>> + ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
> >>> + START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
> >>> + if (ret) {
> >>> + sdev_printk(KERN_WARNING, sdp,
> >>> + "START_STOP failed for power mode: %d, result %x\n",
> >>> + pwr_mode, ret);
> >>> + if (driver_byte(ret) == DRIVER_SENSE)
> >>> + scsi_print_sense_hdr(sdp, NULL, &sshdr);
> >>> + } else {
> >>> + break;
> >>> + }
> >>
> >> The ninth argument of scsi_execute() is called 'retries'. Wouldn't it be
> >> better to pass a nonzero value as the 'retries' argument of
> >> scsi_execute() instead of adding a loop around the scsi_execute() call?
> >
> > If a SCSI command issued via scsi_execute() encounters "timeout" or
> > "check condition", scsi_noretry_cmd() will return 1 (true) because
> > blk_rq_is_passthrough() is true due to REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT
> > flag was set to this SCSI command by scsi_execute(). Therefore even a
> > non-zero 'retries' value is assigned while calling scsi_execute(), the
> > failed command has no chance to be retried since the decision will be
> > no-retry by scsi_noretry_cmd().
> >
> > (Take command timeout as example)
> > scsi_times_out()->scsi_abort_command()->scmd_eh_abort_handler(), here
> > scsi_noretry_cmd() returns 1, and then command will be finished (with
> > error code) without retry.
> >
> > In scsi_noretry_cmd(), there is a comment message in section
> > "check_type" as below
> >
> > /*
> > * assume caller has checked sense and determined
> > * the check condition was retryable.
> > */
> >
> > I am not sure if "timeout" and "check condition" cases in such SCSI
> > commands issued via scsi_execute() are specially designed to be unable
> > to retry.
> >
> > Would you have any suggestions if LLD drivers would like to retry these
> > kinds of SCSI commands?
>
> How about summarizing the above explanation of why the 'retry' argument
> of scsi_execute() cannot be used in a two or three line comment and to
> add that comment above the loop introduced by this patch?
>
I like this patch since this could also recover the "SSU timeout" case
we met before.
Could you please make this as formal patch with Bart's suggestion?
Thanks,
Stanley Chu
