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

Message ID 20210420000845.25873-1-bvanassche@acm.org
Headers show
Series
  • Make better use of static type checking
Related show

Message

Bart Van Assche April 20, 2021, 12:06 a.m.
Hi Martin,

This patch series improves static checking inside the SCSI subsystem as
follows:
- Introduce enumeration types for the SCSI status, message, host and driver
  bytes.
- Change 'int' into 'union scsi_status' in case of SCSI results. This helps
  the compiler and humans to tell the difference between a scalar and a SCSI
  result.

This patch series is long because it touches all SCSI drivers and because it
has been split into one patch per SCSI driver.

This patch series introduces a backwards-incompatible change in the API
between SCSI core and drivers. A possible strategy is to postpone the patch
that removes backwards compatibility to a later kernel version.

Please consider this patch series for kernel version v5.14.

Thanks,

Bart.

Bart Van Assche (117):
  libsas: Introduce more SAM status code aliases in enum exec_status
  Introduce enums for the SAM, message, host and driver status codes
  Change the type of the second argument of
    scsi_host_complete_all_commands()
  libiscsi: Use the host_status enum
  libsas: Use the host_status and sam_status enums
  target: Use enum sam_status instead of u8
  lpfc: Reformat four comparisons
  fc: Add a compile-time structure size check
  iscsi: Add a compile-time structure size check
  ufs: Add a compile-time structure size check
  Introduce the scsi_status union
  block: Convert SCSI and bsg code to the scsi_status union
  core: Convert to the scsi_status union
  ch: Pass union scsi_status to driver_byte()
  sd: Convert to the scsi_status union
  sr: Convert to the scsi_status union
  st: Convert to the scsi_status union
  sg: Convert to the scsi_status union
  3w*: Convert to the scsi_status union
  53c700: Convert to the scsi_status union
  BusLogic: Convert to the scsi_status union
  NCR5380: Convert to the scsi_status union
  a100u2w: Convert to the scsi_status union
  aacraid: Convert to the scsi_status union
  acornscsi: Annotate fallthrough
  acornscsi: Convert to the scsi_status union
  advansys: Convert to the scsi_status union
  aha*: Convert to the scsi_status union
  aic*: Convert to the scsi_status union
  arcmsr: Convert to the scsi_status union
  ata: Convert to the scsi_status union
  atp870u: Convert to the scsi_status union
  be2iscsi: Convert to the scsi_status union
  bfa: Use type int32_t to represent a signed integer
  bfa: Convert to the scsi_status union
  bnx2fc: Convert to the scsi_status union
  cdrom: Convert to the scsi_status union
  csiostor: Convert to the scsi_status union
  cxlflash: Convert to the scsi_status union
  dc395x: Use the set_{host,msg,status}_byte() functions
  dc395x: Convert to the scsi_status union
  dpt_i2o: Convert to the scsi_status union
  esas2r: Convert to the scsi_status union
  esp_scsi: Convert to the scsi_status union
  fas216: Fix two source code comments
  fas216: Convert to the scsi_status union
  fc: Convert to the scsi_status union
  fdomain: Convert to the scsi_status union
  firewire: sbp2: Convert to the scsi_status union
  fnic: Convert to the scsi_status union
  hpsa: Convert to the scsi_status union
  hptiop: Convert to the scsi_status union
  ib_srp: Convert to the scsi_status union
  ibmvfc: Fix the documentation of the return value of
    ibmvfc_host_chkready()
  ibmvfc: Convert to the scsi_status union
  ibmvscsi: Convert to the scsi_status union
  ide: Convert to the scsi_status union
  imm: Convert to the scsi_status union
  initio: Convert to the scsi_status union
  ipr: Convert to the scsi_status union
  ips: Convert to the scsi_status union
  iscsi: Convert to the scsi_status union
  libfc: Convert to the scsi_status union
  sas: Convert to the scsi_status union
  lpfc: Convert to the scsi_status union
  mac53c94: Convert to the scsi_status union
  megaraid: Convert to the scsi_status union
  mesh: Convert to the scsi_status union
  message: fusion: Convert to the scsi_status union
  mpt3sas: Convert to the scsi_status union
  mvumi: Convert to the scsi_status union
  myrb: Convert to the scsi_status union
  myrs: Convert to the scsi_status union
  ncr53c8xx: Convert to the scsi_status union
  nfsd: Convert to the scsi_status union
  nsp32: Convert to the scsi_status union
  pcmcia: Convert to the scsi_status union
  pktcdvd: Convert to the scsi_status union
  pmcraid: Convert to the scsi_status union
  ppa: Convert to the scsi_status union
  ps3rom: Convert to the scsi_status union
  qedf: Convert to the scsi_status union
  qedi: Convert to the scsi_status union
  qla1280: Convert to the scsi_status union
  qla2xxx: Convert to the scsi_status union
  qla4xxx: Convert to the scsi_status union
  qlogicfas408: Convert to the scsi_status union
  qlogicpti: Convert to the scsi_status union
  s390/zfcp: Convert to the scsi_status union
  scsi_debug: Convert to the scsi_status union
  smartpqi: Convert to the scsi_status union
  snic: Convert to the scsi_status union
  staging: Convert to the scsi_status union
  stex: Convert to the scsi_status union
  storvsc: Convert to the scsi_status union
  sym53c8xx_2: Convert to the scsi_status union
  target: Convert to the scsi_status union
  ufs: Remove an unused structure member
  ufs: Remove a local variable
  ufs: Use enum sam_status where appropriate
  ufs: Remove an assignment from ufshcd_transfer_rsp_status()
  ufs: Convert to the scsi_status union
  usb: Convert to the scsi_status union
  virtio-scsi: Convert to the scsi_status union
  vmw_pvscsi: Convert to the scsi_status union
  wd33c93: Convert to the scsi_status union
  wd719x: Convert to the scsi_status union
  xen-scsiback: Pass union status to the {status,msg,host,driver}_byte()
    macros
  xen-scsifront: Convert to the scsi_status union
  Finalize the switch from 'int' to 'union scsi_status'
  Use the scsi_status union more widely
  Change the return type of scsi_execute() into union scsi_status
  Change the return type of scsi_execute_req() into union scsi_status
  Change the return type of scsi_test_unit_ready() into union
    scsi_status
  Change the return types of scsi_mode_sense() and sd_do_mode_sense()
  Change the return type of scsi_mode_select() into union scsi_status
  Change the return type of ioctl_internal_command() into union
    scsi_status

 block/bsg-lib.c                               |  16 +-
 block/bsg.c                                   |   6 +-
 block/scsi_ioctl.c                            |  14 +-
 drivers/ata/libata-sata.c                     |   2 +-
 drivers/ata/libata-scsi.c                     |  60 +++----
 drivers/block/pktcdvd.c                       |   2 +-
 drivers/cdrom/cdrom.c                         |   2 +-
 drivers/firewire/sbp2.c                       |   2 +-
 drivers/hwmon/drivetemp.c                     |   2 +-
 drivers/ide/ide-atapi.c                       |  10 +-
 drivers/ide/ide-cd.c                          |  20 +--
 drivers/ide/ide-cd_ioctl.c                    |   2 +-
 drivers/ide/ide-devsets.c                     |   4 +-
 drivers/ide/ide-dma.c                         |   2 +-
 drivers/ide/ide-eh.c                          |  36 ++---
 drivers/ide/ide-floppy.c                      |  10 +-
 drivers/ide/ide-io.c                          |  10 +-
 drivers/ide/ide-ioctls.c                      |   4 +-
 drivers/ide/ide-park.c                        |   2 +-
 drivers/ide/ide-pm.c                          |   6 +-
 drivers/ide/ide-tape.c                        |   4 +-
 drivers/ide/ide-taskfile.c                    |   6 +-
 drivers/infiniband/ulp/srp/ib_srp.c           |  27 ++--
 drivers/message/fusion/mptfc.c                |   6 +-
 drivers/message/fusion/mptsas.c               |   2 +-
 drivers/message/fusion/mptscsih.c             |  70 ++++-----
 drivers/message/fusion/mptspi.c               |   4 +-
 drivers/s390/scsi/zfcp_dbf.c                  |   2 +-
 drivers/s390/scsi/zfcp_dbf.h                  |   2 +-
 drivers/s390/scsi/zfcp_fc.c                   |   4 +-
 drivers/s390/scsi/zfcp_fc.h                   |   2 +-
 drivers/s390/scsi/zfcp_scsi.c                 |   6 +-
 drivers/scsi/3w-9xxx.c                        |  12 +-
 drivers/scsi/3w-sas.c                         |   8 +-
 drivers/scsi/3w-xxxx.c                        |  20 +--
 drivers/scsi/53c700.c                         |   4 +-
 drivers/scsi/BusLogic.c                       |  25 +--
 drivers/scsi/NCR5380.c                        |  30 ++--
 drivers/scsi/a100u2w.c                        |   2 +-
 drivers/scsi/aacraid/aachba.c                 | 142 ++++++++---------
 drivers/scsi/advansys.c                       |   4 +-
 drivers/scsi/aha152x.c                        |   4 +-
 drivers/scsi/aha1542.c                        |   4 +-
 drivers/scsi/aha1740.c                        |   4 +-
 drivers/scsi/aic7xxx/aic79xx_osm.c            |  10 +-
 drivers/scsi/aic7xxx/aic79xx_osm.h            |  16 +-
 drivers/scsi/aic7xxx/aic7xxx_osm.c            |   8 +-
 drivers/scsi/aic7xxx/aic7xxx_osm.h            |  16 +-
 drivers/scsi/aic94xx/aic94xx_task.c           |   2 +-
 drivers/scsi/arcmsr/arcmsr_hba.c              |  38 ++---
 drivers/scsi/arm/acornscsi.c                  |  30 ++--
 drivers/scsi/arm/fas216.c                     |  44 +++---
 drivers/scsi/atp870u.c                        |  14 +-
 drivers/scsi/be2iscsi/be_main.c               |  12 +-
 drivers/scsi/bfa/bfad_bsg.c                   |  14 +-
 drivers/scsi/bfa/bfad_im.c                    |  30 ++--
 drivers/scsi/bnx2fc/bnx2fc_io.c               |  14 +-
 drivers/scsi/ch.c                             |   3 +-
 drivers/scsi/constants.c                      |   8 +-
 drivers/scsi/csiostor/csio_scsi.c             |  18 ++-
 drivers/scsi/cxlflash/main.c                  |  32 ++--
 drivers/scsi/cxlflash/superpipe.c             |  14 +-
 drivers/scsi/cxlflash/vlun.c                  |   8 +-
 drivers/scsi/dc395x.c                         |  73 ++++-----
 drivers/scsi/device_handler/scsi_dh_alua.c    |  28 ++--
 drivers/scsi/device_handler/scsi_dh_emc.c     |   7 +-
 drivers/scsi/device_handler/scsi_dh_hp_sw.c   |  12 +-
 drivers/scsi/device_handler/scsi_dh_rdac.c    |   2 +-
 drivers/scsi/dpt_i2o.c                        |  28 ++--
 drivers/scsi/esas2r/esas2r.h                  |   2 +-
 drivers/scsi/esas2r/esas2r_main.c             |  12 +-
 drivers/scsi/esp_scsi.c                       |  10 +-
 drivers/scsi/fdomain.c                        |   4 +-
 drivers/scsi/fnic/fnic_scsi.c                 |  38 ++---
 drivers/scsi/hosts.c                          |   8 +-
 drivers/scsi/hpsa.c                           |  74 ++++-----
 drivers/scsi/hptiop.c                         |  20 +--
 drivers/scsi/ibmvscsi/ibmvfc.c                |  26 +--
 drivers/scsi/ibmvscsi/ibmvscsi.c              |  16 +-
 drivers/scsi/imm.c                            |  10 +-
 drivers/scsi/initio.c                         |   2 +-
 drivers/scsi/ipr.c                            |  34 ++--
 drivers/scsi/ips.c                            |  72 ++++-----
 drivers/scsi/isci/request.c                   |  10 +-
 drivers/scsi/isci/task.c                      |   2 +-
 drivers/scsi/libfc/fc_fcp.c                   |  36 ++---
 drivers/scsi/libfc/fc_lport.c                 |   8 +-
 drivers/scsi/libiscsi.c                       |  51 +++---
 drivers/scsi/libsas/sas_ata.c                 |   5 +-
 drivers/scsi/libsas/sas_expander.c            |   2 +-
 drivers/scsi/libsas/sas_scsi_host.c           |  13 +-
 drivers/scsi/libsas/sas_task.c                |   4 +-
 drivers/scsi/lpfc/lpfc_bsg.c                  | 114 +++++++-------
 drivers/scsi/lpfc/lpfc_scsi.c                 |  78 +++++----
 drivers/scsi/mac53c94.c                       |   2 +-
 drivers/scsi/megaraid.c                       |  50 +++---
 drivers/scsi/megaraid/megaraid_mbox.c         |  62 ++++----
 drivers/scsi/megaraid/megaraid_sas_base.c     |  30 ++--
 drivers/scsi/megaraid/megaraid_sas_fusion.c   |  20 +--
 drivers/scsi/mesh.c                           |  10 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c          |  78 ++++-----
 drivers/scsi/mvsas/mv_sas.c                   |  10 +-
 drivers/scsi/mvumi.c                          |  18 +--
 drivers/scsi/myrb.c                           |  48 +++---
 drivers/scsi/myrs.c                           |  14 +-
 drivers/scsi/ncr53c8xx.c                      |   2 +-
 drivers/scsi/nsp32.c                          |  40 ++---
 drivers/scsi/pcmcia/nsp_cs.c                  |  18 ++-
 drivers/scsi/pcmcia/sym53c500_cs.c            |  12 +-
 drivers/scsi/pm8001/pm8001_hwi.c              |  16 +-
 drivers/scsi/pm8001/pm8001_sas.c              |   4 +-
 drivers/scsi/pm8001/pm80xx_hwi.c              |  14 +-
 drivers/scsi/pmcraid.c                        |  28 ++--
 drivers/scsi/ppa.c                            |  12 +-
 drivers/scsi/ps3rom.c                         |  10 +-
 drivers/scsi/qedf/qedf_io.c                   |  24 +--
 drivers/scsi/qedi/qedi_fw.c                   |   2 +-
 drivers/scsi/qla1280.c                        |   2 +-
 drivers/scsi/qla2xxx/qla_bsg.c                | 148 +++++++++---------
 drivers/scsi/qla2xxx/qla_iocb.c               |   4 +-
 drivers/scsi/qla2xxx/qla_isr.c                |  14 +-
 drivers/scsi/qla2xxx/qla_mr.c                 |   6 +-
 drivers/scsi/qla2xxx/qla_os.c                 |  26 +--
 drivers/scsi/qla4xxx/ql4_bsg.c                |  76 ++++-----
 drivers/scsi/qla4xxx/ql4_isr.c                |  32 ++--
 drivers/scsi/qla4xxx/ql4_os.c                 |  14 +-
 drivers/scsi/qlogicfas408.c                   |   4 +-
 drivers/scsi/qlogicpti.c                      |   6 +-
 drivers/scsi/scsi.c                           |  25 ++-
 drivers/scsi/scsi_debug.c                     |  26 +--
 drivers/scsi/scsi_debugfs.c                   |   2 +-
 drivers/scsi/scsi_error.c                     |  46 +++---
 drivers/scsi/scsi_ioctl.c                     |  24 +--
 drivers/scsi/scsi_lib.c                       |  84 +++++-----
 drivers/scsi/scsi_logging.c                   |   8 +-
 drivers/scsi/scsi_scan.c                      |  22 +--
 drivers/scsi/scsi_transport_fc.c              |  10 +-
 drivers/scsi/scsi_transport_iscsi.c           |   5 +-
 drivers/scsi/scsi_transport_sas.c             |   3 +-
 drivers/scsi/scsi_transport_spi.c             |   5 +-
 drivers/scsi/sd.c                             |  83 +++++-----
 drivers/scsi/sd.h                             |   3 +-
 drivers/scsi/sd_zbc.c                         |  12 +-
 drivers/scsi/ses.c                            |   4 +-
 drivers/scsi/sg.c                             |  11 +-
 drivers/scsi/smartpqi/smartpqi_init.c         |  12 +-
 drivers/scsi/snic/snic_scsi.c                 |  14 +-
 drivers/scsi/sr.c                             |  20 +--
 drivers/scsi/sr_ioctl.c                       |   6 +-
 drivers/scsi/st.c                             |  23 +--
 drivers/scsi/st.h                             |   5 +-
 drivers/scsi/stex.c                           |  20 +--
 drivers/scsi/storvsc_drv.c                    |   6 +-
 drivers/scsi/sym53c8xx_2/sym_glue.c           |   2 +-
 drivers/scsi/sym53c8xx_2/sym_glue.h           |   8 +-
 drivers/scsi/ufs/ufs_bsg.c                    |   2 +-
 drivers/scsi/ufs/ufshcd.c                     |  51 +++---
 drivers/scsi/ufs/ufshcd.h                     |   1 -
 drivers/scsi/virtio_scsi.c                    |  14 +-
 drivers/scsi/vmw_pvscsi.c                     |  32 ++--
 drivers/scsi/wd33c93.c                        |  30 ++--
 drivers/scsi/wd719x.c                         |   4 +-
 drivers/scsi/xen-scsifront.c                  |   6 +-
 drivers/staging/rts5208/rtsx.c                |  14 +-
 drivers/staging/rts5208/rtsx_transport.c      |   8 +-
 drivers/staging/unisys/include/iochannel.h    |   3 +-
 .../staging/unisys/visorhba/visorhba_main.c   |  12 +-
 drivers/target/loopback/tcm_loop.c            |   6 +-
 drivers/target/target_core_alua.c             |   6 +-
 drivers/target/target_core_iblock.c           |   2 +-
 drivers/target/target_core_pr.c               |   8 +-
 drivers/target/target_core_pscsi.c            |  16 +-
 drivers/target/target_core_sbc.c              |  10 +-
 drivers/target/target_core_spc.c              |  14 +-
 drivers/target/target_core_transport.c        |   5 +-
 drivers/target/target_core_xcopy.c            |   2 +-
 drivers/usb/image/microtek.c                  |   4 +-
 drivers/usb/storage/cypress_atacb.c           |  12 +-
 drivers/usb/storage/datafab.c                 |   4 +-
 drivers/usb/storage/isd200.c                  |  34 ++--
 drivers/usb/storage/jumpshot.c                |   4 +-
 drivers/usb/storage/realtek_cr.c              |  10 +-
 drivers/usb/storage/scsiglue.c                |   4 +-
 drivers/usb/storage/transport.c               |  30 ++--
 drivers/usb/storage/uas.c                     |   8 +-
 drivers/usb/storage/usb.c                     |  14 +-
 drivers/xen/xen-scsiback.c                    |  11 +-
 fs/nfsd/blocklayout.c                         |   4 +-
 include/linux/bsg-lib.h                       |   3 +-
 include/scsi/libsas.h                         |   3 +
 include/scsi/scsi.h                           |  96 ++----------
 include/scsi/scsi_bsg_iscsi.h                 |   5 +-
 include/scsi/scsi_cmnd.h                      |  22 +--
 include/scsi/scsi_dbg.h                       |  10 +-
 include/scsi/scsi_device.h                    |  26 +--
 include/scsi/scsi_eh.h                        |   4 +-
 include/scsi/scsi_host.h                      |   2 +-
 include/scsi/scsi_proto.h                     |  53 ++++---
 include/scsi/scsi_request.h                   |   3 +-
 include/scsi/scsi_status.h                    | 120 ++++++++++++++
 include/scsi/scsi_transport_srp.h             |  15 +-
 include/target/target_core_backend.h          |   4 +-
 include/target/target_core_base.h             |   3 +-
 include/trace/events/scsi.h                   |   2 +-
 include/uapi/scsi/scsi_bsg_fc.h               |   7 +
 include/uapi/scsi/scsi_bsg_ufs.h              |  10 +-
 206 files changed, 1995 insertions(+), 1861 deletions(-)
 create mode 100644 include/scsi/scsi_status.h

Comments

Hannes Reinecke April 20, 2021, 6:04 a.m. | #1
On 4/20/21 2:06 AM, Bart Van Assche wrote:
> 

> Hi Martin,

> 

> This patch series improves static checking inside the SCSI subsystem as

> follows:

> - Introduce enumeration types for the SCSI status, message, host and driver

>    bytes.

> - Change 'int' into 'union scsi_status' in case of SCSI results. This helps

>    the compiler and humans to tell the difference between a scalar and a SCSI

>    result.

> 

> This patch series is long because it touches all SCSI drivers and because it

> has been split into one patch per SCSI driver.

> 

> This patch series introduces a backwards-incompatible change in the API

> between SCSI core and drivers. A possible strategy is to postpone the patch

> that removes backwards compatibility to a later kernel version.

> 

> Please consider this patch series for kernel version v5.14.

> 

I'd rather not go this way.
We should not try to preserve the split SCSI result value with its four 
distinct fields.

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.

Bart, I would very much prefer if we could work on this together so as 
to avoid duplicate work.

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
Greg Kroah-Hartman April 20, 2021, 7:47 a.m. | #2
On Mon, Apr 19, 2021 at 07:13:38PM -0700, Bart Van Assche wrote:
> An explanation of the purpose of this patch is available in the patch

> "scsi: Introduce the scsi_status union".


Where is that at?

As a stand-alone-patch, this is not ok in a changelog text at all,
sorry, and I can not take this.

greg k-h
Chuck Lever III April 20, 2021, 2:36 p.m. | #3
> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote:

> 

> An explanation of the purpose of this patch is available in the patch

> "scsi: Introduce the scsi_status union".

> 

> Cc: "J. Bruce Fields" <bfields@fieldses.org>

> Cc: Chuck Lever <chuck.lever@oracle.com>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>


Hi Bart, I assume this is going into v5.13 via the SCSI tree?
Do you need an Acked-by: from the NFSD maintainers?


> ---

> fs/nfsd/blocklayout.c | 4 ++--

> 1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c

> index 1058659a8d31..f10f559684a6 100644

> --- a/fs/nfsd/blocklayout.c

> +++ b/fs/nfsd/blocklayout.c

> @@ -255,9 +255,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,

> 	req->cmd_len = COMMAND_SIZE(INQUIRY);

> 

> 	blk_execute_rq(NULL, rq, 1);

> -	if (req->result) {

> +	if (req->status.combined) {

> 		pr_err("pNFS: INQUIRY 0x83 failed with: %x\n",

> -			req->result);

> +			req->status.combined);

> 		error = -EIO;

> 		goto out_put_request;

> 	}


--
Chuck Lever
Bart Van Assche April 20, 2021, 3:02 p.m. | #4
On 4/20/21 12:47 AM, Greg Kroah-Hartman wrote:
> On Mon, Apr 19, 2021 at 07:13:38PM -0700, Bart Van Assche wrote:

>> An explanation of the purpose of this patch is available in the patch

>> "scsi: Introduce the scsi_status union".

> 

> Where is that at?

> 

> As a stand-alone-patch, this is not ok in a changelog text at all,

> sorry, and I can not take this.


Hi Greg,

That is a reference to an earlier patch in this series. I plan to
replace the above text with the more elaborate description when I repost
this patch series. For the current version of this patch series, please
take a look at
https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u

Thanks,

Bart.
Greg Kroah-Hartman April 20, 2021, 3:06 p.m. | #5
On Tue, Apr 20, 2021 at 08:02:46AM -0700, Bart Van Assche wrote:
> On 4/20/21 12:47 AM, Greg Kroah-Hartman wrote:

> > On Mon, Apr 19, 2021 at 07:13:38PM -0700, Bart Van Assche wrote:

> >> An explanation of the purpose of this patch is available in the patch

> >> "scsi: Introduce the scsi_status union".

> > 

> > Where is that at?

> > 

> > As a stand-alone-patch, this is not ok in a changelog text at all,

> > sorry, and I can not take this.

> 

> Hi Greg,

> 

> That is a reference to an earlier patch in this series. I plan to

> replace the above text with the more elaborate description when I repost

> this patch series. For the current version of this patch series, please

> take a look at

> https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u


You should have pointed to the lore.kernel.org link in the commit logs
then, otherwise we have no way of knowing this when I get copied on
patch 93 of a 117 patch series :(

greg k-h
Bart Van Assche April 20, 2021, 4:12 p.m. | #6
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.
Bart Van Assche April 20, 2021, 4:44 p.m. | #7
On 4/20/21 7:36 AM, Chuck Lever III wrote:
>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote:

>> An explanation of the purpose of this patch is available in the patch

>> "scsi: Introduce the scsi_status union".

>>

>> Cc: "J. Bruce Fields" <bfields@fieldses.org>

>> Cc: Chuck Lever <chuck.lever@oracle.com>

>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> 

> Hi Bart, I assume this is going into v5.13 via the SCSI tree?

> Do you need an Acked-by: from the NFSD maintainers?


Hi Chuck,

Thanks for having taken a look. In case you would not yet have found the
"scsi: Introduce the scsi_status union" patch, it is available here:
https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u

An Acked-by or Reviewed-by from an NFS expert would be great.

The names in the Cc: list come from the following entry in the
MAINTAINERS file: "KERNEL NFSD, SUNRPC, AND LOCKD SERVERS".

Bart.
Hannes Reinecke April 20, 2021, 5:11 p.m. | #8
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
Douglas Gilbert April 20, 2021, 5:19 p.m. | #9
On 2021-04-20 12:12 p.m., 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).


The sg_io_v4 interface was specifically designed to _decouple_ the
user visible API from the kernel's 4-bytes-in-1-int representation.
So there are 4 levels of error reporting supported:
    1) from the kernel front-end: yield an errno
    2) from the driver (LLD): set driver_status
    3) from the transport: set transport status
    4) from the device (target or LU): set device_status

Those distinctions aren't that strict, there is some overlap (e.g.
timeouts). The sg version 3 interface (struct sg_io_hdr) is similar
but the names are less generic.

Can't remember if anyone every complained to me about not having
access to the message byte from SPI.

Doug Gilbert
Wei Liu April 20, 2021, 7:39 p.m. | #10
On Mon, Apr 19, 2021 at 07:13:40PM -0700, Bart Van Assche wrote:
> An explanation of the purpose of this patch is available in the patch

> "scsi: Introduce the scsi_status union".

> 

> Cc: "K. Y. Srinivasan" <kys@microsoft.com>

> Cc: Haiyang Zhang <haiyangz@microsoft.com>

> Cc: Stephen Hemminger <sthemmin@microsoft.com>

> Cc: Wei Liu <wei.liu@kernel.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>


If this is ever needed:

Acked-by: Wei Liu <wei.liu@kernel.org>
Bart Van Assche April 20, 2021, 9:10 p.m. | #11
On 4/20/21 10:11 AM, Hannes Reinecke wrote:
> On 4/20/21 6:12 PM, Bart Van Assche wrote:

>> - 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?


My patch series should not change the behavior of any SCSI LLD.
Additionally, most changes have been generated with the help of
Coccinelle. I think the risk of such changes is lower than modifying
SCSI LLDs such that the message byte is handled inside the LLD.

Thanks,

Bart.
Chuck Lever III April 21, 2021, 2:22 p.m. | #12
> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@acm.org> wrote:

> 

> On 4/20/21 7:36 AM, Chuck Lever III wrote:

>>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote:

>>> An explanation of the purpose of this patch is available in the patch

>>> "scsi: Introduce the scsi_status union".

>>> 

>>> Cc: "J. Bruce Fields" <bfields@fieldses.org>

>>> Cc: Chuck Lever <chuck.lever@oracle.com>

>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

>> 

>> Hi Bart, I assume this is going into v5.13 via the SCSI tree?

>> Do you need an Acked-by: from the NFSD maintainers?

> 

> Hi Chuck,

> 

> Thanks for having taken a look. In case you would not yet have found the

> "scsi: Introduce the scsi_status union" patch, it is available here:

> https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u

> 

> An Acked-by or Reviewed-by from an NFS expert would be great.


The NFSD patch looks OK to me, but I'm hesitating on sending
an Acked-by.

I went back and looked at the scsi_status union patch, and
that looks dodgy to me.

AFAIK, "enum" doesn't cause the compiler to reserve any
particular size of storage, it just makes a guess. What
keeps those enum fields from being 16- or 32-bits wide?
Shouldn't those be u8 to enforce the correct field size?

I'm not sure where to look for further discussion on that
part of the series.


> The names in the Cc: list come from the following entry in the

> MAINTAINERS file: "KERNEL NFSD, SUNRPC, AND LOCKD SERVERS".

> 

> Bart.


--
Chuck Lever
Bart Van Assche April 21, 2021, 4:12 p.m. | #13
On 4/21/21 7:22 AM, Chuck Lever III wrote:
> 

> 

>> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@acm.org> wrote:

>>

>> On 4/20/21 7:36 AM, Chuck Lever III wrote:

>>>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote:

>>>> An explanation of the purpose of this patch is available in the patch

>>>> "scsi: Introduce the scsi_status union".

>>>>

>>>> Cc: "J. Bruce Fields" <bfields@fieldses.org>

>>>> Cc: Chuck Lever <chuck.lever@oracle.com>

>>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

>>>

>>> Hi Bart, I assume this is going into v5.13 via the SCSI tree?

>>> Do you need an Acked-by: from the NFSD maintainers?

>>

>> Hi Chuck,

>>

>> Thanks for having taken a look. In case you would not yet have found the

>> "scsi: Introduce the scsi_status union" patch, it is available here:

>> https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u

>>

>> An Acked-by or Reviewed-by from an NFS expert would be great.

> 

> The NFSD patch looks OK to me, but I'm hesitating on sending

> an Acked-by.

> 

> I went back and looked at the scsi_status union patch, and

> that looks dodgy to me.

> 

> AFAIK, "enum" doesn't cause the compiler to reserve any

> particular size of storage, it just makes a guess. What

> keeps those enum fields from being 16- or 32-bits wide?

> Shouldn't those be u8 to enforce the correct field size?

> 

> I'm not sure where to look for further discussion on that

> part of the series.


Hi Chuck,

Although the C standard requires that enums have the same size as an 
int, gcc and clang support the attribute "packed" for enums. From the 
gcc documentation about the packed attribute: "When attached to an enum 
definition, it indicates that the smallest integral type should be used."

Additionally, the following BUILD_BUG_ON() statements verify the size 
and endianness of the members of the scsi_status union (see also 
https://www.spinics.net/lists/linux-scsi/msg157796.html):

+#define TEST_STATUS ((union scsi_status){.combined = 0x01020308})
+
  static int __init init_scsi(void)
  {
  	int error;

+	BUILD_BUG_ON(sizeof(union scsi_status) != 4);
+	BUILD_BUG_ON(TEST_STATUS.combined != 0x01020308);
+	BUILD_BUG_ON(driver_byte(TEST_STATUS) != 1);
+	BUILD_BUG_ON(host_byte(TEST_STATUS) != 2);
+	BUILD_BUG_ON(msg_byte(TEST_STATUS) != 3);
+	BUILD_BUG_ON(status_byte(TEST_STATUS) != 4);

Does this address your concern?

Bart.
Chuck Lever III April 21, 2021, 4:27 p.m. | #14
> On Apr 21, 2021, at 12:12 PM, Bart Van Assche <bvanassche@acm.org> wrote:

> 

> On 4/21/21 7:22 AM, Chuck Lever III wrote:

>>> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@acm.org> wrote:

>>> 

>>> On 4/20/21 7:36 AM, Chuck Lever III wrote:

>>>>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote:

>>>>> An explanation of the purpose of this patch is available in the patch

>>>>> "scsi: Introduce the scsi_status union".

>>>>> 

>>>>> Cc: "J. Bruce Fields" <bfields@fieldses.org>

>>>>> Cc: Chuck Lever <chuck.lever@oracle.com>

>>>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

>>>> 

>>>> Hi Bart, I assume this is going into v5.13 via the SCSI tree?

>>>> Do you need an Acked-by: from the NFSD maintainers?

>>> 

>>> Hi Chuck,

>>> 

>>> Thanks for having taken a look. In case you would not yet have found the

>>> "scsi: Introduce the scsi_status union" patch, it is available here:

>>> https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u

>>> 

>>> An Acked-by or Reviewed-by from an NFS expert would be great.

>> The NFSD patch looks OK to me, but I'm hesitating on sending

>> an Acked-by.

>> I went back and looked at the scsi_status union patch, and

>> that looks dodgy to me.

>> AFAIK, "enum" doesn't cause the compiler to reserve any

>> particular size of storage, it just makes a guess. What

>> keeps those enum fields from being 16- or 32-bits wide?

>> Shouldn't those be u8 to enforce the correct field size?

>> I'm not sure where to look for further discussion on that

>> part of the series.

> 

> Hi Chuck,

> 

> Although the C standard requires that enums have the same size as an int, gcc and clang support the attribute "packed" for enums. From the gcc documentation about the packed attribute: "When attached to an enum definition, it indicates that the smallest integral type should be used."

> 

> Additionally, the following BUILD_BUG_ON() statements verify the size and endianness of the members of the scsi_status union (see also https://www.spinics.net/lists/linux-scsi/msg157796.html):

> 

> +#define TEST_STATUS ((union scsi_status){.combined = 0x01020308})

> +

> static int __init init_scsi(void)

> {

> 	int error;

> 

> +	BUILD_BUG_ON(sizeof(union scsi_status) != 4);

> +	BUILD_BUG_ON(TEST_STATUS.combined != 0x01020308);

> +	BUILD_BUG_ON(driver_byte(TEST_STATUS) != 1);

> +	BUILD_BUG_ON(host_byte(TEST_STATUS) != 2);

> +	BUILD_BUG_ON(msg_byte(TEST_STATUS) != 3);

> +	BUILD_BUG_ON(status_byte(TEST_STATUS) != 4);

> 

> Does this address your concern?


Yes: a compile-time check that these assumptions are being
met is good enough for me.

Acked-by: Chuck Lever <chuck.lever@oracle.com>


--
Chuck Lever
Lee Duncan May 6, 2021, 4:52 p.m. | #15
On 4/19/21 5:06 PM, Bart Van Assche wrote:
> Before modifying the struct iscsi_bsg_reply definition, add a compile-time

> structure size check.

> 

> Cc: Lee Duncan <lduncan@suse.com>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  drivers/scsi/scsi_transport_iscsi.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c

> index bebfb355abdf..4f821118ea23 100644

> --- a/drivers/scsi/scsi_transport_iscsi.c

> +++ b/drivers/scsi/scsi_transport_iscsi.c

> @@ -4729,6 +4729,9 @@ static __init int iscsi_transport_init(void)

>  		.groups	= 1,

>  		.input	= iscsi_if_rx,

>  	};

> +

> +	BUILD_BUG_ON(offsetof(struct iscsi_bsg_reply, reply_data) != 8);

> +

>  	printk(KERN_INFO "Loading iSCSI transport class v%s.\n",

>  		ISCSI_TRANSPORT_VERSION);

>  

> 


Reviewed-by: Lee Duncan <lduncan@suse.com>
Lee Duncan May 6, 2021, 5:04 p.m. | #16
On 4/19/21 5:06 PM, Bart Van Assche wrote:
> Introduce the scsi_status union, a data structure that will be used in the

> next patches to replace SCSI status codes represented as an integer. Define

> that data structure as follows:

> 

> 	union scsi_status {

> 		int32_t combined;

> 		struct {

> 	#if defined(__BIG_ENDIAN)

> 			enum driver_status driver;

> 			enum host_status host;

> 			enum msg_byte msg;

> 			enum sam_status status;

> 	#elif defined(__LITTLE_ENDIAN)

> 			enum sam_status status;

> 			enum msg_byte msg;

> 			enum host_status host;

> 			enum driver_status driver;

> 	#else

> 	#error Endianness?

> 	#endif

> 		} b;

> 	};

> 

> The 'combined' member makes it easy to convert existing SCSI code. The

> 'status', 'msg', 'host' and 'driver' enable access of individual SCSI

> status fields in a type-safe fashion.

> 

> Change 'int result;' into the following to enable converting one driver at

> a time:

> 

> 	union {

> 		int result;

> 		union scsi_status status;

> 	};

> 

> A later patch will remove the outer union and 'int result;'.

> 

> Also to enable converting one driver at a time, make scsi_status_is_good(),

> status_byte(), msg_byte(), host_byte() and driver_byte() accept an int or

> union scsi_status as argument. A later patch will make this function and

> these macros accept the scsi_status union only.

> 

> Cc: Christoph Hellwig <hch@lst.de>

> Cc: Ming Lei <ming.lei@redhat.com>

> Cc: Hannes Reinecke <hare@suse.com>

> Cc: John Garry <john.garry@huawei.com>

> Cc: Can Guo <cang@codeaurora.org>

> Cc: James Smart <james.smart@broadcom.com>

> Cc: Lee Duncan <lduncan@suse.com>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  drivers/scsi/scsi.c              |  9 +++++++++

>  include/linux/bsg-lib.h          |  6 +++++-

>  include/scsi/scsi.h              | 24 +++++++++++++++++++-----

>  include/scsi/scsi_bsg_iscsi.h    |  6 +++++-

>  include/scsi/scsi_cmnd.h         | 14 +++++++++-----

>  include/scsi/scsi_eh.h           |  7 ++++++-

>  include/scsi/scsi_request.h      |  6 +++++-

>  include/scsi/scsi_status.h       | 29 +++++++++++++++++++++++++++++

>  include/uapi/scsi/scsi_bsg_fc.h  | 10 ++++++++++

>  include/uapi/scsi/scsi_bsg_ufs.h | 11 +++++++++++

>  10 files changed, 108 insertions(+), 14 deletions(-)

> 

> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c

> index e9e2f0e15ac8..4f71f2005be4 100644

> --- a/drivers/scsi/scsi.c

> +++ b/drivers/scsi/scsi.c

> @@ -763,10 +763,19 @@ MODULE_LICENSE("GPL");

>  module_param(scsi_logging_level, int, S_IRUGO|S_IWUSR);

>  MODULE_PARM_DESC(scsi_logging_level, "a bit mask of logging levels");

>  

> +#define TEST_STATUS ((union scsi_status){.combined = 0x01020308})

> +

>  static int __init init_scsi(void)

>  {

>  	int error;

>  

> +	BUILD_BUG_ON(sizeof(union scsi_status) != 4);

> +	BUILD_BUG_ON(TEST_STATUS.combined != 0x01020308);

> +	BUILD_BUG_ON(driver_byte(TEST_STATUS) != 1);

> +	BUILD_BUG_ON(host_byte(TEST_STATUS) != 2);

> +	BUILD_BUG_ON(msg_byte(TEST_STATUS) != 3);

> +	BUILD_BUG_ON(status_byte(TEST_STATUS) != 4);

> +

>  	error = scsi_init_procfs();

>  	if (error)

>  		goto cleanup_queue;

> diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h

> index 960988d42f77..f934afc45760 100644

> --- a/include/linux/bsg-lib.h

> +++ b/include/linux/bsg-lib.h

> @@ -11,6 +11,7 @@

>  

>  #include <linux/blkdev.h>

>  #include <scsi/scsi_request.h>

> +#include <scsi/scsi_status.h>

>  

>  struct request;

>  struct device;

> @@ -52,7 +53,10 @@ struct bsg_job {

>  	struct bsg_buffer request_payload;

>  	struct bsg_buffer reply_payload;

>  

> -	int result;

> +	union {

> +		int		  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  	unsigned int reply_payload_rcv_len;

>  

>  	/* BIDI support */

> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h

> index c9ccb6b45b76..18bb1fb2458f 100644

> --- a/include/scsi/scsi.h

> +++ b/include/scsi/scsi.h

> @@ -39,7 +39,7 @@ enum scsi_timeouts {

>   * This returns true for known good conditions that may be treated as

>   * command completed normally

>   */

> -static inline int scsi_status_is_good(int status)

> +static inline bool __scsi_status_is_good(int status)

>  {

>  	/*

>  	 * FIXME: bit0 is listed as reserved in SCSI-2, but is

> @@ -56,6 +56,20 @@ static inline int scsi_status_is_good(int status)

>  		(status == SAM_STAT_COMMAND_TERMINATED));

>  }

>  

> +/*

> + * If the 'status' argument has type int, unsigned int or union scsi_status,

> + * return the combined SCSI status. If the 'status' argument has another type,

> + * trigger a compiler error by passing a struct to a context where an integer

> + * is expected.

> + */

> +#define scsi_status_to_int(status)			\

> +	__builtin_choose_expr(sizeof(status) == 4,	\

> +			      *(int32_t *)&(status),	\

> +			      (union scsi_status){})

> +

> +#define scsi_status_is_good(status)				\

> +	__scsi_status_is_good(scsi_status_to_int(status))

> +

>  

>  /*

>   * standard mode-select header prepended to all mode-select commands

> @@ -134,10 +148,10 @@ enum scsi_disposition {

>   *      driver_byte = set by mid-level.

>   */

>  #define status_byte(result) ((enum sam_status_divided_by_two)	\

> -			     (((result) >> 1) & 0x7f))

> -#define msg_byte(result)    (((result) >> 8) & 0xff)

> -#define host_byte(result)   (((result) >> 16) & 0xff)

> -#define driver_byte(result) (((result) >> 24) & 0xff)

> +			     ((scsi_status_to_int((result)) >> 1) & 0x7f))

> +#define msg_byte(result)    ((scsi_status_to_int((result)) >> 8) & 0xff)

> +#define host_byte(result)   ((scsi_status_to_int((result)) >> 16) & 0xff)

> +#define driver_byte(result) ((scsi_status_to_int((result)) >> 24) & 0xff)

>  

>  #define sense_class(sense)  (((sense) >> 4) & 0x7)

>  #define sense_error(sense)  ((sense) & 0xf)

> diff --git a/include/scsi/scsi_bsg_iscsi.h b/include/scsi/scsi_bsg_iscsi.h

> index 6b8128005af8..d18e7e061927 100644

> --- a/include/scsi/scsi_bsg_iscsi.h

> +++ b/include/scsi/scsi_bsg_iscsi.h

> @@ -13,6 +13,7 @@

>   */

>  

>  #include <scsi/scsi.h>

> +#include <scsi/scsi_status.h>

>  

>  /*

>   * iSCSI Transport SGIO v4 BSG Message Support

> @@ -82,7 +83,10 @@ struct iscsi_bsg_reply {

>  	 * msg and status fields. The per-msgcode reply structure

>  	 * will contain valid data.

>  	 */

> -	uint32_t result;

> +	union {

> +		uint32_t	  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  

>  	/* If there was reply_payload, how much was recevied ? */

>  	uint32_t reply_payload_rcv_len;

> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h

> index 202106e7c814..539be97b0a7d 100644

> --- a/include/scsi/scsi_cmnd.h

> +++ b/include/scsi/scsi_cmnd.h

> @@ -140,7 +140,11 @@ struct scsi_cmnd {

>  					 * obtained by scsi_malloc is guaranteed

>  					 * to be at an address < 16Mb). */

>  

> -	int result;		/* Status code from lower level driver */

> +	/* Status code from lower level driver */

> +	union {

> +		int		  result; /* do not use in new code. */

> +		union scsi_status status;

> +	};

>  	int flags;		/* Command flags */

>  	unsigned long state;	/* Command completion state */

>  

> @@ -317,23 +321,23 @@ static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)

>  static inline void set_status_byte(struct scsi_cmnd *cmd,

>  				   enum sam_status status)

>  {

> -	cmd->result = (cmd->result & 0xffffff00) | status;

> +	cmd->status.b.status = status;

>  }

>  

>  static inline void set_msg_byte(struct scsi_cmnd *cmd, enum msg_byte status)

>  {

> -	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);

> +	cmd->status.b.msg = status;

>  }

>  

>  static inline void set_host_byte(struct scsi_cmnd *cmd, enum host_status status)

>  {

> -	cmd->result = (cmd->result & 0xff00ffff) | (status << 16);

> +	cmd->status.b.host = status;

>  }

>  

>  static inline void set_driver_byte(struct scsi_cmnd *cmd,

>  				   enum driver_status status)

>  {

> -	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);

> +	cmd->status.b.driver = status;

>  }

>  

>  static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)

> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h

> index 468094254b3c..dcd66bb9a1ba 100644

> --- a/include/scsi/scsi_eh.h

> +++ b/include/scsi/scsi_eh.h

> @@ -6,6 +6,8 @@

>  

>  #include <scsi/scsi_cmnd.h>

>  #include <scsi/scsi_common.h>

> +#include <scsi/scsi_status.h>

> +

>  struct scsi_device;

>  struct Scsi_Host;

>  

> @@ -31,7 +33,10 @@ extern int scsi_ioctl_reset(struct scsi_device *, int __user *);

>  

>  struct scsi_eh_save {

>  	/* saved state */

> -	int result;

> +	union {

> +		int		  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  	unsigned int resid_len;

>  	int eh_eflags;

>  	enum dma_data_direction data_direction;

> diff --git a/include/scsi/scsi_request.h b/include/scsi/scsi_request.h

> index b06f28c74908..83f5549cc74c 100644

> --- a/include/scsi/scsi_request.h

> +++ b/include/scsi/scsi_request.h

> @@ -3,6 +3,7 @@

>  #define _SCSI_SCSI_REQUEST_H

>  

>  #include <linux/blk-mq.h>

> +#include <scsi/scsi_status.h>

>  

>  #define BLK_MAX_CDB	16

>  

> @@ -10,7 +11,10 @@ struct scsi_request {

>  	unsigned char	__cmd[BLK_MAX_CDB];

>  	unsigned char	*cmd;

>  	unsigned short	cmd_len;

> -	int		result;

> +	union {

> +		int		  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  	unsigned int	sense_len;

>  	unsigned int	resid_len;	/* residual count */

>  	int		retries;

> diff --git a/include/scsi/scsi_status.h b/include/scsi/scsi_status.h

> index da2ba825f981..120f5a43d2ed 100644

> --- a/include/scsi/scsi_status.h

> +++ b/include/scsi/scsi_status.h

> @@ -3,6 +3,7 @@

>  

>  #include <linux/types.h>

>  #include <linux/compiler_attributes.h>

> +#include <asm/byteorder.h>

>  #include <scsi/scsi_proto.h>

>  

>  /*

> @@ -88,4 +89,32 @@ enum driver_status {

>  	DRIVER_SENSE	= 0x08,

>  } __packed;

>  

> +/**

> + * SCSI status passed by LLDs to the midlayer.

> + * @combined: One of the following:

> + *	- A (driver, host, msg, status) quadruplet encoded as a 32-bit integer.

> + *	- A negative Unix error code.

> + *	- In the IDE code, a positive value that represents an error code, an

> + *	  error counter or a bitfield.

> + * @b: SCSI status code.

> + */

> +union scsi_status {

> +	int32_t combined;

> +	struct {

> +#if defined(__BIG_ENDIAN)

> +		enum driver_status driver;

> +		enum host_status host;

> +		enum msg_byte msg;

> +		enum sam_status status;

> +#elif defined(__LITTLE_ENDIAN)

> +		enum sam_status status;

> +		enum msg_byte msg;

> +		enum host_status host;

> +		enum driver_status driver;

> +#else

> +#error Endianness?

> +#endif

> +	} b;

> +};

> +

>  #endif /* _SCSI_SCSI_STATUS_H */

> diff --git a/include/uapi/scsi/scsi_bsg_fc.h b/include/uapi/scsi/scsi_bsg_fc.h

> index 3ae65e93235c..419db719fe8e 100644

> --- a/include/uapi/scsi/scsi_bsg_fc.h

> +++ b/include/uapi/scsi/scsi_bsg_fc.h

> @@ -9,6 +9,9 @@

>  #define SCSI_BSG_FC_H

>  

>  #include <linux/types.h>

> +#ifdef __KERNEL__

> +#include <scsi/scsi_status.h>

> +#endif

>  

>  /*

>   * This file intended to be included by both kernel and user space

> @@ -291,7 +294,14 @@ struct fc_bsg_reply {

>  	 *    msg and status fields. The per-msgcode reply structure

>  	 *    will contain valid data.

>  	 */

> +#ifdef __KERNEL__

> +	union {

> +		__u32		  result; /* do not use in new kernel code */

> +		union scsi_status status;

> +	};

> +#else

>  	__u32 result;

> +#endif

>  

>  	/* If there was reply_payload, how much was recevied ? */

>  	__u32 reply_payload_rcv_len;

> diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h

> index d55f2176dfd4..3dfe5a5a0842 100644

> --- a/include/uapi/scsi/scsi_bsg_ufs.h

> +++ b/include/uapi/scsi/scsi_bsg_ufs.h

> @@ -9,6 +9,10 @@

>  #define SCSI_BSG_UFS_H

>  

>  #include <linux/types.h>

> +#ifdef __KERNEL__

> +#include <scsi/scsi_status.h>

> +#endif

> +

>  /*

>   * This file intended to be included by both kernel and user space

>   */

> @@ -95,7 +99,14 @@ struct ufs_bsg_reply {

>  	 * msg and status fields. The per-msgcode reply structure

>  	 * will contain valid data.

>  	 */

> +#ifdef __KERNEL__

> +	union {

> +		__u32		  result; /* do not use in new kernel code */

> +		union scsi_status status;

> +	};

> +#else

>  	__u32 result;

> +#endif

>  

>  	/* If there was reply_payload, how much was received? */

>  	__u32 reply_payload_rcv_len;

> 



Reviewed-by: Lee Duncan <lduncan@suse.com>
Lee Duncan May 6, 2021, 6:55 p.m. | #17
On 4/19/21 7:13 PM, Bart Van Assche wrote:
> A previous patch changed 'int result;' into a union and introduced the

> scsi_status member in that union. Now that the conversion from type

> 'int' into 'union scsi_status' is complete, remove the 'result' member.

> 

> Cc: Christoph Hellwig <hch@lst.de>

> Cc: Ming Lei <ming.lei@redhat.com>

> Cc: Hannes Reinecke <hare@suse.de>

> Cc: John Garry <john.garry@huawei.com>

> Cc: Lee Duncan <lduncan@suse.com>

> Cc: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  include/linux/bsg-lib.h          | 5 +----

>  include/scsi/scsi_bsg_iscsi.h    | 7 ++-----

>  include/scsi/scsi_cmnd.h         | 5 +----

>  include/scsi/scsi_eh.h           | 5 +----

>  include/scsi/scsi_request.h      | 5 +----

>  include/uapi/scsi/scsi_bsg_fc.h  | 5 +----

>  include/uapi/scsi/scsi_bsg_ufs.h | 7 ++-----

>  7 files changed, 9 insertions(+), 30 deletions(-)

> 

> diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h

> index f934afc45760..6143a54454db 100644

> --- a/include/linux/bsg-lib.h

> +++ b/include/linux/bsg-lib.h

> @@ -53,10 +53,7 @@ struct bsg_job {

>  	struct bsg_buffer request_payload;

>  	struct bsg_buffer reply_payload;

>  

> -	union {

> -		int		  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	unsigned int reply_payload_rcv_len;

>  

>  	/* BIDI support */

> diff --git a/include/scsi/scsi_bsg_iscsi.h b/include/scsi/scsi_bsg_iscsi.h

> index d18e7e061927..e384df88fab1 100644

> --- a/include/scsi/scsi_bsg_iscsi.h

> +++ b/include/scsi/scsi_bsg_iscsi.h

> @@ -76,17 +76,14 @@ struct iscsi_bsg_request {

>  /* response (request sense data) structure of the sg_io_v4 */

>  struct iscsi_bsg_reply {

>  	/*

> -	 * The completion result. Result exists in two forms:

> +	 * The completion status. Result exists in two forms:

>  	 * if negative, it is an -Exxx system errno value. There will

>  	 * be no further reply information supplied.

>  	 * else, it's the 4-byte scsi error result, with driver, host,

>  	 * msg and status fields. The per-msgcode reply structure

>  	 * will contain valid data.

>  	 */

> -	union {

> -		uint32_t	  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  

>  	/* If there was reply_payload, how much was recevied ? */

>  	uint32_t reply_payload_rcv_len;

> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h

> index 539be97b0a7d..b616e1d8af9a 100644

> --- a/include/scsi/scsi_cmnd.h

> +++ b/include/scsi/scsi_cmnd.h

> @@ -141,10 +141,7 @@ struct scsi_cmnd {

>  					 * to be at an address < 16Mb). */

>  

>  	/* Status code from lower level driver */

> -	union {

> -		int		  result; /* do not use in new code. */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	int flags;		/* Command flags */

>  	unsigned long state;	/* Command completion state */

>  

> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h

> index dcd66bb9a1ba..406e22887d96 100644

> --- a/include/scsi/scsi_eh.h

> +++ b/include/scsi/scsi_eh.h

> @@ -33,10 +33,7 @@ extern int scsi_ioctl_reset(struct scsi_device *, int __user *);

>  

>  struct scsi_eh_save {

>  	/* saved state */

> -	union {

> -		int		  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	unsigned int resid_len;

>  	int eh_eflags;

>  	enum dma_data_direction data_direction;

> diff --git a/include/scsi/scsi_request.h b/include/scsi/scsi_request.h

> index 83f5549cc74c..41b8f9f6a349 100644

> --- a/include/scsi/scsi_request.h

> +++ b/include/scsi/scsi_request.h

> @@ -11,10 +11,7 @@ struct scsi_request {

>  	unsigned char	__cmd[BLK_MAX_CDB];

>  	unsigned char	*cmd;

>  	unsigned short	cmd_len;

> -	union {

> -		int		  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	unsigned int	sense_len;

>  	unsigned int	resid_len;	/* residual count */

>  	int		retries;

> diff --git a/include/uapi/scsi/scsi_bsg_fc.h b/include/uapi/scsi/scsi_bsg_fc.h

> index 419db719fe8e..6d095aefc265 100644

> --- a/include/uapi/scsi/scsi_bsg_fc.h

> +++ b/include/uapi/scsi/scsi_bsg_fc.h

> @@ -295,10 +295,7 @@ struct fc_bsg_reply {

>  	 *    will contain valid data.

>  	 */

>  #ifdef __KERNEL__

> -	union {

> -		__u32		  result; /* do not use in new kernel code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  #else

>  	__u32 result;

>  #endif

> diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h

> index 3dfe5a5a0842..1c282ab144f6 100644

> --- a/include/uapi/scsi/scsi_bsg_ufs.h

> +++ b/include/uapi/scsi/scsi_bsg_ufs.h

> @@ -92,7 +92,7 @@ struct ufs_bsg_request {

>  /* response (request sense data) structure of the sg_io_v4 */

>  struct ufs_bsg_reply {

>  	/*

> -	 * The completion result. Result exists in two forms:

> +	 * The completion status. Exists in two forms:

>  	 * if negative, it is an -Exxx system errno value. There will

>  	 * be no further reply information supplied.

>  	 * else, it's the 4-byte scsi error result, with driver, host,

> @@ -100,10 +100,7 @@ struct ufs_bsg_reply {

>  	 * will contain valid data.

>  	 */

>  #ifdef __KERNEL__

> -	union {

> -		__u32		  result; /* do not use in new kernel code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  #else

>  	__u32 result;

>  #endif

> 


Reviewed-by: Lee Duncan <lduncan@suse.com>
Can Guo May 6, 2021, 11:56 p.m. | #18
On 2021-04-20 10:13, Bart Van Assche wrote:
> A later patch will introduce a type with the name 'scsi_status'. Remove

> a local variable with the same name to prevent confusion. This patch

> does not change any functionality.

> 

> Cc: Can Guo <cang@codeaurora.org>

> Cc: Avri Altman <avri.altman@wdc.com>

> Cc: Bean Huo <beanhuo@micron.com>

> Cc: Alim Akhtar <alim.akhtar@samsung.com>

> Cc: Asutosh Das <asutoshd@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  drivers/scsi/ufs/ufshcd.c | 17 ++++-------------

>  1 file changed, 4 insertions(+), 13 deletions(-)

> 

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c

> index fa596cf66c23..0c2c18f2acf3 100644

> --- a/drivers/scsi/ufs/ufshcd.c

> +++ b/drivers/scsi/ufs/ufshcd.c

> @@ -4934,7 +4934,6 @@ static inline int

>  ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb 

> *lrbp)

>  {

>  	int result = 0;

> -	int scsi_status;

>  	int ocs;

> 

>  	/* overall command status of utrd */

> @@ -4952,18 +4951,10 @@ ufshcd_transfer_rsp_status(struct ufs_hba

> *hba, struct ufshcd_lrb *lrbp)

>  		hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0);

>  		switch (result) {

>  		case UPIU_TRANSACTION_RESPONSE:

> -			/*

> -			 * get the response UPIU result to extract

> -			 * the SCSI command status

> -			 */

> -			result = ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr);

> -

> -			/*

> -			 * get the result based on SCSI status response

> -			 * to notify the SCSI midlayer of the command status

> -			 */

> -			scsi_status = result & MASK_SCSI_STATUS;

> -			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);

> +			/* Propagate the SCSI status to the SCSI midlayer. */

> +			result = ufshcd_scsi_cmd_status(lrbp,

> +				ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr) &

> +				MASK_SCSI_STATUS);

> 

>  			/*

>  			 * Currently we are only supporting BKOPs exception


Reviewed-by: Can Guo <cang@codeaurora.org>
Can Guo May 6, 2021, 11:57 p.m. | #19
On 2021-04-20 10:13, Bart Van Assche wrote:
> The 'scsi_status' member is present since the introduction of the UFS

> driver but has never been used. Hence remove it.

> 

> Cc: Can Guo <cang@codeaurora.org>

> Cc: Avri Altman <avri.altman@wdc.com>

> Cc: Bean Huo <beanhuo@micron.com>

> Cc: Alim Akhtar <alim.akhtar@samsung.com>

> Cc: Asutosh Das <asutoshd@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  drivers/scsi/ufs/ufshcd.h | 1 -

>  1 file changed, 1 deletion(-)

> 

> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h

> index 5eb66a8debc7..20ad78083246 100644

> --- a/drivers/scsi/ufs/ufshcd.h

> +++ b/drivers/scsi/ufs/ufshcd.h

> @@ -211,7 +211,6 @@ struct ufshcd_lrb {

>  	struct scsi_cmnd *cmd;

>  	u8 *sense_buffer;

>  	unsigned int sense_bufflen;

> -	int scsi_status;

> 

>  	int command_type;

>  	int task_tag;


Reviewed-by: Can Guo <cang@codeaurora.org>
Can Guo May 7, 2021, 12:04 a.m. | #20
On 2021-04-20 08:06, Bart Van Assche wrote:
> Introduce the scsi_status union, a data structure that will be used in 

> the

> next patches to replace SCSI status codes represented as an integer. 

> Define

> that data structure as follows:

> 

> 	union scsi_status {

> 		int32_t combined;

> 		struct {

> 	#if defined(__BIG_ENDIAN)

> 			enum driver_status driver;

> 			enum host_status host;

> 			enum msg_byte msg;

> 			enum sam_status status;

> 	#elif defined(__LITTLE_ENDIAN)

> 			enum sam_status status;

> 			enum msg_byte msg;

> 			enum host_status host;

> 			enum driver_status driver;

> 	#else

> 	#error Endianness?

> 	#endif

> 		} b;

> 	};

> 

> The 'combined' member makes it easy to convert existing SCSI code. The

> 'status', 'msg', 'host' and 'driver' enable access of individual SCSI

> status fields in a type-safe fashion.

> 

> Change 'int result;' into the following to enable converting one driver 

> at

> a time:

> 

> 	union {

> 		int result;

> 		union scsi_status status;

> 	};

> 

> A later patch will remove the outer union and 'int result;'.

> 

> Also to enable converting one driver at a time, make 

> scsi_status_is_good(),

> status_byte(), msg_byte(), host_byte() and driver_byte() accept an int 

> or

> union scsi_status as argument. A later patch will make this function 

> and

> these macros accept the scsi_status union only.

> 

> Cc: Christoph Hellwig <hch@lst.de>

> Cc: Ming Lei <ming.lei@redhat.com>

> Cc: Hannes Reinecke <hare@suse.com>

> Cc: John Garry <john.garry@huawei.com>

> Cc: Can Guo <cang@codeaurora.org>

> Cc: James Smart <james.smart@broadcom.com>

> Cc: Lee Duncan <lduncan@suse.com>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  drivers/scsi/scsi.c              |  9 +++++++++

>  include/linux/bsg-lib.h          |  6 +++++-

>  include/scsi/scsi.h              | 24 +++++++++++++++++++-----

>  include/scsi/scsi_bsg_iscsi.h    |  6 +++++-

>  include/scsi/scsi_cmnd.h         | 14 +++++++++-----

>  include/scsi/scsi_eh.h           |  7 ++++++-

>  include/scsi/scsi_request.h      |  6 +++++-

>  include/scsi/scsi_status.h       | 29 +++++++++++++++++++++++++++++

>  include/uapi/scsi/scsi_bsg_fc.h  | 10 ++++++++++

>  include/uapi/scsi/scsi_bsg_ufs.h | 11 +++++++++++

>  10 files changed, 108 insertions(+), 14 deletions(-)

> 

> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c

> index e9e2f0e15ac8..4f71f2005be4 100644

> --- a/drivers/scsi/scsi.c

> +++ b/drivers/scsi/scsi.c

> @@ -763,10 +763,19 @@ MODULE_LICENSE("GPL");

>  module_param(scsi_logging_level, int, S_IRUGO|S_IWUSR);

>  MODULE_PARM_DESC(scsi_logging_level, "a bit mask of logging levels");

> 

> +#define TEST_STATUS ((union scsi_status){.combined = 0x01020308})

> +

>  static int __init init_scsi(void)

>  {

>  	int error;

> 

> +	BUILD_BUG_ON(sizeof(union scsi_status) != 4);

> +	BUILD_BUG_ON(TEST_STATUS.combined != 0x01020308);

> +	BUILD_BUG_ON(driver_byte(TEST_STATUS) != 1);

> +	BUILD_BUG_ON(host_byte(TEST_STATUS) != 2);

> +	BUILD_BUG_ON(msg_byte(TEST_STATUS) != 3);

> +	BUILD_BUG_ON(status_byte(TEST_STATUS) != 4);

> +

>  	error = scsi_init_procfs();

>  	if (error)

>  		goto cleanup_queue;

> diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h

> index 960988d42f77..f934afc45760 100644

> --- a/include/linux/bsg-lib.h

> +++ b/include/linux/bsg-lib.h

> @@ -11,6 +11,7 @@

> 

>  #include <linux/blkdev.h>

>  #include <scsi/scsi_request.h>

> +#include <scsi/scsi_status.h>

> 

>  struct request;

>  struct device;

> @@ -52,7 +53,10 @@ struct bsg_job {

>  	struct bsg_buffer request_payload;

>  	struct bsg_buffer reply_payload;

> 

> -	int result;

> +	union {

> +		int		  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  	unsigned int reply_payload_rcv_len;

> 

>  	/* BIDI support */

> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h

> index c9ccb6b45b76..18bb1fb2458f 100644

> --- a/include/scsi/scsi.h

> +++ b/include/scsi/scsi.h

> @@ -39,7 +39,7 @@ enum scsi_timeouts {

>   * This returns true for known good conditions that may be treated as

>   * command completed normally

>   */

> -static inline int scsi_status_is_good(int status)

> +static inline bool __scsi_status_is_good(int status)

>  {

>  	/*

>  	 * FIXME: bit0 is listed as reserved in SCSI-2, but is

> @@ -56,6 +56,20 @@ static inline int scsi_status_is_good(int status)

>  		(status == SAM_STAT_COMMAND_TERMINATED));

>  }

> 

> +/*

> + * If the 'status' argument has type int, unsigned int or union 

> scsi_status,

> + * return the combined SCSI status. If the 'status' argument has 

> another type,

> + * trigger a compiler error by passing a struct to a context where an 

> integer

> + * is expected.

> + */

> +#define scsi_status_to_int(status)			\

> +	__builtin_choose_expr(sizeof(status) == 4,	\

> +			      *(int32_t *)&(status),	\

> +			      (union scsi_status){})

> +

> +#define scsi_status_is_good(status)				\

> +	__scsi_status_is_good(scsi_status_to_int(status))

> +

> 

>  /*

>   * standard mode-select header prepended to all mode-select commands

> @@ -134,10 +148,10 @@ enum scsi_disposition {

>   *      driver_byte = set by mid-level.

>   */

>  #define status_byte(result) ((enum sam_status_divided_by_two)	\

> -			     (((result) >> 1) & 0x7f))

> -#define msg_byte(result)    (((result) >> 8) & 0xff)

> -#define host_byte(result)   (((result) >> 16) & 0xff)

> -#define driver_byte(result) (((result) >> 24) & 0xff)

> +			     ((scsi_status_to_int((result)) >> 1) & 0x7f))

> +#define msg_byte(result)    ((scsi_status_to_int((result)) >> 8) & 

> 0xff)

> +#define host_byte(result)   ((scsi_status_to_int((result)) >> 16) & 

> 0xff)

> +#define driver_byte(result) ((scsi_status_to_int((result)) >> 24) & 

> 0xff)

> 

>  #define sense_class(sense)  (((sense) >> 4) & 0x7)

>  #define sense_error(sense)  ((sense) & 0xf)

> diff --git a/include/scsi/scsi_bsg_iscsi.h 

> b/include/scsi/scsi_bsg_iscsi.h

> index 6b8128005af8..d18e7e061927 100644

> --- a/include/scsi/scsi_bsg_iscsi.h

> +++ b/include/scsi/scsi_bsg_iscsi.h

> @@ -13,6 +13,7 @@

>   */

> 

>  #include <scsi/scsi.h>

> +#include <scsi/scsi_status.h>

> 

>  /*

>   * iSCSI Transport SGIO v4 BSG Message Support

> @@ -82,7 +83,10 @@ struct iscsi_bsg_reply {

>  	 * msg and status fields. The per-msgcode reply structure

>  	 * will contain valid data.

>  	 */

> -	uint32_t result;

> +	union {

> +		uint32_t	  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

> 

>  	/* If there was reply_payload, how much was recevied ? */

>  	uint32_t reply_payload_rcv_len;

> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h

> index 202106e7c814..539be97b0a7d 100644

> --- a/include/scsi/scsi_cmnd.h

> +++ b/include/scsi/scsi_cmnd.h

> @@ -140,7 +140,11 @@ struct scsi_cmnd {

>  					 * obtained by scsi_malloc is guaranteed

>  					 * to be at an address < 16Mb). */

> 

> -	int result;		/* Status code from lower level driver */

> +	/* Status code from lower level driver */

> +	union {

> +		int		  result; /* do not use in new code. */

> +		union scsi_status status;

> +	};

>  	int flags;		/* Command flags */

>  	unsigned long state;	/* Command completion state */

> 

> @@ -317,23 +321,23 @@ static inline struct scsi_data_buffer

> *scsi_prot(struct scsi_cmnd *cmd)

>  static inline void set_status_byte(struct scsi_cmnd *cmd,

>  				   enum sam_status status)

>  {

> -	cmd->result = (cmd->result & 0xffffff00) | status;

> +	cmd->status.b.status = status;

>  }

> 

>  static inline void set_msg_byte(struct scsi_cmnd *cmd, enum msg_byte 

> status)

>  {

> -	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);

> +	cmd->status.b.msg = status;

>  }

> 

>  static inline void set_host_byte(struct scsi_cmnd *cmd, enum

> host_status status)

>  {

> -	cmd->result = (cmd->result & 0xff00ffff) | (status << 16);

> +	cmd->status.b.host = status;

>  }

> 

>  static inline void set_driver_byte(struct scsi_cmnd *cmd,

>  				   enum driver_status status)

>  {

> -	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);

> +	cmd->status.b.driver = status;

>  }

> 

>  static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)

> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h

> index 468094254b3c..dcd66bb9a1ba 100644

> --- a/include/scsi/scsi_eh.h

> +++ b/include/scsi/scsi_eh.h

> @@ -6,6 +6,8 @@

> 

>  #include <scsi/scsi_cmnd.h>

>  #include <scsi/scsi_common.h>

> +#include <scsi/scsi_status.h>

> +

>  struct scsi_device;

>  struct Scsi_Host;

> 

> @@ -31,7 +33,10 @@ extern int scsi_ioctl_reset(struct scsi_device *,

> int __user *);

> 

>  struct scsi_eh_save {

>  	/* saved state */

> -	int result;

> +	union {

> +		int		  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  	unsigned int resid_len;

>  	int eh_eflags;

>  	enum dma_data_direction data_direction;

> diff --git a/include/scsi/scsi_request.h b/include/scsi/scsi_request.h

> index b06f28c74908..83f5549cc74c 100644

> --- a/include/scsi/scsi_request.h

> +++ b/include/scsi/scsi_request.h

> @@ -3,6 +3,7 @@

>  #define _SCSI_SCSI_REQUEST_H

> 

>  #include <linux/blk-mq.h>

> +#include <scsi/scsi_status.h>

> 

>  #define BLK_MAX_CDB	16

> 

> @@ -10,7 +11,10 @@ struct scsi_request {

>  	unsigned char	__cmd[BLK_MAX_CDB];

>  	unsigned char	*cmd;

>  	unsigned short	cmd_len;

> -	int		result;

> +	union {

> +		int		  result; /* do not use in new code */

> +		union scsi_status status;

> +	};

>  	unsigned int	sense_len;

>  	unsigned int	resid_len;	/* residual count */

>  	int		retries;

> diff --git a/include/scsi/scsi_status.h b/include/scsi/scsi_status.h

> index da2ba825f981..120f5a43d2ed 100644

> --- a/include/scsi/scsi_status.h

> +++ b/include/scsi/scsi_status.h

> @@ -3,6 +3,7 @@

> 

>  #include <linux/types.h>

>  #include <linux/compiler_attributes.h>

> +#include <asm/byteorder.h>

>  #include <scsi/scsi_proto.h>

> 

>  /*

> @@ -88,4 +89,32 @@ enum driver_status {

>  	DRIVER_SENSE	= 0x08,

>  } __packed;

> 

> +/**

> + * SCSI status passed by LLDs to the midlayer.

> + * @combined: One of the following:

> + *	- A (driver, host, msg, status) quadruplet encoded as a 32-bit 

> integer.

> + *	- A negative Unix error code.

> + *	- In the IDE code, a positive value that represents an error code, 

> an

> + *	  error counter or a bitfield.

> + * @b: SCSI status code.

> + */

> +union scsi_status {

> +	int32_t combined;

> +	struct {

> +#if defined(__BIG_ENDIAN)

> +		enum driver_status driver;

> +		enum host_status host;

> +		enum msg_byte msg;

> +		enum sam_status status;

> +#elif defined(__LITTLE_ENDIAN)

> +		enum sam_status status;

> +		enum msg_byte msg;

> +		enum host_status host;

> +		enum driver_status driver;

> +#else

> +#error Endianness?

> +#endif

> +	} b;

> +};

> +

>  #endif /* _SCSI_SCSI_STATUS_H */

> diff --git a/include/uapi/scsi/scsi_bsg_fc.h 

> b/include/uapi/scsi/scsi_bsg_fc.h

> index 3ae65e93235c..419db719fe8e 100644

> --- a/include/uapi/scsi/scsi_bsg_fc.h

> +++ b/include/uapi/scsi/scsi_bsg_fc.h

> @@ -9,6 +9,9 @@

>  #define SCSI_BSG_FC_H

> 

>  #include <linux/types.h>

> +#ifdef __KERNEL__

> +#include <scsi/scsi_status.h>

> +#endif

> 

>  /*

>   * This file intended to be included by both kernel and user space

> @@ -291,7 +294,14 @@ struct fc_bsg_reply {

>  	 *    msg and status fields. The per-msgcode reply structure

>  	 *    will contain valid data.

>  	 */

> +#ifdef __KERNEL__

> +	union {

> +		__u32		  result; /* do not use in new kernel code */

> +		union scsi_status status;

> +	};

> +#else

>  	__u32 result;

> +#endif

> 

>  	/* If there was reply_payload, how much was recevied ? */

>  	__u32 reply_payload_rcv_len;

> diff --git a/include/uapi/scsi/scsi_bsg_ufs.h 

> b/include/uapi/scsi/scsi_bsg_ufs.h

> index d55f2176dfd4..3dfe5a5a0842 100644

> --- a/include/uapi/scsi/scsi_bsg_ufs.h

> +++ b/include/uapi/scsi/scsi_bsg_ufs.h

> @@ -9,6 +9,10 @@

>  #define SCSI_BSG_UFS_H

> 

>  #include <linux/types.h>

> +#ifdef __KERNEL__

> +#include <scsi/scsi_status.h>

> +#endif

> +

>  /*

>   * This file intended to be included by both kernel and user space

>   */

> @@ -95,7 +99,14 @@ struct ufs_bsg_reply {

>  	 * msg and status fields. The per-msgcode reply structure

>  	 * will contain valid data.

>  	 */

> +#ifdef __KERNEL__

> +	union {

> +		__u32		  result; /* do not use in new kernel code */

> +		union scsi_status status;

> +	};

> +#else

>  	__u32 result;

> +#endif

> 

>  	/* If there was reply_payload, how much was received? */

>  	__u32 reply_payload_rcv_len;


Reviewed-by: Can Guo <cang@codeaurora.org>
Can Guo May 7, 2021, 12:09 a.m. | #21
Hi Bart,

On 2021-04-20 10:13, Bart Van Assche wrote:
> An explanation of the purpose of this patch is available in the patch
> "scsi: Introduce the scsi_status union".
> 
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufs_bsg.c |  2 +-
>  drivers/scsi/ufs/ufshcd.c  | 27 +++++++++++++++------------
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> index 5b2bc1a6f922..4dcc09136a50 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -152,7 +152,7 @@ static int ufs_bsg_request(struct bsg_job *job)
>  	kfree(desc_buff);
> 
>  out:
> -	bsg_reply->result = ret;
> +	bsg_reply->status.combined = ret;
>  	job->reply_len = sizeof(struct ufs_bsg_reply);
>  	/* complete the job here only if no error */
>  	if (ret == 0)
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d966d80838fb..cec555d3fcd7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4933,7 +4933,7 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp,
> enum sam_status scsi_status)
>  static inline int
>  ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb 
> *lrbp)
>  {
> -	int result = 0;
> +	union scsi_status result;
>  	int ocs;
> 
>  	/* overall command status of utrd */
> @@ -4951,7 +4951,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>  		switch (ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr)) {
>  		case UPIU_TRANSACTION_RESPONSE:
>  			/* Propagate the SCSI status to the SCSI midlayer. */
> -			result = ufshcd_scsi_cmd_status(lrbp,
> +			result.combined = ufshcd_scsi_cmd_status(lrbp,
>  				ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr) &
>  				MASK_SCSI_STATUS);
> 
> @@ -4981,23 +4981,23 @@ ufshcd_transfer_rsp_status(struct ufs_hba
> *hba, struct ufshcd_lrb *lrbp)
>  			break;
>  		case UPIU_TRANSACTION_REJECT_UPIU:
>  			/* TODO: handle Reject UPIU Response */
> -			result = DID_ERROR << 16;
> +			result.combined = DID_ERROR << 16;
>  			dev_err(hba->dev,
>  				"Reject UPIU not fully implemented\n");
>  			break;
>  		default:
>  			dev_err(hba->dev,
>  				"Unexpected request response code = %x\n",
> -				result);
> -			result = DID_ERROR << 16;
> +				result.combined);
> +			result.combined = DID_ERROR << 16;
>  			break;
>  		}
>  		break;
>  	case OCS_ABORTED:
> -		result |= DID_ABORT << 16;
> +		result.combined |= DID_ABORT << 16;
>  		break;
>  	case OCS_INVALID_COMMAND_STATUS:
> -		result |= DID_REQUEUE << 16;
> +		result.combined |= DID_REQUEUE << 16;
>  		break;
>  	case OCS_INVALID_CMD_TABLE_ATTR:
>  	case OCS_INVALID_PRDT_ATTR:
> @@ -5009,7 +5009,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>  	case OCS_INVALID_CRYPTO_CONFIG:
>  	case OCS_GENERAL_CRYPTO_ERROR:
>  	default:
> -		result |= DID_ERROR << 16;
> +		result.combined |= DID_ERROR << 16;
>  		dev_err(hba->dev,
>  				"OCS error from controller = %x for tag %d\n",
>  				ocs, lrbp->task_tag);
> @@ -5021,7 +5021,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>  	if ((host_byte(result) != DID_OK) &&
>  	    (host_byte(result) != DID_REQUEUE) && !hba->silence_err_logs)
>  		ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
> -	return result;
> +	return result.combined;
>  }
> 
>  /**
> @@ -5083,7 +5083,7 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  			ufshcd_add_command_trace(hba, index, UFS_CMD_COMP);
>  			result = ufshcd_transfer_rsp_status(hba, lrbp);
>  			scsi_dma_unmap(cmd);
> -			cmd->result = result;
> +			cmd->status.combined = result;
>  			/* Mark completed command as NULL in LRB */
>  			lrbp->cmd = NULL;
>  			/* Do not touch lrbp after scsi done */
> @@ -8437,6 +8437,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba 
> *hba,
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_device *sdp;
>  	unsigned long flags;
> +	union scsi_status start_stop_res;
>  	int ret;
> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> @@ -8471,13 +8472,15 @@ static int ufshcd_set_dev_pwr_mode(struct 
> ufs_hba *hba,
>  	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
>  	 * already suspended childs.
>  	 */
> -	ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
> +	start_stop_res.combined =
> +		scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
>  			START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
> +	ret = start_stop_res.combined;
>  	if (ret) {
>  		sdev_printk(KERN_WARNING, sdp,
>  			    "START_STOP failed for power mode: %d, result %x\n",
>  			    pwr_mode, ret);
> -		if (driver_byte(ret) == DRIVER_SENSE)
> +		if (driver_byte(start_stop_res) == DRIVER_SENSE)
>  			scsi_print_sense_hdr(sdp, NULL, &sshdr);
>  	}

Thanks for the change, do you miss ufshcd_send_request_sense()?

Regards,
Can Guo.
Can Guo May 7, 2021, 12:24 a.m. | #22
On 2021-04-20 10:13, Bart Van Assche wrote:
> A previous patch changed 'int result;' into a union and introduced the

> scsi_status member in that union. Now that the conversion from type

> 'int' into 'union scsi_status' is complete, remove the 'result' member.

> 

> Cc: Christoph Hellwig <hch@lst.de>

> Cc: Ming Lei <ming.lei@redhat.com>

> Cc: Hannes Reinecke <hare@suse.de>

> Cc: John Garry <john.garry@huawei.com>

> Cc: Lee Duncan <lduncan@suse.com>

> Cc: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  include/linux/bsg-lib.h          | 5 +----

>  include/scsi/scsi_bsg_iscsi.h    | 7 ++-----

>  include/scsi/scsi_cmnd.h         | 5 +----

>  include/scsi/scsi_eh.h           | 5 +----

>  include/scsi/scsi_request.h      | 5 +----

>  include/uapi/scsi/scsi_bsg_fc.h  | 5 +----

>  include/uapi/scsi/scsi_bsg_ufs.h | 7 ++-----

>  7 files changed, 9 insertions(+), 30 deletions(-)

> 

> diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h

> index f934afc45760..6143a54454db 100644

> --- a/include/linux/bsg-lib.h

> +++ b/include/linux/bsg-lib.h

> @@ -53,10 +53,7 @@ struct bsg_job {

>  	struct bsg_buffer request_payload;

>  	struct bsg_buffer reply_payload;

> 

> -	union {

> -		int		  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	unsigned int reply_payload_rcv_len;

> 

>  	/* BIDI support */

> diff --git a/include/scsi/scsi_bsg_iscsi.h 

> b/include/scsi/scsi_bsg_iscsi.h

> index d18e7e061927..e384df88fab1 100644

> --- a/include/scsi/scsi_bsg_iscsi.h

> +++ b/include/scsi/scsi_bsg_iscsi.h

> @@ -76,17 +76,14 @@ struct iscsi_bsg_request {

>  /* response (request sense data) structure of the sg_io_v4 */

>  struct iscsi_bsg_reply {

>  	/*

> -	 * The completion result. Result exists in two forms:

> +	 * The completion status. Result exists in two forms:

>  	 * if negative, it is an -Exxx system errno value. There will

>  	 * be no further reply information supplied.

>  	 * else, it's the 4-byte scsi error result, with driver, host,

>  	 * msg and status fields. The per-msgcode reply structure

>  	 * will contain valid data.

>  	 */

> -	union {

> -		uint32_t	  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

> 

>  	/* If there was reply_payload, how much was recevied ? */

>  	uint32_t reply_payload_rcv_len;

> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h

> index 539be97b0a7d..b616e1d8af9a 100644

> --- a/include/scsi/scsi_cmnd.h

> +++ b/include/scsi/scsi_cmnd.h

> @@ -141,10 +141,7 @@ struct scsi_cmnd {

>  					 * to be at an address < 16Mb). */

> 

>  	/* Status code from lower level driver */

> -	union {

> -		int		  result; /* do not use in new code. */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	int flags;		/* Command flags */

>  	unsigned long state;	/* Command completion state */

> 

> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h

> index dcd66bb9a1ba..406e22887d96 100644

> --- a/include/scsi/scsi_eh.h

> +++ b/include/scsi/scsi_eh.h

> @@ -33,10 +33,7 @@ extern int scsi_ioctl_reset(struct scsi_device *,

> int __user *);

> 

>  struct scsi_eh_save {

>  	/* saved state */

> -	union {

> -		int		  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	unsigned int resid_len;

>  	int eh_eflags;

>  	enum dma_data_direction data_direction;

> diff --git a/include/scsi/scsi_request.h b/include/scsi/scsi_request.h

> index 83f5549cc74c..41b8f9f6a349 100644

> --- a/include/scsi/scsi_request.h

> +++ b/include/scsi/scsi_request.h

> @@ -11,10 +11,7 @@ struct scsi_request {

>  	unsigned char	__cmd[BLK_MAX_CDB];

>  	unsigned char	*cmd;

>  	unsigned short	cmd_len;

> -	union {

> -		int		  result; /* do not use in new code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  	unsigned int	sense_len;

>  	unsigned int	resid_len;	/* residual count */

>  	int		retries;

> diff --git a/include/uapi/scsi/scsi_bsg_fc.h 

> b/include/uapi/scsi/scsi_bsg_fc.h

> index 419db719fe8e..6d095aefc265 100644

> --- a/include/uapi/scsi/scsi_bsg_fc.h

> +++ b/include/uapi/scsi/scsi_bsg_fc.h

> @@ -295,10 +295,7 @@ struct fc_bsg_reply {

>  	 *    will contain valid data.

>  	 */

>  #ifdef __KERNEL__

> -	union {

> -		__u32		  result; /* do not use in new kernel code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  #else

>  	__u32 result;

>  #endif

> diff --git a/include/uapi/scsi/scsi_bsg_ufs.h 

> b/include/uapi/scsi/scsi_bsg_ufs.h

> index 3dfe5a5a0842..1c282ab144f6 100644

> --- a/include/uapi/scsi/scsi_bsg_ufs.h

> +++ b/include/uapi/scsi/scsi_bsg_ufs.h

> @@ -92,7 +92,7 @@ struct ufs_bsg_request {

>  /* response (request sense data) structure of the sg_io_v4 */

>  struct ufs_bsg_reply {

>  	/*

> -	 * The completion result. Result exists in two forms:

> +	 * The completion status. Exists in two forms:

>  	 * if negative, it is an -Exxx system errno value. There will

>  	 * be no further reply information supplied.

>  	 * else, it's the 4-byte scsi error result, with driver, host,

> @@ -100,10 +100,7 @@ struct ufs_bsg_reply {

>  	 * will contain valid data.

>  	 */

>  #ifdef __KERNEL__

> -	union {

> -		__u32		  result; /* do not use in new kernel code */

> -		union scsi_status status;

> -	};

> +	union scsi_status status;

>  #else

>  	__u32 result;

>  #endif


Reviewed-by: Can Guo <cang@codeaurora.org>
Bart Van Assche May 7, 2021, 3:35 a.m. | #23
On 5/6/21 5:09 PM, Can Guo wrote:
> Thanks for the change, do you miss ufshcd_send_request_sense()?


Hi Can,

That's a good question. I can change the type of the
ufshcd_send_request_sense() return value but I'm not sure whether it's
worth it since that function only has one caller and that caller does
not care about the subcomponents of the SCSI status value. All it cares
about is whether or not ufshcd_send_request_sense() returns 0.

Thanks,

Bart.
Can Guo May 7, 2021, 4:48 a.m. | #24
Hi Bart,

On 2021-05-07 11:35, Bart Van Assche wrote:
> On 5/6/21 5:09 PM, Can Guo wrote:

>> Thanks for the change, do you miss ufshcd_send_request_sense()?

> 

> Hi Can,

> 

> That's a good question. I can change the type of the

> ufshcd_send_request_sense() return value but I'm not sure whether it's

> worth it since that function only has one caller and that caller does

> not care about the subcomponents of the SCSI status value. All it cares

> about is whether or not ufshcd_send_request_sense() returns 0.

> 

> Thanks,

> 

> Bart.


I agree.

Reviewed-by: Can Guo <cang@codeaurora.org>