Re: [PATCH v4 06/43] hpsa: hpsa decode sense data for io and tmf

[Hannes Reinecke <hare@suse.de>]

Hid Don,

some comments at the end.

On 04/16/2015 03:47 PM, Don Brace wrote:
> From: Stephen Cameron <stephenmcameron@gmail.com>
> 
> In hba mode, we could get sense data in descriptor format so
> we need to handle that.
> 
> It's possible for CommandStatus to have value 0x0D
> "TMF Function Status", which we should handle.  We will get
> this from a P1224 when aborting a non-existent tag, for
> example.  The "ScsiStatus" field of the errinfo field
> will contain the TMF function status value.
> 
> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
> Signed-off-by: Don Brace <don.brace@pmcs.com>
> ---
>  drivers/scsi/hpsa.c     |  143 +++++++++++++++++++++++++++++++++++------------
>  drivers/scsi/hpsa_cmd.h |    9 +++
>  2 files changed, 117 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 6ee92af..68238dd 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -43,6 +43,7 @@
>  #include <scsi/scsi_device.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_tcq.h>
> +#include <scsi/scsi_eh.h>
>  #include <linux/cciss_ioctl.h>
>  #include <linux/string.h>
>  #include <linux/bitmap.h>
> @@ -268,16 +269,49 @@ static inline struct ctlr_info *shost_to_hba(struct Scsi_Host *sh)
>  	return (struct ctlr_info *) *priv;
>  }
>  
> +/* extract sense key, asc, and ascq from sense data.  -1 means invalid. */
> +static void decode_sense_data(const u8 *sense_data, int sense_data_len,
> +			int *sense_key, int *asc, int *ascq)
> +{
> +	struct scsi_sense_hdr sshdr;
> +	bool rc;
> +
> +	*sense_key = -1;
> +	*asc = -1;
> +	*ascq = -1;
> +
> +	if (sense_data_len < 1)
> +		return;
> +
> +	rc = scsi_normalize_sense(sense_data, sense_data_len, &sshdr);
> +	if (rc) {
> +		*sense_key = sshdr.sense_key;
> +		*asc = sshdr.asc;
> +		*ascq = sshdr.ascq;
> +	}
> +}
> +
>  static int check_for_unit_attention(struct ctlr_info *h,
>  	struct CommandList *c)
>  {
> -	if (c->err_info->SenseInfo[2] != UNIT_ATTENTION)
> +	int sense_key, asc, ascq;
> +	int sense_len;
> +
> +	if (c->err_info->SenseLen > sizeof(c->err_info->SenseInfo))
> +		sense_len = sizeof(c->err_info->SenseInfo);
> +	else
> +		sense_len = c->err_info->SenseLen;
> +
> +	decode_sense_data(c->err_info->SenseInfo, sense_len,
> +				&sense_key, &asc, &ascq);
> +	if (sense_key != UNIT_ATTENTION || asc == -1)
>  		return 0;
>  
> -	switch (c->err_info->SenseInfo[12]) {
> +	switch (asc) {
>  	case STATE_CHANGED:
> -		dev_warn(&h->pdev->dev, HPSA "%d: a state change "
> -			"detected, command retried\n", h->ctlr);
> +		dev_warn(&h->pdev->dev,
> +			HPSA "%d: a state change detected, command retried\n",
> +			h->ctlr);
>  		break;
>  	case LUN_FAILED:
>  		dev_warn(&h->pdev->dev,
> @@ -1860,6 +1894,34 @@ retry_cmd:
>  	queue_work_on(raw_smp_processor_id(), h->resubmit_wq, &c->work);
>  }
>  
> +/* Returns 0 on success, < 0 otherwise. */
> +static int hpsa_evaluate_tmf_status(struct ctlr_info *h,
> +					struct CommandList *cp)
> +{
> +	u8 tmf_status = cp->err_info->ScsiStatus;
> +
> +	switch (tmf_status) {
> +	case CISS_TMF_COMPLETE:
> +		/*
> +		 * CISS_TMF_COMPLETE never happens, instead,
> +		 * ei->CommandStatus == 0 for this case.
> +		 */
> +	case CISS_TMF_SUCCESS:
> +		return 0;
> +	case CISS_TMF_INVALID_FRAME:
> +	case CISS_TMF_NOT_SUPPORTED:
> +	case CISS_TMF_FAILED:
> +	case CISS_TMF_WRONG_LUN:
> +	case CISS_TMF_OVERLAPPED_TAG:
> +		break;
> +	default:
> +		dev_warn(&h->pdev->dev, "Unknown TMF status: 0x%02x\n",
> +				tmf_status);
> +		break;
> +	}
> +	return -tmf_status;
> +}
> +
>  static void complete_scsi_command(struct CommandList *cp)
>  {
>  	struct scsi_cmnd *cmd;
> @@ -1867,9 +1929,9 @@ static void complete_scsi_command(struct CommandList *cp)
>  	struct ErrorInfo *ei;
>  	struct hpsa_scsi_dev_t *dev;
>  
> -	unsigned char sense_key;
> -	unsigned char asc;      /* additional sense code */
> -	unsigned char ascq;     /* additional sense code qualifier */
> +	int sense_key;
> +	int asc;      /* additional sense code */
> +	int ascq;     /* additional sense code qualifier */
>  	unsigned long sense_data_size;
>  
>  	ei = cp->err_info;
> @@ -1904,8 +1966,6 @@ static void complete_scsi_command(struct CommandList *cp)
>  	if (cp->cmd_type == CMD_IOACCEL2)
>  		return process_ioaccel2_completion(h, cp, cmd, dev);
>  
> -	cmd->result |= ei->ScsiStatus;
> -
>  	scsi_set_resid(cmd, ei->ResidualCnt);
>  	if (ei->CommandStatus == 0) {
>  		if (cp->cmd_type == CMD_IOACCEL1)
> @@ -1915,16 +1975,6 @@ static void complete_scsi_command(struct CommandList *cp)
>  		return;
>  	}
>  
> -	/* copy the sense data */
> -	if (SCSI_SENSE_BUFFERSIZE < sizeof(ei->SenseInfo))
> -		sense_data_size = SCSI_SENSE_BUFFERSIZE;
> -	else
> -		sense_data_size = sizeof(ei->SenseInfo);
> -	if (ei->SenseLen < sense_data_size)
> -		sense_data_size = ei->SenseLen;
> -
> -	memcpy(cmd->sense_buffer, ei->SenseInfo, sense_data_size);
> -
>  	/* For I/O accelerator commands, copy over some fields to the normal
>  	 * CISS header used below for error handling.
>  	 */
> @@ -1956,14 +2006,18 @@ static void complete_scsi_command(struct CommandList *cp)
>  	switch (ei->CommandStatus) {
>  
>  	case CMD_TARGET_STATUS:
> -		if (ei->ScsiStatus) {
> -			/* Get sense key */
> -			sense_key = 0xf & ei->SenseInfo[2];
> -			/* Get additional sense code */
> -			asc = ei->SenseInfo[12];
> -			/* Get addition sense code qualifier */
> -			ascq = ei->SenseInfo[13];
> -		}
> +		cmd->result |= ei->ScsiStatus;
> +		/* copy the sense data */
> +		if (SCSI_SENSE_BUFFERSIZE < sizeof(ei->SenseInfo))
> +			sense_data_size = SCSI_SENSE_BUFFERSIZE;
> +		else
> +			sense_data_size = sizeof(ei->SenseInfo);
> +		if (ei->SenseLen < sense_data_size)
> +			sense_data_size = ei->SenseLen;
> +		memcpy(cmd->sense_buffer, ei->SenseInfo, sense_data_size);
> +		if (ei->ScsiStatus)
> +			decode_sense_data(ei->SenseInfo, sense_data_size,
> +				&sense_key, &asc, &ascq);
>  		if (ei->ScsiStatus == SAM_STAT_CHECK_CONDITION) {
>  			if (sense_key == ABORTED_COMMAND) {
>  				cmd->result |= DID_SOFT_ERROR << 16;
> @@ -2058,6 +2112,10 @@ static void complete_scsi_command(struct CommandList *cp)
>  		cmd->result = DID_ERROR << 16;
>  		dev_warn(&h->pdev->dev, "Command unabortable\n");
>  		break;
> +	case CMD_TMF_STATUS:
> +		if (hpsa_evaluate_tmf_status(h, cp)) /* TMF failed? */
> +			cmd->result = DID_ERROR << 16;
> +		break;
>  	case CMD_IOACCEL_DISABLED:
>  		/* This only handles the direct pass-through case since RAID
>  		 * offload is handled above.  Just attempt a retry.
> @@ -2208,16 +2266,22 @@ static void hpsa_scsi_interpret_error(struct ctlr_info *h,
>  {
>  	const struct ErrorInfo *ei = cp->err_info;
>  	struct device *d = &cp->h->pdev->dev;
> -	const u8 *sd = ei->SenseInfo;
> +	int sense_key, asc, ascq, sense_len;
>  
>  	switch (ei->CommandStatus) {
>  	case CMD_TARGET_STATUS:
> +		if (ei->SenseLen > sizeof(ei->SenseInfo))
> +			sense_len = sizeof(ei->SenseInfo);
> +		else
> +			sense_len = ei->SenseLen;
> +		decode_sense_data(ei->SenseInfo, sense_len,
> +					&sense_key, &asc, &ascq);
>  		hpsa_print_cmd(h, "SCSI status", cp);
>  		if (ei->ScsiStatus == SAM_STAT_CHECK_CONDITION)
> -			dev_warn(d, "SCSI Status = 02, Sense key = %02x, ASC = %02x, ASCQ = %02x\n",
> -				sd[2] & 0x0f, sd[12], sd[13]);
> +			dev_warn(d, "SCSI Status = 02, Sense key = 0x%02x, ASC = 0x%02x, ASCQ = 0x%02x\n",
> +				sense_key, asc, ascq);
>  		else
> -			dev_warn(d, "SCSI Status = %02x\n", ei->ScsiStatus);
> +			dev_warn(d, "SCSI Status = 0x%02x\n", ei->ScsiStatus);
>  		if (ei->ScsiStatus == 0)
>  			dev_warn(d, "SCSI status is abnormally zero.  "
>  			"(probably indicates selection timeout "
> @@ -2762,7 +2826,8 @@ static int hpsa_volume_offline(struct ctlr_info *h,
>  					unsigned char scsi3addr[])
>  {
>  	struct CommandList *c;
> -	unsigned char *sense, sense_key, asc, ascq;
> +	unsigned char *sense;
> +	int sense_key, asc, ascq, sense_len;
>  	int rc, ldstat = 0;
>  	u16 cmd_status;
>  	u8 scsi_status;
> @@ -2780,9 +2845,11 @@ static int hpsa_volume_offline(struct ctlr_info *h,
>  		return 0;
>  	}
>  	sense = c->err_info->SenseInfo;
> -	sense_key = sense[2];
> -	asc = sense[12];
> -	ascq = sense[13];
> +	if (c->err_info->SenseLen > sizeof(c->err_info->SenseInfo))
> +		sense_len = sizeof(c->err_info->SenseInfo);
> +	else
> +		sense_len = c->err_info->SenseLen;
> +	decode_sense_data(sense, sense_len, &sense_key, &asc, &ascq);
>  	cmd_status = c->err_info->CommandStatus;
>  	scsi_status = c->err_info->ScsiStatus;
>  	cmd_free(h, c);
> @@ -2858,6 +2925,9 @@ static int hpsa_device_supports_aborts(struct ctlr_info *h,
>  	case CMD_ABORT_FAILED:
>  		rc = 1;
>  		break;
> +	case CMD_TMF_STATUS:
> +		rc = hpsa_evaluate_tmf_status(h, c);
> +		break;
>  	default:
>  		rc = 0;
>  		break;
> @@ -4665,6 +4735,9 @@ static int hpsa_send_abort(struct ctlr_info *h, unsigned char *scsi3addr,
>  	switch (ei->CommandStatus) {
>  	case CMD_SUCCESS:
>  		break;
> +	case CMD_TMF_STATUS:
> +		rc = hpsa_evaluate_tmf_status(h, c);
> +		break;
>  	case CMD_UNABORTABLE: /* Very common, don't make noise. */
>  		rc = -1;
>  		break;

Hmm. It feels weird, to have ASC and ASCQ values as integers ...
any specifc reasons for this?
If not, why can't you keep them as unsigned char ?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html