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@xxxxxxxxxxx/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,
>
