Re: [PATCH RFC 00/24] scsi: Revamp result values

[Hannes Reinecke <hare@suse.de>]

On 10/21/19 8:32 PM, Douglas Gilbert wrote:
> On 2019-10-21 5:52 a.m., Hannes Reinecke wrote:
>> Hi all,
>>
>> the 'result' field in the SCSI command is defined as having
>> 4 fields. The top byte is declared as a 'driver_byte', where the
>> driver can signal some internal status back to the midlayer.
>> However, there is quite a bit of overlap between the driver_byte
>> and the host_byte, resulting in the driver_byte being used in
>> very few places, and mostly in legacy drivers.
>> Additionally, we have _two_ sets of definitions for the
>> last byte (status byte), which can specified either in SAM terms
>> or in the linux-specific terms, which are shifted right by one
>> from the SAM ones.
>> Needless to say, the linux-specific ones are declared obsolete
>> for years now.
>> And to make the confusion complete, both the status byte and
>> the driver byte have a byte for a valid sense code, resulting
>> in quite some confusion which of those bits to check.
>>
>> This patchset does several things:
>> - remove the linux-specific status byte definitions, and use
>>    the SAM values throughout
>> - replace the driver-byte values with either SAM ones (for sense
>>    code checking) or host-byte definitions
>> - remove the driver-byte definitions
> 
> This is a brave change proposal. The masked_status has been tricked
> up so it won't break user code. However the driver byte is exposed
> by the sg v2, v3 and v4 interfaces which means via bsg device nodes,
> sg devices nodes and many other block storage device nodes (e.g.
> /dev/sdc and /dev/st1) via:
>       ioctl(<storage_dev>, SG_IO, ptr_to_v3_interface) .
> 
> So if there is any user space code out there that checks the
> driver byte (e.g. 'sg_io_hdr::driver_status & DRIVER_SENSE') do we
> have a problem?
> 
> If so, we could hack the DRIVER_SENSE case *** by putting it back
> for the user space to see when the driver (e.g. sg) knows there
> is sense data. What about the other values?
> 
>> As usual, comments and reviews are welcome.
> 
> It is hard to make an omelette without breaking some eggs.
> 
> Doug Gilbert
> 
>> Please note, commit 66cf50e65b18 ("scsi: qla2xxx: fixup incorrect
>> usage of host_byte") from 5.4/scsi-fixes is a prerequisite for
>> this patch series.
> 
> <snip>
> 
> *** Here is a snippet from sg3_utils library code:
> 
>     if ((SAM_STAT_CHECK_CONDITION == scsi_status) ||
>         (SAM_STAT_COMMAND_TERMINATED == scsi_status) ||
>         (SG_LIB_DRIVER_SENSE == masked_driver_status))
>         return sg_err_category_sense(sense_buffer, sb_len);
> 
> Due to the logical OR, as long as SAM_STAT_CHECK_CONDITION is set
> whenever there is sense, then we don't care about DRIVER_SENSE.
> 
> I believe this code comes from the days before auto-sense when say a
> READ(6) would fail, send back a CHECK_CONDITION and the host would then
> need to issue a REQUEST SENSE command to get the sense data. However
> REQUEST SENSE could itself yield a CHECK_CONDITION. Hence DRIVER_SENSE
> set, SAM_STAT_CHECK_CONDITION clear could be interpreted as the
> initial command failing and the follow-up REQUEST SENSE succeeded; if
> they are both set, then both commands failed (e.g. the disk has gone
> away).
Well, the easier explanation is that not every driver sets DRIVER_SENSE;
some do, some don't, relying on CHECK_CONDITION here.

Which also means that any code relying on DRIVER_SENSE alone would break
even today.
So really I don't think this should break anything; but if the consensus
is that we need to fake DRIVER_SENSE for userland ABI stability so be it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer