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@xxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: John Garry <john.garry@xxxxxxxxxx>
> Cc: Can Guo <cang@xxxxxxxxxxxxxx>
> Cc: James Smart <james.smart@xxxxxxxxxxxx>
> Cc: Lee Duncan <lduncan@xxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> 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@xxxxxxxx>
