Re: [PATCH 1/1] cciss: fix error reporting for SG_IO

[Jens Axboe]

On Tue, Aug 14 2007, Mike Miller (OS Dev) wrote:
> Patch 1/1
> Steve has been trying to send this out but it doesn't seem to be getting
> anywhere. Please review this patch for accuracy. There's a couple of things
> not clear to us.
> 
> Thanks,
> mikem
> 
> --------------------------------------------------------------------------------
> We found a problem with the way cciss was filling out rq->errors.
> Previously, it just put a 1 or a 0 in there and used the negation
> of this as the uptodate parameter to one of the functions in the
> block layer, being a block device.
> 
> For the SG_IO support, this is not sufficient, and we noticed
> that for example, sg_turs from sg3_utils does not correctly
> detect problems due to cciss filling out rq->errors incorrectly.
> 
> So, below is my attempt at fixing this, but I'm not completely
> sure I did it all right.  It is better than before, in that
> now sg_turs seems to detect when things go wrong (e.g.:
> when a disk is inaccessible due to cables being yanked, it
> notices.)
> 
> There is some stuff in scsi.h:
> 
> > /*
> >  *  Use these to separate status msg and our bytes
> >  *
> >  *  These are set by:
> >  *
> >  *      status byte = set from target device
> >  *      msg_byte    = return status from host adapter itself.
> >  *      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)
> 
> I'm unsure about the msg_byte (sg_turs appears not to 
> look at it.)  I used a device specific code (cciss 
> notion of CommandStatus) here.  Not sure if that's correct, 
> but not sure what else I would use.  Any clarification on 
> that would be helpful.  And of course, let me know if you 
> notice anything else I might have screwed up.
> 
> -- steve
> 
> Signed-off-by: Stephen M. Cameron <steve.cameron@xxxxxx>
> 
> ---
> 
>  drivers/block/cciss.c |   81 ++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 63 insertions(+), 18 deletions(-)
> 
> diff -puN drivers/block/cciss.c~sg_io_fix_error_reporting drivers/block/cciss.c
> --- linux-2.6.23-rc2/drivers/block/cciss.c~sg_io_fix_error_reporting	2007-08-13 09:44:58.000000000 -0500
> +++ linux-2.6.23-rc2-scameron/drivers/block/cciss.c	2007-08-13 09:44:58.000000000 -0500
> @@ -2366,30 +2366,57 @@ static inline void resend_cciss_cmd(ctlr
>  	start_io(h);
>  }
>  
> +/* inverse of macros in scsi.h */
> +
> +#define shift_status_byte(x) ((x) & 0xff)
> +#define shift_msg_byte(x) (((x) & 0xff) << 8)
> +#define shift_host_byte(x) (((x) & 0xff) << 16)
> +#define shift_driver_byte(x) (((x) & 0xff) << 24)
> +
> +#define make_status_bytes(s, m, h, d) \
> +	shift_status_byte((s)) |\
> +	shift_msg_byte((m)) |\
> +	shift_host_byte((h)) |\
> +	 shift_driver_byte((d))

Please don't make that a macro, make it a function. Otherwise it looks
ok, care to fix that up and resubmit?

-- 
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/