RE: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

["Elliott, Robert (Server Storage)" <Elliott@hp.com>]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Wednesday, 25 June, 2014 6:07 AM
> To: Maurizio Lombardi
> Cc: James.Bottomley@HansenPartnership.com; linux-scsi@vger.kernel.org;
> hch@infradead.org
> Subject: Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code"
> messages.
> 
> Can I get another review for this one?
> 
> On Fri, Jun 06, 2014 at 10:10:35AM +0200, Maurizio Lombardi wrote:
...
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
...
> > @@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
> unsigned int good_bytes)
> >  			action = ACTION_FAIL;
> >  			break;
> >  		default:
> > -			description = "Unhandled sense code";
> > +			description = "Failing command with sense code:";

The default case handles sense keys other than:
	#define NOT_READY           0x02
	#define ILLEGAL_REQUEST     0x05
	#define UNIT_ATTENTION      0x06
	#define ABORTED_COMMAND     0x0b
	#define VOLUME_OVERFLOW     0x0d

which means these #defines (and any other values)
#define NO_SENSE            0x00
#define RECOVERED_ERROR     0x01
#define MEDIUM_ERROR        0x03
#define HARDWARE_ERROR      0x04
#define DATA_PROTECT        0x07
#define BLANK_CHECK         0x08
#define COPY_ABORTED        0x0a
#define MISCOMPARE          0x0e

The other description strings that result in ACTION_FAIL are based
on the sense key and sometimes the additional sense code:
description = "Media Changed";
description = "Host Data Integrity Failure";
description = "Discard failure";
description = "Write same failure";
description = "Invalid command failure";
description = "Target Data Integrity Failure";
description = "Device not ready";

Also, there is this one, which is not based on the sense key:
description = "Command timed out";

Since the ACTION_FAIL case always prints the sense key
and additional sense code:
	if (!(req->cmd_flags & REQ_QUIET)) {
		if (description)
			scmd_printk(KERN_INFO, cmd, " % \n",
				description);
		scsi_print_result(cmd);
		if (driver_byte(result) & DRIVER_SENSE)
			scsi_print_sense("", cmd);
		scsi_print_command(cmd);
	}

perhaps the description string should be removed altogether?

In this example:
	sd 1:0:0:0: [sdb] Unhandled sense code
	sd 1:0:0:0: [sdb]
	Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
	sd 1:0:0:0: [sdb]
	Sense Key : Medium Error [current]
	sd 1:0:0:0: [sdb]
	Add. Sense: Unrecovered read error
	sd 1:0:0:0: [sdb] CDB:
	Read(10): 28 00 00 18 f8 a8 00 00 08 00
	end_request: critical medium error, dev sdb, sector 1636520

"Sense Key : Medium Error" is much more informative than
"Unhandled sense code" or "Failing command with sense code".
All the other descriptions represent failing commands,
so using different wording is a bit confusing.

For the "Unhandled error code" (for which you are proposing
removing the string) and the timeout case, the scsi_print_result 
call already prints hostbyte and driverbyte, which explain what 
happened in more detail:

	sd 1:0:0:0: [sda] Unhandled error code                                          
	sd 1:0:0:0: [sda]                                                               
	Result: hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK                              
	sd 1:0:0:0: [sda] CDB:                                                          
	Test Unit Ready:                                                                
	end_request: I/O error, dev sda, sector 481013632                               

---
Rob Elliott    HP Server Storage