| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 Aug 2014 16:12:19 +0200
From: Hannes Reinecke <hare@...e.de>
To: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@...achi.com>
CC: linux-scsi@...r.kernel.org,
"Martin K. Petersen" <martin.petersen@...cle.com>,
yrl.pp-manager.tt@...achi.com, linux-kernel@...r.kernel.org,
"James E.J. Bottomley" <JBottomley@...allels.com>,
Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
Doug Gilbert <dgilbert@...erlog.com>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Christoph Hellwig <hch@....de>
Subject: Re: [RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point
instead of printk
On 08/08/2014 01:50 PM, Yoshihiro YUNOMAE wrote:
> Current SCSI trace has hostbyte table and driverbyte table, so we don't need to
> have the same table in scsi/constants.c.
>
> - Result examples
>
> <Before> (printk)
> sd 2:0:0:0: [sda] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>
> <After> (ftrace)
> scsi_show_result: host_no=2 channel=0 id=0 lun=0 [sda] result=(driver=DRIVER_SENSE host=DID_OK)
>
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@...achi.com>
> Cc: Hannes Reinecke <hare@...e.de>
> Cc: Doug Gilbert <dgilbert@...erlog.com>
> Cc: Martin K. Petersen <martin.petersen@...cle.com>
> Cc: Christoph Hellwig <hch@....de>
> Cc: "James E.J. Bottomley" <JBottomley@...allels.com>
> Cc: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> ---
> drivers/scsi/constants.c | 52 -------------------------------------------
> drivers/scsi/scsi_trace.c | 16 +++++++++++++
> include/trace/events/scsi.h | 38 +++++++++++++++++++++++++++++++
> 3 files changed, 53 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 6fad6b4..f7b7f32 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -1488,55 +1488,3 @@ void scsi_print_sense(struct scsi_cmnd *cmd)
> SCSI_SENSE_BUFFERSIZE);
> }
> EXPORT_SYMBOL(scsi_print_sense);
> -
> -#ifdef CONFIG_SCSI_CONSTANTS
> -
> -static const char * const hostbyte_table[]={
> -"DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT", "DID_BAD_TARGET",
> -"DID_ABORT", "DID_PARITY", "DID_ERROR", "DID_RESET", "DID_BAD_INTR",
> -"DID_PASSTHROUGH", "DID_SOFT_ERROR", "DID_IMM_RETRY", "DID_REQUEUE",
> -"DID_TRANSPORT_DISRUPTED", "DID_TRANSPORT_FAILFAST", "DID_TARGET_FAILURE",
> -"DID_NEXUS_FAILURE" };
> -#define NUM_HOSTBYTE_STRS ARRAY_SIZE(hostbyte_table)
> -
> -static const char * const driverbyte_table[]={
> -"DRIVER_OK", "DRIVER_BUSY", "DRIVER_SOFT", "DRIVER_MEDIA", "DRIVER_ERROR",
> -"DRIVER_INVALID", "DRIVER_TIMEOUT", "DRIVER_HARD", "DRIVER_SENSE"};
> -#define NUM_DRIVERBYTE_STRS ARRAY_SIZE(driverbyte_table)
> -
> -void scsi_show_result(struct scsi_device *sdev, const char *name, int result)
> -{
> - int hb = host_byte(result);
> - int db = driver_byte(result);
> - const char *hb_string;
> - const char *db_string;
> -
> - hb_string = (hb < NUM_HOSTBYTE_STRS) ? hostbyte_table[hb] : "invalid";
> - db_string = (db < NUM_DRIVERBYTE_STRS) ?
> - driverbyte_table[db] : "invalid";
> -
> -
> - sdev_printk(KERN_INFO, sdev,
> - "[%s] Result: hostbyte=%s driverbyte=%s\n",
> - name, hb_string, db_string);
> -}
> -
> -#else
> -
> -void scsi_show_result(struct scsi_device *sdev, const char *name, int result)
> -{
> - sdev_printk(KERN_INFO, sdev,
> - "[%s] Result: hostbyte=0x%02x driverbyte=0x%02x\n",
> - name, host_byte(result), driver_byte(result));
> -}
> -
> -#endif
> -EXPORT_SYMBOL(scsi_show_result);
> -
> -void scsi_print_result(struct scsi_cmnd *cmd)
> -{
> - const char *devname = cmd->request->rq_disk ?
> - cmd->request->rq_disk->disk_name : "scsi";
> - scsi_show_result(cmd->device, devname, cmd->result);
> -}
> -EXPORT_SYMBOL(scsi_print_result);
> diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c
> index 2bea4f0..6ffbc40 100644
> --- a/drivers/scsi/scsi_trace.c
> +++ b/drivers/scsi/scsi_trace.c
> @@ -19,6 +19,8 @@
> #include <linux/trace_seq.h>
> #include <trace/events/scsi.h>
>
> +#include <scsi/scsi_dbg.h>
> +
> #define SERVICE_ACTION16(cdb) (cdb[1] & 0x1f)
> #define SERVICE_ACTION32(cdb) ((cdb[8] << 8) | cdb[9])
>
> @@ -286,3 +288,17 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len)
> return scsi_trace_misc(p, cdb, len);
> }
> }
> +
> +void scsi_show_result(struct scsi_device *sdev, const char *name, int result)
> +{
> + trace_scsi_show_result(sdev, name, result);
> +}
> +EXPORT_SYMBOL(scsi_show_result);
> +
> +void scsi_print_result(struct scsi_cmnd *cmd)
> +{
> + const char *devname = cmd->request->rq_disk ?
> + cmd->request->rq_disk->disk_name : "scsi";
> + scsi_show_result(cmd->device, devname, cmd->result);
> +}
> +EXPORT_SYMBOL(scsi_print_result);
> diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
> index 8aecdc2..0675195 100644
> --- a/include/trace/events/scsi.h
> +++ b/include/trace/events/scsi.h
> @@ -123,7 +123,11 @@
> scsi_hostbyte_name(DID_IMM_RETRY), \
> scsi_hostbyte_name(DID_REQUEUE), \
> scsi_hostbyte_name(DID_TRANSPORT_DISRUPTED), \
> - scsi_hostbyte_name(DID_TRANSPORT_FAILFAST))
> + scsi_hostbyte_name(DID_TRANSPORT_FAILFAST), \
> + scsi_hostbyte_name(DID_TARGET_FAILURE), \
> + scsi_hostbyte_name(DID_NEXUS_FAILURE), \
> + scsi_hostbyte_name(DID_ALLOC_FAILURE), \
> + scsi_hostbyte_name(DID_MEDIUM_ERROR))
>
> #define scsi_driverbyte_name(result) { result, #result }
> #define show_driverbyte_name(val) \
> @@ -359,6 +363,38 @@ TRACE_EVENT(scsi_eh_wakeup,
> TP_printk("host_no=%u", __entry->host_no)
> );
>
> +TRACE_EVENT(scsi_show_result,
> +
> + TP_PROTO(struct scsi_device *sdev, const char *devname, int result),
> +
> + TP_ARGS(sdev, devname, result),
> +
> + TP_STRUCT__entry(
> + __field( unsigned int, host_no )
> + __field( unsigned int, channel )
> + __field( unsigned int, id )
> + __field( unsigned int, lun )
> + __string(devname, devname )
> + __field( int, result )
> + ),
> +
> + TP_fast_assign(
> + __entry->host_no = sdev->host->host_no;
> + __entry->channel = sdev->channel;
> + __entry->id = sdev->id;
> + __entry->lun = sdev->lun;
> + __assign_str(devname, devname);
> + __entry->result = result;
> + ),
> +
> + TP_printk("host_no=%u channel=%u id=%u lun=%u [%s] result=(driver=%s "\
> + "host=%s)",
> + __entry->host_no, __entry->channel, __entry->id, __entry->lun,
> + __get_str(devname),
> + show_driverbyte_name(driver_byte(__entry->result)),
> + show_hostbyte_name(host_byte(__entry->result)))
> +);
> +
> #endif /* _TRACE_SCSI_H */
>
> /* This part must be outside protection */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hmm.
I'm not sure this is the correct way.
Currently we have quite some code duplication in scsi_trace.c and
constants.c, correct.
So I definitely would like to see them both merged.
But constants.c is influenced by CONFIG_SCSI_CONSTANTS, whereas
scsi_trace isn't, and the functions in constants.c are used
throughout the scsi stack.
So I'd rather see to have scsi_trace to be updated to use the
functions from constants.c, and remove the duplicate code in
scsi_trace.
At a later step we should be working on removing/replacing existing
logging code in the SCSI stack with trace points, but that should be
reserved for a separate patchset.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@...e.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists