Re: [PATCH 07/42] scsi: Kill DRIVER_SENSE

Douglas Gilbert
2021-04-21T19:46:18Z

On 2021-04-21 1:47 p.m., Hannes Reinecke wrote:
> Replace the check for DRIVER_SENSE with a check for
> SAM_STAT_CHECK_CONDITION and audit all callsites to
> ensure the SAM status is set correctly.
> For backwards compability move the DRIVER_SENSE definition
> to sg.h, and update the sg driver to set the DRIVER_SENSE
> driver_status whenever SAM_STAT_CHECK_CONDITION is present.

I may have missed it but you probably want to do the same
backwards compatibility DRIVER_SENSE trick for the
ioctl(SG_IO) implemented in block/scsi_ioctl.c . That way
DRIVER_SENSE will appear in the sg_io_hdr::driver_status byte
when check_condition_sense are set for both these cases:
ioctl(sd_fd, SG_IO, &a_sg_v3_obj)
ioctl(sg_fd, SG_IO, &a_sg_v3_obj)

And for bsg which uses sg_io_v4 for SCSI commands you set
sg_io_v4::driver_status = 0
in all cases. If check_condition and sense are active, why
not set DRIVER_SENSE for consistency. block/scsi_ioctl.c
includes scsi/sg.h so the DRIVER_SENSE define is visble.

Doug Gilbert

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/ata/libata-scsi.c | 13 ++------
> drivers/scsi/NCR5380.c | 2 +-
> drivers/scsi/advansys.c | 2 --
> drivers/scsi/aic7xxx/aic79xx_osm.c | 19 +++++-------
> drivers/scsi/aic7xxx/aic7xxx_osm.c | 1 -
> drivers/scsi/arcmsr/arcmsr_hba.c | 1 -
> drivers/scsi/ch.c | 2 +-
> drivers/scsi/cxlflash/superpipe.c | 3 +-
> drivers/scsi/dc395x.c | 13 ++------
> drivers/scsi/esp_scsi.c | 4 +--
> drivers/scsi/megaraid.c | 10 +++---
> drivers/scsi/megaraid/megaraid_mbox.c | 8 ++---
> drivers/scsi/megaraid/megaraid_sas_base.c | 2 --
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 -
> drivers/scsi/mvumi.c | 1 -
> drivers/scsi/scsi.c | 7 -----
> drivers/scsi/scsi_debug.c | 4 +--
> drivers/scsi/scsi_ioctl.c | 3 +-
> drivers/scsi/scsi_lib.c | 9 ++----
> drivers/scsi/scsi_scan.c | 2 +-
> drivers/scsi/scsi_transport_spi.c | 2 +-
> drivers/scsi/sd.c | 34 +++++++++++----------
> drivers/scsi/sd_zbc.c | 3 +-
> drivers/scsi/sg.c | 9 ++++--
> drivers/scsi/stex.c | 4 +--
> drivers/scsi/sym53c8xx_2/sym_glue.c | 6 ++--
> drivers/scsi/ufs/ufshcd.c | 2 +-
> drivers/scsi/virtio_scsi.c | 3 +-
> drivers/scsi/vmw_pvscsi.c | 3 --
> drivers/target/loopback/tcm_loop.c | 1 -
> drivers/usb/storage/cypress_atacb.c | 4 +--
> drivers/xen/xen-scsiback.c | 2 +-
> include/scsi/sg.h | 2 ++
> 33 files changed, 68 insertions(+), 114 deletions(-)
>

<snip>

> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 737cea9d908e..9122d05563d0 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -498,9 +498,11 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
> old_hdr->host_status = hp->host_status;
> old_hdr->driver_status = hp->driver_status;
> if ((CHECK_CONDITION & hp->masked_status) ||
> - (DRIVER_SENSE & hp->driver_status))
> + (srp->sense_b && (srp->sense_b[0] & 0x70) == 0x70)) {
> + old_hdr->driver_status |= DRIVER_SENSE;
> memcpy(old_hdr->sense_buffer, srp->sense_b,
> sizeof (old_hdr->sense_buffer));
> + }
> switch (hp->host_status) {
> /* This setup of 'result' is for backward compatibility and is best
> ignored by the user who should use target, host + driver status */
> @@ -574,7 +576,7 @@ sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
> hp->sb_len_wr = 0;
> if ((hp->mx_sb_len > 0) && hp->sbp) {
> if ((CHECK_CONDITION & hp->masked_status) ||
> - (DRIVER_SENSE & hp->driver_status)) {
> + (srp->sense_b && (srp->sense_b[0] & 0x70) == 0x70)) {
> int sb_len = SCSI_SENSE_BUFFERSIZE;
> sb_len = (hp->mx_sb_len > sb_len) ? sb_len : hp->mx_sb_len;
> len = 8 + (int) srp->sense_b[7]; /* Additional sense length field */
> @@ -582,7 +584,8 @@ sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
> if (copy_to_user(hp->sbp, srp->sense_b, len)) {
> err = -EFAULT;
> goto err_out;
> - }
> + } else
> + hp->driver_status |= DRIVER_SENSE;
> hp->sb_len_wr = len;
> }
> }

<snip>

> diff --git a/include/scsi/sg.h b/include/scsi/sg.h
> index 7327e12f3373..a90703cf15f4 100644
> --- a/include/scsi/sg.h
> +++ b/include/scsi/sg.h
> @@ -131,6 +131,8 @@ struct compat_sg_io_hdr {
> #define SG_INFO_DIRECT_IO 0x2 /* direct IO requested and performed */
> #define SG_INFO_MIXED_IO 0x4 /* part direct, part indirect IO */
>
> +/* Obsolete DRIVER_SENSE setting */
> +#define DRIVER_SENSE 0x08
>
> typedef struct sg_scsi_id { /* used by SG_GET_SCSI_ID ioctl() */
> int host_no; /* as in "scsi<n>" where 'n' is one of 0, 1, 2 etc */
>