On 10/4/22 5:27 PM, Bart Van Assche wrote:
Agree and will handle the other review comments.
>> @@ -1608,6 +1608,7 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
>> /* Usually overridden by the ULP */
>> cmd->allowed = 0;
>> + cmd->failures = NULL;
>> memset(cmd->cmnd, 0, sizeof(cmd->cmnd));
>> return scsi_cmd_to_driver(cmd)->init_command(cmd);
>> }
>
> Shouldn't the cmd->failures assignment be moved into scsi_initialize_rq()
> rather than keeping it in scsi_prepare_cmd() such that the value set by
> scsi_execute() is preserved?
There is that check:
/*
* Special handling for passthrough commands, which don't go to the ULP
* at all:
*/
if (blk_rq_is_passthrough(req))
return scsi_setup_scsi_cmnd(sdev, req);
above this code so we only hit the failures = NULL code for non-passthrough. So
the value set by scsi_execute is preserved like is done for allowed and passthrough
commands.
I agree it should be moved though. It makes more sense to be in scsi_initialize_rq
since it's only called for non passthrough right now.
>
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index bac55decf900..9ab97e48c5c2 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -65,6 +65,24 @@ enum scsi_cmnd_submitter {
>> SUBMITTED_BY_SCSI_RESET_IOCTL = 2,
>> } __packed;
>> +#define SCMD_FAILURE_NONE 0
>> +#define SCMD_FAILURE_ANY -1
>
> This is the same value as -EPERM. Would something like 0x7fffffff be a better
> choice for SCMD_FAILURE_ANY?
I didn't get that. All of this is used for the scsi_cmnd->result evaluation
and retries (host, status, internal bytes) which are all positive and where
we don't use -Exyx errors.
>
>> +struct scsi_failure {
>> + int result;
>> + u8 sense;
>> + u8 asc;
>> + u8 ascq;
>> +
>> + s8 allowed;
>> + s8 retries;
>> +};
>
> Why has 'retries' type s8 instead of u8?
>
There is a:
#define SCMD_FAILURE_NO_LIMIT -1
in the defines that got cut off when you replied which is used for some
users that had no limit on retries.
