| Message ID | 20220228112224.18942-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | include/public: add command result definitions to vscsiif.h | expand |
Ping? On 28.02.22 12:22, Juergen Gross wrote: > The result field of struct vscsiif_response is lacking a detailed > definition. Today the Linux kernel internal scsi definitions are being > used, which is not a sane interface for a PV device driver. > > Add macros to change that by using today's values in the XEN namespace. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > xen/include/public/io/vscsiif.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h > index c9ceb1884d..17a9033b43 100644 > --- a/xen/include/public/io/vscsiif.h > +++ b/xen/include/public/io/vscsiif.h > @@ -315,6 +315,33 @@ struct vscsiif_response { > }; > typedef struct vscsiif_response vscsiif_response_t; > > +/* SCSI I/O status from vscsiif_response->rslt */ > +#define XEN_VSCSIIF_RSLT_STATUS(x) (x & 0x00ff) > + > +/* Host I/O status from vscsiif_response->rslt */ > +#define XEN_VSCSIIF_RSLT_HOST(x) ((unsigned)x >> 16) > +#define XEN_VSCSIIF_RSLT_HOST_OK 0 > +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN 1 /* Couldn't connect before timeout */ > +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2 /* BUS busy through timeout */ > +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3 /* TIMED OUT for other reason */ > +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG 4 /* BAD target */ > +#define XEN_VSCSIIF_RSLT_HOST_ABORT 5 /* Abort for some other reason */ > +#define XEN_VSCSIIF_RSLT_HOST_PARITY 6 /* Parity error */ > +#define XEN_VSCSIIF_RSLT_HOST_ERROR 7 /* Internal error */ > +#define XEN_VSCSIIF_RSLT_HOST_RESET 8 /* Reset by somebody */ > +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9 /* Unexpected interrupt */ > +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR 10 /* Force command past mid-layer */ > +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR 11 /* Retry requested */ > +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR 12 /* Hidden retry requested */ > +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13 /* Requeue command requested */ > +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT 14 /* Transport error disrupted I/O */ > +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST 15 /* Transport class fastfailed */ > +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */ > +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL 17 /* Permanent nexus failure on path */ > +#define XEN_VSCSIIF_RSLT_HOST_NOMEM 18 /* Space allocation failed */ > +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR 19 /* Medium error */ > +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL 20 /* Transport marginal errors */ > + > DEFINE_RING_TYPES(vscsiif, struct vscsiif_request, struct vscsiif_response); > >
On 28.02.2022 12:22, Juergen Gross wrote: > --- a/xen/include/public/io/vscsiif.h > +++ b/xen/include/public/io/vscsiif.h > @@ -315,6 +315,33 @@ struct vscsiif_response { > }; > typedef struct vscsiif_response vscsiif_response_t; > > +/* SCSI I/O status from vscsiif_response->rslt */ > +#define XEN_VSCSIIF_RSLT_STATUS(x) (x & 0x00ff) No #define-s for individual values for this? I see the backend use e.g. SUCCESS and FAILED, wherever these come from ... Also please parenthesize x here and ... > +/* Host I/O status from vscsiif_response->rslt */ > +#define XEN_VSCSIIF_RSLT_HOST(x) ((unsigned)x >> 16) ... here. You cast to unsigned here, but rslt is a signed field. Is it really the entire upper 16 bits that are the host I/O status? > +#define XEN_VSCSIIF_RSLT_HOST_OK 0 > +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN 1 /* Couldn't connect before timeout */ > +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2 /* BUS busy through timeout */ > +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3 /* TIMED OUT for other reason */ > +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG 4 /* BAD target */ Are the all-upper-case words really in need of mirroring this aspect from Linux? To me it gives the impression of this being acronyms of some sort at the first glance. > +#define XEN_VSCSIIF_RSLT_HOST_ABORT 5 /* Abort for some other reason */ > +#define XEN_VSCSIIF_RSLT_HOST_PARITY 6 /* Parity error */ > +#define XEN_VSCSIIF_RSLT_HOST_ERROR 7 /* Internal error */ > +#define XEN_VSCSIIF_RSLT_HOST_RESET 8 /* Reset by somebody */ > +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9 /* Unexpected interrupt */ > +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR 10 /* Force command past mid-layer */ > +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR 11 /* Retry requested */ > +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR 12 /* Hidden retry requested */ > +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13 /* Requeue command requested */ > +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT 14 /* Transport error disrupted I/O */ > +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST 15 /* Transport class fastfailed */ > +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */ > +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL 17 /* Permanent nexus failure on path */ > +#define XEN_VSCSIIF_RSLT_HOST_NOMEM 18 /* Space allocation failed */ > +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR 19 /* Medium error */ > +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL 20 /* Transport marginal errors */ Some of the name shortening that you did, comparing with the Linux names, has gone a little too far for my taste. But you're the maintainer ... Jan
On 14.03.22 10:55, Jan Beulich wrote: > On 28.02.2022 12:22, Juergen Gross wrote: >> --- a/xen/include/public/io/vscsiif.h >> +++ b/xen/include/public/io/vscsiif.h >> @@ -315,6 +315,33 @@ struct vscsiif_response { >> }; >> typedef struct vscsiif_response vscsiif_response_t; >> >> +/* SCSI I/O status from vscsiif_response->rslt */ >> +#define XEN_VSCSIIF_RSLT_STATUS(x) (x & 0x00ff) > > No #define-s for individual values for this? I see the backend use > e.g. SUCCESS and FAILED, wherever these come from ... Oh, right, those are being used for the reset actions. Thanks for spotting. The "normal" request result values are defined at the SCSI layer. > Also please parenthesize x here and ... > >> +/* Host I/O status from vscsiif_response->rslt */ >> +#define XEN_VSCSIIF_RSLT_HOST(x) ((unsigned)x >> 16) > > ... here. Okay. > You cast to unsigned here, but rslt is a signed field. Is it really > the entire upper 16 bits that are the host I/O status? I thought I have seen it being used this way, but now I've found the definition of "host_byte()" indicating it is only 8 bits wide. Will change that. > >> +#define XEN_VSCSIIF_RSLT_HOST_OK 0 >> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN 1 /* Couldn't connect before timeout */ >> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2 /* BUS busy through timeout */ >> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3 /* TIMED OUT for other reason */ >> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG 4 /* BAD target */ > > Are the all-upper-case words really in need of mirroring this > aspect from Linux? To me it gives the impression of this being > acronyms of some sort at the first glance. The backend can return all these values, so I think I need to define them here. > >> +#define XEN_VSCSIIF_RSLT_HOST_ABORT 5 /* Abort for some other reason */ >> +#define XEN_VSCSIIF_RSLT_HOST_PARITY 6 /* Parity error */ >> +#define XEN_VSCSIIF_RSLT_HOST_ERROR 7 /* Internal error */ >> +#define XEN_VSCSIIF_RSLT_HOST_RESET 8 /* Reset by somebody */ >> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9 /* Unexpected interrupt */ >> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR 10 /* Force command past mid-layer */ >> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR 11 /* Retry requested */ >> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR 12 /* Hidden retry requested */ >> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13 /* Requeue command requested */ >> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT 14 /* Transport error disrupted I/O */ >> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST 15 /* Transport class fastfailed */ >> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */ >> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL 17 /* Permanent nexus failure on path */ >> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM 18 /* Space allocation failed */ >> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR 19 /* Medium error */ >> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL 20 /* Transport marginal errors */ > > Some of the name shortening that you did, comparing with the > Linux names, has gone a little too far for my taste. But you're > the maintainer ... There are basically the following alternatives: - use longer names (using the Linux names would end up in e.g. XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer - drop some part of the common prefix, e.g. the "RSLT_HOST_" part - keep it as is I'm basically fine with any of those. Juergen
On 16.03.2022 10:03, Juergen Gross wrote: > On 14.03.22 10:55, Jan Beulich wrote: >> On 28.02.2022 12:22, Juergen Gross wrote: >>> +#define XEN_VSCSIIF_RSLT_HOST_OK 0 >>> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN 1 /* Couldn't connect before timeout */ >>> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2 /* BUS busy through timeout */ >>> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3 /* TIMED OUT for other reason */ >>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG 4 /* BAD target */ >> >> Are the all-upper-case words really in need of mirroring this >> aspect from Linux? To me it gives the impression of this being >> acronyms of some sort at the first glance. > > The backend can return all these values, so I think I need to define > them here. Oh, I realize I didn't say so explicitly and hence what I said ended up being ambiguous: The remark was only about the all- upper-case words in the comments. I would think they can be spelled normally. >>> +#define XEN_VSCSIIF_RSLT_HOST_ABORT 5 /* Abort for some other reason */ >>> +#define XEN_VSCSIIF_RSLT_HOST_PARITY 6 /* Parity error */ >>> +#define XEN_VSCSIIF_RSLT_HOST_ERROR 7 /* Internal error */ >>> +#define XEN_VSCSIIF_RSLT_HOST_RESET 8 /* Reset by somebody */ >>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9 /* Unexpected interrupt */ >>> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR 10 /* Force command past mid-layer */ >>> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR 11 /* Retry requested */ >>> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR 12 /* Hidden retry requested */ >>> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13 /* Requeue command requested */ >>> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT 14 /* Transport error disrupted I/O */ >>> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST 15 /* Transport class fastfailed */ >>> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */ >>> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL 17 /* Permanent nexus failure on path */ >>> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM 18 /* Space allocation failed */ >>> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR 19 /* Medium error */ >>> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL 20 /* Transport marginal errors */ >> >> Some of the name shortening that you did, comparing with the >> Linux names, has gone a little too far for my taste. But you're >> the maintainer ... > > There are basically the following alternatives: > > - use longer names (using the Linux names would end up in e.g. > XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer > - drop some part of the common prefix, e.g. the "RSLT_HOST_" part > - keep it as is > > I'm basically fine with any of those. My personal preference would be in the order you named the alternatives, perhaps with prepending them by "use longer names, but in extreme cases not quite as long as Linux'es". Jan
On 16.03.22 10:09, Jan Beulich wrote: > On 16.03.2022 10:03, Juergen Gross wrote: >> On 14.03.22 10:55, Jan Beulich wrote: >>> On 28.02.2022 12:22, Juergen Gross wrote: >>>> +#define XEN_VSCSIIF_RSLT_HOST_OK 0 >>>> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN 1 /* Couldn't connect before timeout */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2 /* BUS busy through timeout */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3 /* TIMED OUT for other reason */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG 4 /* BAD target */ >>> >>> Are the all-upper-case words really in need of mirroring this >>> aspect from Linux? To me it gives the impression of this being >>> acronyms of some sort at the first glance. >> >> The backend can return all these values, so I think I need to define >> them here. > > Oh, I realize I didn't say so explicitly and hence what I said > ended up being ambiguous: The remark was only about the all- > upper-case words in the comments. I would think they can be > spelled normally. Ah, okay. :-) Will change that. > >>>> +#define XEN_VSCSIIF_RSLT_HOST_ABORT 5 /* Abort for some other reason */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_PARITY 6 /* Parity error */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_ERROR 7 /* Internal error */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_RESET 8 /* Reset by somebody */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9 /* Unexpected interrupt */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR 10 /* Force command past mid-layer */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR 11 /* Retry requested */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR 12 /* Hidden retry requested */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13 /* Requeue command requested */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT 14 /* Transport error disrupted I/O */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST 15 /* Transport class fastfailed */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL 17 /* Permanent nexus failure on path */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM 18 /* Space allocation failed */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR 19 /* Medium error */ >>>> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL 20 /* Transport marginal errors */ >>> >>> Some of the name shortening that you did, comparing with the >>> Linux names, has gone a little too far for my taste. But you're >>> the maintainer ... >> >> There are basically the following alternatives: >> >> - use longer names (using the Linux names would end up in e.g. >> XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer >> - drop some part of the common prefix, e.g. the "RSLT_HOST_" part >> - keep it as is >> >> I'm basically fine with any of those. > > My personal preference would be in the order you named the > alternatives, perhaps with prepending them by "use longer names, > but in extreme cases not quite as long as Linux'es". Okay, fine with me. Juergen
diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h index c9ceb1884d..17a9033b43 100644 --- a/xen/include/public/io/vscsiif.h +++ b/xen/include/public/io/vscsiif.h @@ -315,6 +315,33 @@ struct vscsiif_response { }; typedef struct vscsiif_response vscsiif_response_t; +/* SCSI I/O status from vscsiif_response->rslt */ +#define XEN_VSCSIIF_RSLT_STATUS(x) (x & 0x00ff) + +/* Host I/O status from vscsiif_response->rslt */ +#define XEN_VSCSIIF_RSLT_HOST(x) ((unsigned)x >> 16) +#define XEN_VSCSIIF_RSLT_HOST_OK 0 +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN 1 /* Couldn't connect before timeout */ +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2 /* BUS busy through timeout */ +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3 /* TIMED OUT for other reason */ +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG 4 /* BAD target */ +#define XEN_VSCSIIF_RSLT_HOST_ABORT 5 /* Abort for some other reason */ +#define XEN_VSCSIIF_RSLT_HOST_PARITY 6 /* Parity error */ +#define XEN_VSCSIIF_RSLT_HOST_ERROR 7 /* Internal error */ +#define XEN_VSCSIIF_RSLT_HOST_RESET 8 /* Reset by somebody */ +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9 /* Unexpected interrupt */ +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR 10 /* Force command past mid-layer */ +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR 11 /* Retry requested */ +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR 12 /* Hidden retry requested */ +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13 /* Requeue command requested */ +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT 14 /* Transport error disrupted I/O */ +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST 15 /* Transport class fastfailed */ +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */ +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL 17 /* Permanent nexus failure on path */ +#define XEN_VSCSIIF_RSLT_HOST_NOMEM 18 /* Space allocation failed */ +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR 19 /* Medium error */ +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL 20 /* Transport marginal errors */ + DEFINE_RING_TYPES(vscsiif, struct vscsiif_request, struct vscsiif_response);
The result field of struct vscsiif_response is lacking a detailed definition. Today the Linux kernel internal scsi definitions are being used, which is not a sane interface for a PV device driver. Add macros to change that by using today's values in the XEN namespace. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/include/public/io/vscsiif.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)