On Fri, Apr 06, 2018 at 02:59:16PM +0000, Bart Van Assche wrote:
> On Fri, 2018-04-06 at 09:45 +0200, Johannes Thumshirn wrote:
> > On 05/04/18 19:49, Martin K. Petersen wrote:
> > > Longer term I'd really like to see the command result integer
> > > host/driver/msg/status stuff cleaned up. It's super arcane and the
> > > associated naming schemes make it a very error-prone interface.
> > >
> >
> > I did start a series [1] for this but than got distracted by more urgent
> > things. I can pick it up again I think.
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE
>
> Hello Johannes,
>
> Since modifying how SCSI results are communicated from SCSI LLDs to the SCSI
> core requires to touch all SCSI LLDs, please consider to include the following
> changes in your patch series:
> - Remove the deprecated SCSI status codes (GOOD, CHECK_CONDITION, etc.) and
> use the SAM_STAT_* symbolic names instead.
I'll have a look, thanks for the heads up.
> - Ensure that assigning an int to scsi_cmnd.result triggers a compiler error.
> There is code in the Linux kernel that mixes traditional error codes (-EIO)
> with the SCSI result codes (DID_ERROR << 16). I think most of that code is
> buggy and that it should be fixed. Changing the type of scsi_cmnd.result is
> one way to find such code. How about something like the following (untested)
> patch?
I'll give it a shot.
>
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index cb85eddb47ea..27f1c347f0ea 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -18,6 +18,27 @@ enum scsi_timeouts {
> SCSI_DEFAULT_EH_TIMEOUT = 10 * HZ,
> };
>
> +/*
> + * Note: .result is signed because SCSI LLDs store a SCSI result in
> + * .result while the bsg driver stores a negative error code in .result.
> + */
> +typedef union {
> + s32 result;
> + struct {
> +#if defined(__BIG_ENDIAN)
> + u8 driver_byte;
> + u8 host_byte;
> + u8 msg_byte;
> + u8 status_byte;
> +#elif defined(__LITTLE_ENDIAN)
> + u8 status_byte;
> + u8 msg_byte;
> + u8 host_byte;
> + u8 driver_byte;
> +#endif
> + };
> +} scsi_result;
> +
> /*
> * DIX-capable adapters effectively support infinite chaining for the
> * protection information scatterlist
> @@ -38,8 +59,10 @@ 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 int scsi_status_is_good(scsi_result result)
> {
> + u8 status = result.status_byte;
> +
> /*
> * FIXME: bit0 is listed as reserved in SCSI-2, but is
> * significant in SCSI-3. For now, we follow the SCSI-2
> @@ -205,10 +228,10 @@ static inline int scsi_is_wlun(u64 lun)
> * host_byte = set by low-level driver to indicate status.
> * driver_byte = set by mid-level.
> */
> -#define status_byte(result) (((result) >> 1) & 0x7f)
> -#define msg_byte(result) (((result) >> 8) & 0xff)
> -#define host_byte(result) (((result) >> 16) & 0xff)
> -#define driver_byte(result) (((result) >> 24) & 0xff)
> +#define status_byte(result) ((result).status_byte >> 1)
> +#define msg_byte(result) ((result).msg_byte)
> +#define host_byte(result) ((result).host_byte)
> +#define driver_byte(result) ((result).driver_byte)
>
> #define sense_class(sense) (((sense) >> 4) & 0x7)
> #define sense_error(sense) ((sense) & 0xf)
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 2280b2351739..cbc5df948dd3 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -144,7 +144,7 @@ struct scsi_cmnd {
> * obtained by scsi_malloc is guaranteed
> * to be at an address < 16Mb). */
>
> - int result; /* Status code from lower level driver */
> + scsi_result result; /* Status code from lower level driver */
> int flags; /* Command flags */
>
> unsigned char tag; /* SCSI-II queued command tag */
>
> Bart.
--
Johannes Thumshirn Storage
jthumsh...@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
