Heitor Ricardo Alves de Siqueira <hal...@linux.vnet.ibm.com> writes:
> Hello,
>
> This commit implements a new iprdumpfmt command that extracts driver
> information
> from ipr dump files. It strips out driver version and IOA location, prints the
> driver trace in the correct order and displays information from the smart dump
> tables. It should be working correctly on both LE and BE dump formats.
>
> I would greatly appreciate any feedback on the code, especially on output
> formatting.
Hi Heitor,
Thanks a lot for following through with this. The patch is getting
pretty good and I only have a few comments. Some of them are regarding
C style but I also left some comments on the logic and the output
formatting.
Feel free to ping me if you have questions about the comments I've made.
Thanks!
>
> Thanks.
>
> Signed-off-by: Heitor Ricardo Alves de Siqueira <hal...@linux.vnet.ibm.com>
> ---
> Makefile.am | 5 +-
> iprdumpfmt.c | 683
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 687 insertions(+), 1 deletion(-)
> create mode 100644 iprdumpfmt.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 4636ecbaf8ce..96b0cd2a5c4c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -10,7 +10,7 @@
> noinst_LTLIBRARIES = libipr.la
> libipr_la_SOURCES = iprlib.c iprlib.h
>
> -sbin_PROGRAMS = iprconfig iprdump iprupdate iprinit iprdbg
> +sbin_PROGRAMS = iprconfig iprdump iprupdate iprinit iprdbg iprdumpfmt
> dist_sbin_SCRIPTS = iprsos
>
> iprconfig_SOURCES = iprconfig.c iprconfig.h
> @@ -28,6 +28,9 @@ iprinit_LDADD= libipr.la
> iprdbg_SOURCES = iprdbg.c
> iprdbg_LDADD= libipr.la
>
> +iprdumpfmt_SOURCES = iprdumpfmt.c
> +iprdumpfmt_LDADD = libipr.la
> +
Build system looks good.
> if STATIC_BUILD
> sbin_PROGRAMS += iprconfig-static
>
> diff --git a/iprdumpfmt.c b/iprdumpfmt.c
> new file mode 100644
> index 000000000000..e5dc10a292c5
> --- /dev/null
> +++ b/iprdumpfmt.c
> @@ -0,0 +1,683 @@
> +#include <endian.h>
> +#include "iprlib.h"
> +
> +#define EYE_CATCHER_BE 0xC5D4E3F2
> +#define EYE_CATCHER_LE 0xF2E3D4C5
> +
> +#define OS_LINUX 0x4C4E5558
> +#define OS_I5OS 0x69354F53
> +
> +#define DRV_IPR2 0x49505232
> +#define DRV_V5R4 0x56355234
> +
> +#define TYPE_ASCII 0x41534349
> +#define TYPE_BIN 0x42494E41
> +#define TYPE_MR32 0x4D523332
> +#define TYPE_MR64 0x4D523634
> +
> +#define ID_IOA_DUMP 0x494F4131
> +#define ID_IOA_LOC 0x4C4F4341
> +#define ID_DRV_TRACE 0x54524143
> +#define ID_DRV_VER 0x44525652
> +#define ID_DRV_TYPE 0x54595045
> +#define ID_DRV_CTRL_BLK 0x494F4342
> +#define ID_DRV_PEND_OPS 0x414F5053
> +
> +#define IPR_DUMP_STATUS_SUCCESS 0
> +#define IPR_DUMP_STATUS_QUAL_SUCCESS 2
> +#define IPR_DUMP_STATUS_FAILED 0xFFFFFFFF
> +
> +#define IPR_FMT2_SDT_READY_TO_USE 0xC4D4E3F2
> +#define IPR_FMT3_SDT_READY_TO_USE 0xC4D4E3F3
> +
> +static const struct {
> + const u8 op;
> + const char *name;
> +} scsi_cmnds[] = {
> + { 0xc1, "IPR_CANCEL_REQUEST" },
> + { 0x01, "IPR_CANCEL_64BIT_IOARCB" },
> + { 0xc2, "IPR_QUERY_RSRC_STATE" },
> + { 0xc3, "IPR_RESET_DEVICE" },
> + { 0x80, "IPR_RESET_TYPE_SELECT" },
> + { 0x40, "IPR_LUN_RESET" },
> + { 0x20, "IPR_TARGET_RESET" },
> + { 0x10, "IPR_BUS_RESET" },
> + { 0x80, "IPR_ATA_PHY_RESET" },
> + { 0xc4, "IPR_ID_HOST_RR_Q" },
> + { 0xc5, "IPR_QUERY_IOA_CONFIG" },
> + { 0xce, "IPR_CANCEL_ALL_REQUESTS" },
> + { 0xcf, "IPR_HOST_CONTROLLED_ASYNC" },
> + { 0x01, "IPR_HCAM_CDB_OP_CODE_CONFIG_CHANGE" },
> + { 0x02, "IPR_HCAM_CDB_OP_CODE_LOG_DATA" },
> + { 0xfb, "IPR_SET_SUPPORTED_DEVICES" },
> + { 0x80, "IPR_SET_ALL_SUPPORTED_DEVICES" },
> + { 0xf7, "IPR_IOA_SHUTDOWN" },
> + { 0x05, "IPR_WR_BUF_DOWNLOAD_AND_SAVE" },
> +
> + { 0x00, "TEST_UNIT_READY" },
> + { 0x01, "REZERO_UNIT" },
> + { 0x03, "REQUEST_SENSE" },
> + { 0x04, "FORMAT_UNIT" },
> + { 0x05, "READ_BLOCK_LIMITS" },
> + { 0x07, "REASSIGN_BLOCKS" },
> + { 0x07, "INITIALIZE_ELEMENT_STATUS" },
> + { 0x08, "READ_6" },
> + { 0x0a, "WRITE_6" },
> + { 0x0b, "SEEK_6" },
> + { 0x0f, "READ_REVERSE" },
> + { 0x10, "WRITE_FILEMARKS" },
> + { 0x11, "SPACE" },
> + { 0x12, "INQUIRY" },
> + { 0x14, "RECOVER_BUFFERED_DATA" },
> + { 0x15, "MODE_SELECT" },
> + { 0x16, "RESERVE" },
> + { 0x17, "RELEASE" },
> + { 0x18, "COPY" },
> + { 0x19, "ERASE" },
> + { 0x1a, "MODE_SENSE" },
> + { 0x1b, "START_STOP" },
> + { 0x1c, "RECEIVE_DIAGNOSTIC" },
> + { 0x1d, "SEND_DIAGNOSTIC" },
> + { 0x1e, "ALLOW_MEDIUM_REMOVAL" },
> + { 0x23, "READ_FORMAT_CAPACITIES" },
> + { 0x24, "SET_WINDOW" },
> + { 0x25, "READ_CAPACITY" },
> + { 0x28, "READ_10" },
> + { 0x2a, "WRITE_10" },
> + { 0x2b, "SEEK_10" },
> + { 0x2b, "POSITION_TO_ELEMENT" },
> + { 0x2e, "WRITE_VERIFY" },
> + { 0x2f, "VERIFY" },
> + { 0x30, "SEARCH_HIGH" },
> + { 0x31, "SEARCH_EQUAL" },
> + { 0x32, "SEARCH_LOW" },
> + { 0x33, "SET_LIMITS" },
> + { 0x34, "PRE_FETCH" },
> + { 0x34, "READ_POSITION" },
> + { 0x35, "SYNCHRONIZE_CACHE" },
> + { 0x36, "LOCK_UNLOCK_CACHE" },
> + { 0x37, "READ_DEFECT_DATA" },
> + { 0x38, "MEDIUM_SCAN" },
> + { 0x39, "COMPARE" },
> + { 0x3a, "COPY_VERIFY" },
> + { 0x3b, "WRITE_BUFFER" },
> + { 0x3c, "READ_BUFFER" },
> + { 0x3d, "UPDATE_BLOCK" },
> + { 0x3e, "READ_LONG" },
> + { 0x3f, "WRITE_LONG" },
> + { 0x40, "CHANGE_DEFINITION" },
> + { 0x41, "WRITE_SAME" },
> + { 0x42, "UNMAP" },
> + { 0x43, "READ_TOC" },
> + { 0x44, "READ_HEADER" },
> + { 0x4a, "GET_EVENT_STATUS_NOTIFICATION" },
> + { 0x4c, "LOG_SELECT" },
> + { 0x4d, "LOG_SENSE" },
> + { 0x53, "XDWRITEREAD_10" },
> + { 0x55, "MODE_SELECT_10" },
> + { 0x56, "RESERVE_10" },
> + { 0x57, "RELEASE_10" },
> + { 0x5a, "MODE_SENSE_10" },
> + { 0x5e, "PERSISTENT_RESERVE_IN" },
> + { 0x5f, "PERSISTENT_RESERVE_OUT" },
> + { 0x7f, "VARIABLE_LENGTH_CMD" },
> + { 0xa0, "REPORT_LUNS" },
> + { 0xa2, "SECURITY_PROTOCOL_IN" },
> + { 0xa3, "MAINTENANCE_IN" },
> + { 0xa4, "MAINTENANCE_OUT" },
> + { 0xa5, "MOVE_MEDIUM" },
> + { 0xa6, "EXCHANGE_MEDIUM" },
> + { 0xa8, "READ_12" },
> + { 0xa9, "SERVICE_ACTION_OUT_12" },
> + { 0xaa, "WRITE_12" },
> + { 0xab, "SERVICE_ACTION_IN_12" },
> + { 0xae, "WRITE_VERIFY_12" },
> + { 0xaf, "VERIFY_12" },
> + { 0xb0, "SEARCH_HIGH_12" },
> + { 0xb1, "SEARCH_EQUAL_12" },
> + { 0xb2, "SEARCH_LOW_12" },
> + { 0xb5, "SECURITY_PROTOCOL_OUT" },
> + { 0xb8, "READ_ELEMENT_STATUS" },
> + { 0xb6, "SEND_VOLUME_TAG" },
> + { 0xea, "WRITE_LONG_2" },
> + { 0x83, "EXTENDED_COPY" },
> + { 0x84, "RECEIVE_COPY_RESULTS" },
> + { 0x86, "ACCESS_CONTROL_IN" },
> + { 0x87, "ACCESS_CONTROL_OUT" },
> + { 0x88, "READ_16" },
> + { 0x89, "COMPARE_AND_WRITE" },
> + { 0x8a, "WRITE_16" },
> + { 0x8c, "READ_ATTRIBUTE" },
> + { 0x8d, "WRITE_ATTRIBUTE" },
> + { 0x8f, "VERIFY_16" },
> + { 0x91, "SYNCHRONIZE_CACHE_16" },
> + { 0x93, "WRITE_SAME_16" },
> + { 0x9d, "SERVICE_ACTION_BIDIRECTIONAL" },
> + { 0x9e, "SERVICE_ACTION_IN_16" },
> + { 0x9f, "SERVICE_ACTION_OUT_16" }
> +};
> +
> +struct ipr_dump_header {
> + u32 eye_catcher;
> + u32 len;
> + u32 num_entries;
> + u32 first_entry_offset;
> + u32 status;
> + u32 os;
> + u32 driver_name;
> +};
> +
> +struct ipr_dump_entry_header {
> + u32 eye_catcher;
> + u32 len; /* does not include entry header */
> + u32 num_elems;
> + u32 offset; /* offset to data from beginning of dump file */
> + u32 data_type;
> + u32 id;
> + u32 status;
> +};
> +
> +struct ipr_trace_entry {
> + u32 time;
> +
> + u8 op_code; /* SCSI opcode */
> + u8 ata_op_code;
> + u8 type;
> +#define IPR_TRACE_START 0x00
> +#define IPR_TRACE_FINISH 0xff
> + u8 cmd_index;
> +
> + u32 res_handle; /* stored as BE */
> + union {
> + u32 ioasc;
> + u32 add_data;
> + u32 res_addr;
> + } u;
> +};
> +
> +struct ipr_sdt_header {
> + /* all values in ipr_sdt_header are stored as BE */
> + u32 state;
> + u32 num_entries;
> + u32 entries_used;
> + u32 dump_size;
> +};
> +
> +struct ipr_sdt {
> + struct ipr_sdt_header hdr;
> + struct ipr_sdt_entry *entry;
> +};
> +
> +struct dump_node {
> + u32 pos;
> + struct ipr_dump_entry_header hdr;
> + struct dump_node *next;
> +};
> +
> +struct trace_node {
> + struct ipr_trace_entry t;
> + struct trace_node *next;
> +};
> +
> +void *dump_map;
> +FILE *out_fp;
Add some comments for these global variables.
> +
> +/**
> + * get_scsi_command_literal -
> + * @value: SCSI opcode
> + *
> + * Returns:
> + * SCSI command string
> + **/
> +static const char* get_scsi_command_literal(u32 value)
> +{
> + u32 i = 0;
> +
> + for (i=0; i<ARRAY_SIZE(scsi_cmnds); ++i) {
> + if (value == scsi_cmnds[i].op)
> + return scsi_cmnds[i].name;
> + }
> + return "UNKNOWN";
> +}
Suggest returning NULL instead of UNKNOWN..
> +
> +/**
> + * read_dump_header -
> + * @fd: dump file descriptor
> + *
> + * Returns:
> + * ipr dump header structure
> + **/
> +static struct ipr_dump_header read_dump_header(int fd)
> +{
> + struct ipr_dump_header hdr;
> +
> + read(fd, &hdr, sizeof(hdr));
No need to read the main header separately. Just mmap the entire file
at once.
> + hdr.eye_catcher = htobe32(hdr.eye_catcher);
> +
> + /* convert to correct endianness */
> + if (hdr.eye_catcher == EYE_CATCHER_BE) {
> + hdr.len = be32toh(hdr.len);
> + hdr.num_entries = be32toh(hdr.num_entries);
> + hdr.first_entry_offset = be32toh(hdr.first_entry_offset);
> + hdr.status = be32toh(hdr.status);
> + hdr.os = be32toh(hdr.os);
> + hdr.driver_name = be32toh(hdr.driver_name);
> + } else {
> + hdr.len = le32toh(hdr.len);
> + hdr.num_entries = le32toh(hdr.num_entries);
> + hdr.first_entry_offset = le32toh(hdr.first_entry_offset);
> + hdr.status = le32toh(hdr.status);
> + hdr.os = le32toh(hdr.os);
> + hdr.driver_name = le32toh(hdr.driver_name);
> + }
> +
> + return hdr;
> +}
> +
> +/**
> + * print_dump_header -
> + * @hdr: ipr dump header
> + *
> + * Returns:
> + * nothing
> + **/
> +static void print_dump_header(struct ipr_dump_header hdr)
> +{
> + fprintf(out_fp, "IPR Adapter Dump Report\n\n");
> + switch (hdr.eye_catcher) {
> + case EYE_CATCHER_BE:
> + fprintf(out_fp, "Big Endian format: ");
> + break;
> + case EYE_CATCHER_LE:
> + fprintf(out_fp, "Little Endian format: ");
> + break;
> + default:
> + fprintf(out_fp, "Unknown dump format: ");
> + }
> + fprintf(out_fp, "Eye Catcher is 0x%08X\n", hdr.eye_catcher);
> +
> + fprintf(out_fp, "Total length: %d bytes\n", hdr.len);
> + fprintf(out_fp, "Number of dump entries: %d\n", hdr.num_entries);
> + fprintf(out_fp, "Offset to first entry: 0x%08X bytes\n",
> + hdr.first_entry_offset);
> + fprintf(out_fp, "Dump status: 0x%08X ", hdr.status);
> + switch (hdr.status) {
> + case IPR_DUMP_STATUS_SUCCESS:
> + fprintf(out_fp, "(SUCCESS)\n");
> + break;
> + case IPR_DUMP_STATUS_QUAL_SUCCESS:
> + fprintf(out_fp, "(QUAL_SUCCESS)\n");
> + break;
> + case IPR_DUMP_STATUS_FAILED:
> + fprintf(out_fp, "(FAILED)\n");
> + break;
> + default:
> + fprintf(out_fp, "(UNKNOWN)\n");
> + }
> +
> + fprintf(out_fp, "Operating System: ");
> + switch (hdr.os) {
> + case OS_LINUX:
> + fprintf(out_fp, "Linux\n");
> + break;
> + case OS_I5OS:
> + fprintf(out_fp, "i5OS\n");
> + break;
> + default:
> + fprintf(out_fp, "unknown\n");
> + }
> +
> + fprintf(out_fp, "Driver: ");
> + switch (hdr.driver_name) {
> + case DRV_IPR2:
> + fprintf(out_fp, "ipr2\n");
> + break;
> + case DRV_V5R4:
> + fprintf(out_fp, "V5R4\n");
> + break;
> + default:
> + fprintf(out_fp, "unknown\n");
> + }
> +}
> +
> +/**
> + * read_entry_header -
> + * @hdr: ipr dump entry header
> + * @offset: offset to entry data
> + *
> + * Returns:
> + * offset to next entry header
> + **/
> +static u32 read_entry_header(struct ipr_dump_entry_header *hdr, u32 offset)
> +{
> + memcpy(hdr, dump_map+offset, sizeof(*hdr));
> + hdr->eye_catcher = htobe32(hdr->eye_catcher);
hmm, instead of memcpying the values and them updating them, just cast
ipr_dump_entry_header on the dump+map structure and do your endianness
conversion to your hdr using the cast as source. What do you think?
struct ipr_dump_entry_header *raw_hdr =
(struct ipr_dump_entry_header *) (dump_map+offset);
then you can just do:
hdr->len = target_to_host32(raw++hdr->len);
And avoid the explicit memcpy.
> +
> + /* convert to correct endianness */
> + if (hdr->eye_catcher == EYE_CATCHER_BE) {
> + hdr->len = be32toh(hdr->len);
> + hdr->num_elems = be32toh(hdr->num_elems);
> + hdr->offset = be32toh(hdr->offset);
> + hdr->data_type = be32toh(hdr->data_type);
> + hdr->id = be32toh(hdr->id);
> + hdr->status = be32toh(hdr->status);
> + } else if (hdr->eye_catcher == EYE_CATCHER_LE) {
> + hdr->len = le32toh(hdr->len);
> + hdr->num_elems = le32toh(hdr->num_elems);
> + hdr->offset = le32toh(hdr->offset);
> + hdr->data_type = le32toh(hdr->data_type);
> + hdr->id = le32toh(hdr->id);
> + hdr->status = le32toh(hdr->status);
> + }
Don't wanna keep pushing with the endiannes stuff, but I think you could
make things much easier by just creating something like
#define target_to_host32(val) \
(target_endiannes == host_endianness) ? val : convert32(val);
Then, hunks like this will be much simpler:
hdr->len = target_to_host32(hdr->len);
hdr->num_elems = target_to_host32(hdr->num_elems);
hdr->offset = target_to_host32(hdr->offset);
hdr->data_type = target_to_host32(hdr->data_type);
hdr->id = target_to_host32(hdr->id);
hdr->status = target_to_host32(hdr->status);
My point being that we can reduce the size of the code and the number of
places where we might introduce endianness bugs.
> + hdr->offset += offset;
> +
> + return (hdr->offset + hdr->len);
> +}
> +
> +/**
> + * read_trace_data -
> + * @offset: offset to entry data
> + * @eye_catcher: ipr entry eye catcher
> + *
> + * Returns:
> + * ipr trace entry structure
> + **/
> +static struct ipr_trace_entry read_trace_data(u32 offset, u32 eye_catcher)
> +{
> + struct ipr_trace_entry trace;
> + memcpy(&trace, dump_map+offset, sizeof(trace));
> +
> + /* always stored as BE */
> + trace.res_handle = htobe32(trace.res_handle);
> +
> + /* convert to correct endianness */
> + if (eye_catcher == EYE_CATCHER_BE) {
> + trace.time = be32toh(trace.time);
> + trace.u.add_data = be32toh(trace.u.add_data);
> + } else {
> + trace.time = le32toh(trace.time);
> + trace.u.add_data = le32toh(trace.u.add_data);
> + }
> + return trace;
> +}
> +
> +/**
> + * print_trace_data -
> + * @trace: ipr trace entry
> + *
> + * Returns:
> + * Nothing
> + **/
> +static void print_trace_data(struct ipr_trace_entry trace)
pass trace as a reference instead of copying the structure.
> +{
> + fprintf(out_fp, "\t Timestamp: %d (%lld us)\n", trace.time,
> + (long long unsigned)trace.time*1000*1000/sysconf(_SC_CLK_TCK));
> + fprintf(out_fp, "\t SCSI Opcode: 0x%02X ", trace.op_code);
> + fprintf(out_fp, "\t (%s)\n", get_scsi_command_literal(trace.op_code));
> + fprintf(out_fp, "\t ATA Opcode: 0x%02X\n", trace.ata_op_code);
> + fprintf(out_fp, "\t Type: 0x%02X ", trace.type);
> + switch (trace.type) {
> + case IPR_TRACE_START:
> + fprintf(out_fp, "(IPR_TRACE_START)\n");
> + break;
> + case IPR_TRACE_FINISH:
> + fprintf(out_fp, "(IPR_TRACE_FINISH)\n");
> + break;
> + default:
> + fprintf(out_fp, "(UNKNOWN)\n");
> + }
> + fprintf(out_fp, "\t Command index: 0x02%X\n", trace.cmd_index);
> + fprintf(out_fp, "\t Res Handle: 0x%08X\n", trace.res_handle);
> + fprintf(out_fp, "\t Additional Data: 0x08%X\n", trace.u.add_data);
Regarding the output format. Since we are going to print several trace
entries, maybe we want to print each entry in a single line, more
similar to the old version of this tool, in a tabular fashion. What do
you think? I think it might be easier to analyze the dump later.
Are trace.ata_op_code and trace.op_code valid at the same time? I
recall we discussed these fields some time ago, but I don't remember the
outcome.
> +}
> +
> +/**
> + * build_sdt -
> + * @hdr: ipr dump entry header
> + *
> + * Returns:
> + * ipr sdt structure
> + **/
> +static struct ipr_sdt build_sdt(struct ipr_dump_entry_header hdr)
Pass hdr as a const reference instead of by value here.
> +{
> + u32 i, offset;
> + struct ipr_sdt sdt;
> + struct ipr_sdt_entry *entry;
> +
> + memcpy(&sdt.hdr, dump_map+hdr.offset, sizeof(sdt.hdr));
> + offset = hdr.offset + sizeof(sdt.hdr);
> +
> + /* values below are always stored as BE */
> + sdt.hdr.state = htobe32(sdt.hdr.state);
> + sdt.hdr.num_entries = htobe32(sdt.hdr.num_entries);
> + sdt.hdr.entries_used = htobe32(sdt.hdr.entries_used);
> + sdt.hdr.dump_size = htobe32(sdt.hdr.dump_size);
> +
> + sdt.entry = malloc(sdt.hdr.entries_used * sizeof(*sdt.entry));
> + for (i=0; i<sdt.hdr.entries_used; ++i) {
> + entry = &sdt.entry[i];
> +
> + memcpy(entry, dump_map+offset+i*sizeof(*entry), sizeof(*entry));
> + /* values below are always stored as BE */
> + entry->bar_str_offset = htobe32(entry->bar_str_offset);
> + entry->end_offset = htobe32(entry->end_offset);
> + entry->priority = htobe16(entry->priority);
> + }
> +
> + return sdt;
> +}
> +
> +/**
> + * build_trace_list -
> + * @hdr: ipr dump entry header
> + *
> + * Returns:
> + * circular list of ipr trace entries
> + **/
> +static struct trace_node* build_trace_list(struct ipr_dump_entry_header hdr)
Pass hdr by const reference instead of by value here.
> +{
> + u32 i, min_time, min_pos = 0;
> + struct trace_node *head = malloc(sizeof(*head));
> + struct trace_node *aux = head;
> +
> + head->t = read_trace_data(hdr.offset, hdr.eye_catcher);
> + min_time = head->t.time;
> + for (i=1; i<hdr.len/sizeof(head->t); ++i) {
> + aux->next = malloc(sizeof(*aux->next));
> + aux = aux->next;
> + aux->t = read_trace_data(hdr.offset+i*sizeof(head->t),
> + hdr.eye_catcher);
> + if (min_time > aux->t.time) {
> + min_time = aux->t.time;
> + min_pos = i;
> + }
> + }
> + aux->next = head;
> + while (min_pos--)
> + head = head->next;
> + return head;
> +}
> +
> +/**
> + * clean_trace_list -
> + * @head: circular list of ipr trace entries
> + *
> + * Returns:
> + * nothing
> + **/
> +static void clean_trace_list(struct trace_node* head)
> +{
> + struct trace_node* aux = head->next;
> + struct trace_node* del;
> + while (aux != head) {
> + del = aux;
> + aux = aux->next;
> + free(del);
> + }
> + free(head);
> +}
> +
> +/**
> + * print_sdt -
> + * @sdt: ipr sdt structure
> + *
> + * Returns:
> + * nothing
> + **/
> +static void print_sdt(struct ipr_sdt sdt)
> +{
> + u32 i;
> + struct ipr_sdt_entry *entry;
> +
> + fprintf(out_fp, "Smart Dump Table Data:\n");
> + fprintf(out_fp, "\t state: 0x%X ", sdt.hdr.state);
> + switch(sdt.hdr.state) {
> + case IPR_FMT2_SDT_READY_TO_USE:
> + case IPR_FMT3_SDT_READY_TO_USE:
> + fprintf(out_fp, "(READY_TO_USE)\n");
> + break;
> + default:
> + fprintf(out_fp, "(UNKNOWN)\n");
> + }
If sdt.hdr.state is UNKNOWN, is it worth printing?
> + fprintf(out_fp, "\t entries found: %d\n\n", sdt.hdr.entries_used);
> +
> + for (i=0; i<sdt.hdr.entries_used; ++i) {
> + entry = &sdt.entry[i];
> + fprintf(out_fp, "\t start offset: 0x%08X\n",
> + entry->bar_str_offset);
> + fprintf(out_fp, "\t end offset: 0x%08X\n", entry->end_offset);
> + fprintf(out_fp, "\t entry byte: 0x%02X\n", entry->entry_byte);
> + fprintf(out_fp, "\t endian: 0x%X\n", entry->endian);
> + fprintf(out_fp, "\t valid entry: 0x%X\n", entry->valid_entry);
> + fprintf(out_fp, "\t resv: %d\n", entry->resv);
> + fprintf(out_fp, "\t priority: 0x%04X\n", entry->priority);
> + fprintf(out_fp, "\n");
> + }
> +}
> +
> +/**
> + * build_trace_list -
> + * @hdr: ipr dump entry header
> + *
> + * Returns:
> + * nothing
> + **/
> +static void dump_entry_data(struct ipr_dump_entry_header hdr)
> +{
> + u32 i;
> + char *s;
> + struct trace_node *tn, *tn_aux;
> + struct ipr_sdt sdt;
> +
> + switch (hdr.id) {
> + case ID_IOA_DUMP:
> + sdt = build_sdt(hdr);
> + print_sdt(sdt);
> + free(sdt.entry);
> + break;
Can't we put sdt in the stack and avoid the malloc ?
> + case ID_IOA_LOC:
> + fprintf(out_fp, "IOA Location:\n");
> + s = dump_map+hdr.offset;
> + fprintf(out_fp, "\t %.*s\n", hdr.len, s);
> + break;
> + case ID_DRV_VER:
> + fprintf(out_fp, "Driver Version:\n");
> + s = dump_map+hdr.offset;
> + fprintf(out_fp, "\t %.*s\n", hdr.len, s);
> + break;
> + case ID_DRV_TRACE:
> + fprintf(out_fp, "Driver Trace Entries:\n");
> + tn = build_trace_list(hdr);
> + tn_aux = tn;
> + print_trace_data(tn->t);
> + tn = tn->next;
> + while (tn != tn_aux) {
> + fprintf(out_fp, "\n");
> + print_trace_data(tn->t);
> + tn = tn->next;
> + }
> + clean_trace_list(tn);
Can't we find and print the trace entries at once to avoid malloc-ing
and freeing the list all the time? Since this is a ring buffer of trace
entries, we might be able to cast them to a list of headers, find the
head using the timestamp and them iterate from there. What do you
think? Let's put all of this logic into a separate print_trace_data
function.
This way we eliminate build_trace_list and clean_trace_list.
> + break;
> + case ID_DRV_TYPE:
> + fprintf(out_fp, "Driver Type:\n");
> + memcpy(&i, dump_map+hdr.offset, sizeof(i));
> + if (hdr.eye_catcher == EYE_CATCHER_BE)
> + i = be32toh(i);
> + else
> + i = le32toh(i);
No need to memcpy. Just cast from dump_map+hdr.offset straight into the
be32toh. Something like this could replace this hunk:
i = target_to_host(*((u32*) dump_map+hdr.offset));
> + fprintf(out_fp, "\t CCIN: 0x%04X\n", i);
> + memcpy(&i, dump_map+hdr.offset+sizeof(i), sizeof(i));
> + if (hdr.eye_catcher == EYE_CATCHER_BE)
> + i = be32toh(i);
> + else
> + i = le32toh(i);
Likewise.
> + fprintf(out_fp, "\t Adapter firmware Version: ");
> + fprintf(out_fp, " %02X:%02X:%02X:%02X\n",
> + i >> 24, /* major release */
> + (i & 0x00FF0000) >> 16, /* card type */
> + (i & 0x0000FF00) >> 8, /* minor release 0 */
> + i & 0x000000FF); /* minor release 1 */
> + break;
> + case ID_DRV_CTRL_BLK:
> + case ID_DRV_PEND_OPS:
> + /* not implemented yet */
> + fprintf(stderr, "Warning: found IPR Control Block or ");
> + fprintf(stderr, "IPR Pending Ops structure!\n");
> + fprintf(stderr, "These are not implemented
> yet. Skipping...\n");
Join these fprintf calls into a single statement.
> + break;
> + default:
> + fprintf(stderr,
> + "Warning: did not recognize data identification ");
> + fprintf(stderr, "(id 0x%08X). Skipping...\n", hdr.id);
Likewise.
> + }
> + fprintf(out_fp, "\n");
Suggest putting the \n with the fprintf calls so we don't have it here
alone here.
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + u32 i, offset;
> + int in_fd;
> + struct ipr_dump_header hdr;
> + struct ipr_dump_entry_header entry_hdr;
> +
> + if (argc != 3) {
> + fprintf(stderr, "Usage: %s <dump_file> <report_file>\n",
> + argv[0]);
> + return 1;
> + }
> +
> + in_fd = open(argv[1], O_RDONLY);
> + out_fp = fopen(argv[2], "w");
> +
> + if (in_fd < 0) {
> + fprintf(stderr, "Error: could not open dump file %s\n",
> + argv[1]);
> + fprintf(stderr, "Aborting...\n");
Suggest printing errno here. Much more helpful. See strerror(3).
> + return 1;
> + }
> + if (out_fp == NULL) {
> + fprintf(stderr, "Error: could not open report file %s\n",
> + argv[2]);
> + fprintf(stderr, "Aborting...\n");
Likewise.
> + return 1;
> + }
> +
> + hdr = read_dump_header(in_fd);
> + print_dump_header(hdr);
> + fprintf(out_fp, "\n");
> +
> + dump_map = mmap(NULL, hdr.len, PROT_READ, MAP_SHARED, in_fd, 0);
Don't use MAP_SHARED.
> + if (dump_map == MAP_FAILED) {
> + fprintf(stderr, "Error: could not map dump file to memory\n");
> + fprintf(stderr, "Aborting...\n");
> + return 1;
errno.
> + }
> +
> + offset = hdr.first_entry_offset;
> + for (i=0; i<hdr.num_entries; ++i) {
^^ missing spaces.
> + offset = read_entry_header(&entry_hdr, offset);
> + dump_entry_data(entry_hdr);
> + }
> +
> + return 0;
> +}
Thanks!
--
Gabriel Krisman Bertazi
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
