Re: [PATCH 4/4] scsi: Trace SCSI sense data

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Fri, Apr 28, 2023 at 11:36:29AM -0700, Bart Van Assche wrote:
> On 4/28/23 01:09, Niklas Cassel wrote:
> > Do we really need to dump the whole sense buffer?
> > 
> > Shouldn't simply printing the SK, ASC, ASCQ be sufficient?
> Hi Niklas,
> 
> How about replacing patch 4/4 of this series with the patch below?
> 
> Thanks,
> 
> Bart.
> 
> 
> 
> diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
> index 370ade0d4093..d4a27cd4040b 100644
> --- a/include/trace/events/scsi.h
> +++ b/include/trace/events/scsi.h
> @@ -258,9 +258,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__field( unsigned int,	prot_sglen )
>  		__field( unsigned char,	prot_op )
>  		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
> +		__field( u8, sense_key )
> +		__field( u8, asc )
> +		__field( u8, ascq )
>  	),
> 
>  	TP_fast_assign(
> +		struct scsi_sense_hdr sshdr;
> +
>  		__entry->host_no	= cmd->device->host->host_no;
>  		__entry->channel	= cmd->device->channel;
>  		__entry->id		= cmd->device->id;
> @@ -272,11 +277,21 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
>  		__entry->prot_op	= scsi_get_prot_op(cmd);
>  		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
> +		if (cmd->result & 0xff &&
> +		    scsi_command_normalize_sense(cmd, &sshdr)) {

Looks good to me, but considering that a command can be SAM_STAT_GOOD
(which is defined as 0x00), and still have sense data (e.g. CDL policy 0xD
timeout), perhaps you could use the same check as in:
https://lore.kernel.org/linux-ide/20230406113252.41211-1-nks@flawful.org/T/#me609b85c48b4048561662b0a4b1d102e19d7a13d

i.e., something like:
	if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
	    scsi_command_normalize_sense(cmd, &sshdr)) {

instead of cmd->result & 0xff.

(Yes, I know that the CDL series is not merged yet, but the point is that a
SCSI command can be SAM_STAT_GOOD and still have valid sense data.)


Kind regards,
Niklas


> +			__entry->sense_key = sshdr.sense_key;
> +			__entry->asc = sshdr.asc;
> +			__entry->ascq = sshdr.ascq;
> +		} else {
> +			__entry->sense_key = 0;
> +			__entry->asc = 0;
> +			__entry->ascq = 0;
> +		}
>  	),
> 
>  	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u " \
>  		  "prot_sgl=%u prot_op=%s cmnd=(%s %s raw=%s) result=(driver=" \
> -		  "%s host=%s message=%s status=%s)",
> +		  "%s host=%s message=%s status=%s sense_key=%u asc=%#x ascq=%#x)",
>  		  __entry->host_no, __entry->channel, __entry->id,
>  		  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
>  		  show_prot_op_name(__entry->prot_op),
> @@ -286,7 +301,8 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		  "DRIVER_OK",
>  		  show_hostbyte_name(((__entry->result) >> 16) & 0xff),
>  		  "COMMAND_COMPLETE",
> -		  show_statusbyte_name(__entry->result & 0xff))
> +		  show_statusbyte_name(__entry->result & 0xff),
> +		  __entry->sense_key, __entry->asc, __entry->ascq)
>  );
> 
>  DEFINE_EVENT(scsi_cmd_done_timeout_template, scsi_dispatch_cmd_done,
>