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

[Hannes Reinecke <hare@suse.de>]

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@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer