On 2019/10/18 21:43, Martin K. Petersen wrote:
> Hannes,
>
>> The one thing which I patently don't like is the ambivalence between
>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
>> of them is set? IE what would be the correct way of action if
>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
>> way around?
> I agree, it's a mess.
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
>> But more important, from a quick glance not all drivers set the
>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>> never evaluated after this patchset.
> And yet we appear to have several code paths where sense evaluation is
> contingent on DRIVER_SENSE. So no matter what, behavior might
> change if we enforce consistent semantics. *sigh*
So what should we do to prevent unit-value access of sshdr?
1. still init sshdr in __scsi_execute?
@@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const
unsigned char *cmd,
struct scsi_request *rq;
int ret = DRIVER_ERROR << 24;
+ /*
+ * Zero-initialize sshdr for those callers that check the *sshdr
+ * contents even if no sense data is available.
+ */
+ if (sshdr)
+ memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
req = blk_get_request(sdev->request_queue,
data_direction == DMA_TO_DEVICE ?
REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
2. init sshdr in callers of scsi_execute, instead of check the result of
scsi_execute and check
whether sshdr is valid? for example:
@@ -506,6 +506,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned
char *buffer,
put_unaligned_be32(len, &cmd[6]);
memset(buffer, 0, len);
+ memset(&sshdr, 0 ,sizeof(sshdr));
result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
&sshdr, 30 * HZ, 3, NULL)
Besides, should sd_pr_command init sshdr?? cause sd_pr_command have checked
the result of scsi_execute
and check whether sshdr is valid.
3. get rid of DRIVER_* completely? even this, we should init sshdr first.
sshdr is just 8 bytes, init it does not affect performance
My advice is 2.
>
>> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
>> scsi_sense_valid() only.
> I would really like to get rid of DRIVER_* completely. Except for
> DRIVER_SENSE, few are actually in use:
>
> DRIVER_OK: 0
> DRIVER_BUSY: 0
> DRIVER_SOFT: 0
> DRIVER_MEDIA: 0
> DRIVER_ERROR: 6
> DRIVER_INVALID: 4
> DRIVER_TIMEOUT: 1
> DRIVER_SENSE: 58
>
> Johannes: Whatever happened to your efforts at cleaning all this up? Do
> you have a patch series or a working tree we could use as starting
> point?
>
