On 4/20/21 6:12 PM, Bart Van Assche wrote:
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.
Yes, I know. But that's the user-space API; there is nothing forcing us
to continue using it internally as long as we keep the userspace API intact.
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.
No, not really. I've got a patch for that; we should have separated the
CAM values from the SAM status values a long time ago.
- 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.
Indeed, the _drivers_ do.
But the SCSI midlayer does not; is just checks for a non-zero value here.
So we can easily map a non-zero message byte to DID_ERROR before calling
->scsi_done(), allowing us to keep the message byte handling within the
SCSI parallel drivers.
I got patches for that ...
- 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.
... true, but then your patchset suffers from the same issue, no?
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.
I'm currently porting the patchset to 5.13/scsi-queue, and hope to have
it published soon.
Don't get me wrong; I like your idea with introducing enums for stricter
type-checking in the SCSI stack.
My point is simply that if we were converting the SCSI result (for
whatever reason) we should drop those bits which are pointless (like
DRIVER_SENSE), or never have been used at all (like DRIVER_HARD).
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
