| Message ID | 20210420000845.25873-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Make better use of static type checking | expand |
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
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
> 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
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.
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
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.
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.
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
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
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>
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.
> 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
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.
> 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
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>
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>
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>
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>
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>
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>
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.
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>
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.
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>
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