Re: [PATCH 000/117] Make better use of static type checking

Bart Van Assche
2021-04-20T16:12:28Z

On 4/19/21 11:04 PM, Hannes Reinecke wrote:
> We should not try to preserve the split SCSI result value with its four
> distinct fields.

I don't think that we have the freedom to drop the four byte SCSI result
entirely since multiple user space APIs use that data structure. The
four-byte SCSI result value is embedded in the following user space API
data structures (there may be others):
* struct sg_io_v4, the SG_IO header includes the driver_status
(driver_byte()), transport_status (host_byte()) and device_status
(scsi_status & 0xff) (the message byte is not included).
* struct fc_bsg_reply.
* struct iscsi_bsg_reply.
* struct ufs_bsg_reply.

> I'd rather have the message byte handling moved into the SCSI parallel
> drivers (where really it should've been in the first place).
> The driver byte can go entirely as the DRIVER_SENSE flag can be replaced
> with a check for valid sense code, DRIVER_TIMEOUT is pretty much
> identical to DID_TIMEOUT (with the semantic difference _who_ set the
> timeout), and DRIVER_ERROR can be folded back into the caller.
> All other values are unused, allowing us to drop driver error completely.
>
> With that we're only having two fields (host byte and status byte) left,
> which should be treated as two distinct values.
>
> As it so happens I do have a patchset for this; guess I'll be posting it
> to demonstrate the idea.

This patch series does not prevent the conversion described above.
Although I think that the changes described above would help, I have a
few concerns:
- Some drivers use the result member of struct scsi_cmnd for another
purpose than storing SCSI result values. See e.g. the CAM_* values
defined in drivers/scsi/aic7xxx/cam.h and the use of these values in
drivers/scsi/aic7xxx/aic79xx_osm.c. From the master branch:

[ ... ]
cmd->scsi_done = scsi_done;
cmd->result = CAM_REQ_INPROG << 16;
rtn = ahd_linux_run_command(ahd, dev, cmd);
[ ... ]

[ ... ]
if ((cmd->result & (CAM_DEV_QFRZN << 16)) != 0) {
cmd->result &= ~(CAM_DEV_QFRZN << 16);
dev->qfrozen--;
}
[ ... ]

Converting this driver could be challenging and may end up in rewriting
this driver.
- The SCSI drivers that do something meaningful with the message byte
are parallel SCSI drivers. Parallel SCSI is a technology that was
popular 20-30 years ago but that is no longer popular today. Finding
test hardware may be a big challenge.
- The parallel SCSI technology is no longer commercially relevant. It
may be challenging to motivate people (including yourself) to convert a
significant number of parallel SCSI drivers that each have a small user
base.

Although I appreciate it that you have shared this proposal and also
that you have proposed to lead this effort, I'm not convinced that this
proposal will be implemented soon. So I propose to proceed with this
patch series.

Thanks,

Bart.