From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934915AbaH0OMZ (ORCPT ); Wed, 27 Aug 2014 10:12:25 -0400 Received: from cantor2.suse.de ([195.135.220.15]:36625 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934604AbaH0OMV (ORCPT ); Wed, 27 Aug 2014 10:12:21 -0400 Message-ID: <53FDE743.2040705@suse.de> Date: Wed, 27 Aug 2014 16:12:19 +0200 From: Hannes Reinecke User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Yoshihiro YUNOMAE CC: linux-scsi@vger.kernel.org, "Martin K. Petersen" , yrl.pp-manager.tt@hitachi.com, linux-kernel@vger.kernel.org, "James E.J. Bottomley" , Hidehiro Kawai , Doug Gilbert , Masami Hiramatsu , Christoph Hellwig Subject: Re: [RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point instead of printk References: <20140808115004.6768.97014.stgit@yuno-kbuild.novalocal> <20140808115022.6768.33712.stgit@yuno-kbuild.novalocal> In-Reply-To: <20140808115022.6768.33712.stgit@yuno-kbuild.novalocal> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > (printk) > sd 2:0:0:0: [sda] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE > > (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 > Cc: Hannes Reinecke > Cc: Doug Gilbert > Cc: Martin K. Petersen > Cc: Christoph Hellwig > Cc: "James E.J. Bottomley" > Cc: Hidehiro Kawai > Cc: Masami Hiramatsu > --- > 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 > #include > > +#include > + > #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@vger.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@suse.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)