[U-Boot] [PATCH v2 0/6] Iotrace improvements

[U-Boot] [PATCH v2 0/6] Iotrace improvements

[Ramon Fried]

These set of patches add few improvements to iotrace.
* Region limiting - allows setting an address and size where only
					io operations that falls into that address are
					logged.
* Timestamping - Timestamp every iotrace record with current timestamp
* dumping - iotrace dump command for dumping all records from buffer
			in a readable fashion.

In terms of backwards compatibility, the timestamp is not backward
compatible as it changes the iotrace record. so if one developed an
offline parsing tool it will be broken.
I though of adding #ifdef specific for that, but eventually I didn't.

v2:
	* fixed printf format
	* added a fix when the buffer is full

Ramon Fried (6):
  cmd: iotrace: add set region command
  iotrace: add IO region limit
  common: iotrace: add timestamp to iotrace records
  iotrace: move record definitons to header file
  cmd: iotrace: add dump trace command
  common: iotrace: fix behaviour when buffer is full

 cmd/iotrace.c     | 58 ++++++++++++++++++++++++++++++++++++++++++++++-
 common/iotrace.c  | 55 ++++++++++++++++++++++++--------------------
 include/iotrace.h | 52 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 26 deletions(-)

-- 
2.17.0

[U-Boot] [PATCH 1/6] cmd: iotrace: add set region command

[Ramon Fried]

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
---
 cmd/iotrace.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/cmd/iotrace.c b/cmd/iotrace.c
index e496787e0d..601b8c8e32 100644
--- a/cmd/iotrace.c
+++ b/cmd/iotrace.c
@@ -15,6 +15,9 @@ static void do_print_stats(void)
 	iotrace_get_buffer(&start, &size, &offset, &count);
 	printf("Start:  %08lx\n", start);
 	printf("Size:   %08lx\n", size);
+	iotrace_get_region(&start, &size);
+	printf("Region: %08lx\n", start);
+	printf("Size:   %08lx\n", size);
 	printf("Offset: %08lx\n", offset);
 	printf("Output: %08lx\n", start + offset);
 	printf("Count:  %08lx\n", count);
@@ -37,6 +40,22 @@ static int do_set_buffer(int argc, char * const argv[])
 	return 0;
 }
 
+static int do_set_region(int argc, char * const argv[])
+{
+	ulong addr = 0, size = 0;
+
+	if (argc == 2) {
+		addr = simple_strtoul(*argv++, NULL, 16);
+		size = simple_strtoul(*argv++, NULL, 16);
+	} else if (argc != 0) {
+		return CMD_RET_USAGE;
+	}
+
+	iotrace_set_region(addr, size);
+
+	return 0;
+}
+
 int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	const char *cmd = argc < 2 ? NULL : argv[1];
@@ -46,6 +65,8 @@ int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	switch (*cmd) {
 	case 'b':
 		return do_set_buffer(argc - 2, argv + 2);
+	case 'l':
+		return do_set_region(argc - 2, argv + 2);
 	case 'p':
 		iotrace_set_enabled(0);
 		break;
@@ -67,6 +88,7 @@ U_BOOT_CMD(
 	"iotrace utility commands",
 	"stats                        - display iotrace stats\n"
 	"iotrace buffer <address> <size>      - set iotrace buffer\n"
+	"iotrace limit <address> <size>       - set iotrace region limit\n"
 	"iotrace pause                        - pause tracing\n"
 	"iotrace resume                       - resume tracing"
 );
-- 
2.17.0

[U-Boot] [PATCH 1/6] cmd: iotrace: add set region command

[Simon Glass]

On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> ---
>  cmd/iotrace.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>


Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/6] iotrace: add IO region limit

[Ramon Fried]

When dealing with a lot of IO regions, sometimes
it makes sense only to trace a specific one.
This patch adds support for region limits.
If region is not set, the iotrace works the same as it was.
If region is set, the iotrace only logs io operation that falls
in the defined region.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
---
 common/iotrace.c  | 27 +++++++++++++++++++++++++++
 include/iotrace.h | 24 ++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/common/iotrace.c b/common/iotrace.c
index b16b0d612d..f39885663a 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -42,6 +42,8 @@ struct iotrace_record {
  * @start:	Start address of iotrace buffer
  * @size:	Size of iotrace buffer in bytes
  * @offset:	Current write offset into iotrace buffer
+ * @region_start: Address of IO region to trace
+ * @region_size: Size of region to trace. if 0 will trace all address space
  * @crc32:	Current value of CRC chceksum of trace records
  * @enabled:	true if enabled, false if disabled
  */
@@ -49,6 +51,8 @@ static struct iotrace {
 	ulong start;
 	ulong size;
 	ulong offset;
+	ulong region_start;
+	ulong region_size;
 	u32 crc32;
 	bool enabled;
 } iotrace;
@@ -66,6 +70,11 @@ static void add_record(int flags, const void *ptr, ulong value)
 	if (!(gd->flags & GD_FLG_RELOC) || !iotrace.enabled)
 		return;
 
+	if (iotrace.region_size)
+		if ((ulong)ptr < iotrace.region_start ||
+		    (ulong)ptr > iotrace.region_start + iotrace.region_size)
+			return;
+
 	/* Store it if there is room */
 	if (iotrace.offset + sizeof(*rec) < iotrace.size) {
 		rec = (struct iotrace_record *)map_sysmem(
@@ -142,6 +151,24 @@ u32 iotrace_get_checksum(void)
 	return iotrace.crc32;
 }
 
+void iotrace_set_region(ulong start, ulong size)
+{
+	iotrace.region_start = start;
+	iotrace.region_size = size;
+}
+
+void iotrace_reset_region(void)
+{
+	iotrace.region_start = 0;
+	iotrace.region_size = 0;
+}
+
+void iotrace_get_region(ulong *start, ulong *size)
+{
+	*start = iotrace.region_start;
+	*size = iotrace.region_size;
+}
+
 void iotrace_set_enabled(int enable)
 {
 	iotrace.enabled = enable;
diff --git a/include/iotrace.h b/include/iotrace.h
index 9fe5733f87..1efb117343 100644
--- a/include/iotrace.h
+++ b/include/iotrace.h
@@ -58,6 +58,30 @@ void iotrace_reset_checksum(void);
  */
 u32 iotrace_get_checksum(void);
 
+/**
+ * iotrace_set_region() - Set whether iotrace is limited to a specific
+ * io region.
+ *
+ * Defines the address and size of the limited region.
+ *
+ * @start: address of the beginning of the region
+ * @size: size of the region in bytes.
+ */
+void iotrace_set_region(ulong start, ulong size);
+
+/**
+ * iotrace_reset_region() - Reset the region limit
+ */
+void iotrace_reset_region(void);
+
+/**
+ * iotrace_get_region() - Get region information
+ *
+ * @start: Returns start address of region
+ * @size: Returns size of region in bytes
+ */
+void iotrace_get_region(ulong *start, ulong *size);
+
 /**
  * iotrace_set_enabled() - Set whether iotracing is enabled or not
  *
-- 
2.17.0

[U-Boot] [PATCH 2/6] iotrace: add IO region limit

[Simon Glass]

On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
> When dealing with a lot of IO regions, sometimes
> it makes sense only to trace a specific one.
> This patch adds support for region limits.
> If region is not set, the iotrace works the same as it was.
> If region is set, the iotrace only logs io operation that falls
> in the defined region.
>
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> ---
>  common/iotrace.c  | 27 +++++++++++++++++++++++++++
>  include/iotrace.h | 24 ++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/6] common: iotrace: add timestamp to iotrace records

[Ramon Fried]

Add timestamp to each iotrace record to aid in debugging
of IO timing access bugs.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
---
 common/iotrace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/iotrace.c b/common/iotrace.c
index f39885663a..3530688ab1 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -27,11 +27,13 @@ enum iotrace_flags {
  * struct iotrace_record - Holds a single I/O trace record
  *
  * @flags: I/O access type
+ * @timestamp: Timestamp of access
  * @addr: Address of access
  * @value: Value written or read
  */
 struct iotrace_record {
 	enum iotrace_flags flags;
+	u64 timestamp;
 	phys_addr_t addr;
 	iovalue_t value;
 };
@@ -82,6 +84,7 @@ static void add_record(int flags, const void *ptr, ulong value)
 					sizeof(value));
 	}
 
+	rec->timestamp = get_ticks();
 	rec->flags = flags;
 	rec->addr = map_to_sysmem(ptr);
 	rec->value = value;
-- 
2.17.0

[U-Boot] [PATCH 3/6] common: iotrace: add timestamp to iotrace records

[Simon Glass]

HI Ramon,

On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
> Add timestamp to each iotrace record to aid in debugging
> of IO timing access bugs.
>
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> ---
>  common/iotrace.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/common/iotrace.c b/common/iotrace.c
> index f39885663a..3530688ab1 100644
> --- a/common/iotrace.c
> +++ b/common/iotrace.c
> @@ -27,11 +27,13 @@ enum iotrace_flags {
>   * struct iotrace_record - Holds a single I/O trace record
>   *
>   * @flags: I/O access type
> + * @timestamp: Timestamp of access
>   * @addr: Address of access
>   * @value: Value written or read
>   */
>  struct iotrace_record {
>         enum iotrace_flags flags;
> +       u64 timestamp;
>         phys_addr_t addr;
>         iovalue_t value;
>  };
> @@ -82,6 +84,7 @@ static void add_record(int flags, const void *ptr, ulong value)
>                                         sizeof(value));
>         }
>
> +       rec->timestamp = get_ticks();

Would it not be better to use timer_get_us() here?

>         rec->flags = flags;
>         rec->addr = map_to_sysmem(ptr);
>         rec->value = value;
> --
> 2.17.0
>

Regards,
Simon

[U-Boot] [PATCH 3/6] common: iotrace: add timestamp to iotrace records

[Ramon Fried]

On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote:
> HI Ramon,
>
> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
>> Add timestamp to each iotrace record to aid in debugging
>> of IO timing access bugs.
>>
>> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>> ---
>>  common/iotrace.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/common/iotrace.c b/common/iotrace.c
>> index f39885663a..3530688ab1 100644
>> --- a/common/iotrace.c
>> +++ b/common/iotrace.c
>> @@ -27,11 +27,13 @@ enum iotrace_flags {
>>   * struct iotrace_record - Holds a single I/O trace record
>>   *
>>   * @flags: I/O access type
>> + * @timestamp: Timestamp of access
>>   * @addr: Address of access
>>   * @value: Value written or read
>>   */
>>  struct iotrace_record {
>>         enum iotrace_flags flags;
>> +       u64 timestamp;
>>         phys_addr_t addr;
>>         iovalue_t value;
>>  };
>> @@ -82,6 +84,7 @@ static void add_record(int flags, const void *ptr, ulong value)
>>                                         sizeof(value));
>>         }
>>
>> +       rec->timestamp = get_ticks();
>
> Would it not be better to use timer_get_us() here?
Yes. I'll change it. Thanks.
>
>>         rec->flags = flags;
>>         rec->addr = map_to_sysmem(ptr);
>>         rec->value = value;
>> --
>> 2.17.0
>>
>
> Regards,
> Simon

[U-Boot] [PATCH 4/6] iotrace: move record definitons to header file

[Ramon Fried]

The header definitions are needed for reading
record information in cmd/iotrace.c

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
---
 common/iotrace.c  | 27 ---------------------------
 include/iotrace.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/common/iotrace.c b/common/iotrace.c
index 3530688ab1..74408a5dbb 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -11,33 +11,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/* Support up to the machine word length for now */
-typedef ulong iovalue_t;
-
-enum iotrace_flags {
-	IOT_8 = 0,
-	IOT_16,
-	IOT_32,
-
-	IOT_READ = 0 << 3,
-	IOT_WRITE = 1 << 3,
-};
-
-/**
- * struct iotrace_record - Holds a single I/O trace record
- *
- * @flags: I/O access type
- * @timestamp: Timestamp of access
- * @addr: Address of access
- * @value: Value written or read
- */
-struct iotrace_record {
-	enum iotrace_flags flags;
-	u64 timestamp;
-	phys_addr_t addr;
-	iovalue_t value;
-};
-
 /**
  * struct iotrace - current trace status and checksum
  *
diff --git a/include/iotrace.h b/include/iotrace.h
index 1efb117343..b6e006ced0 100644
--- a/include/iotrace.h
+++ b/include/iotrace.h
@@ -7,6 +7,34 @@
 #define __IOTRACE_H
 
 #include <linux/types.h>
+#include <common.h>
+
+/* Support up to the machine word length for now */
+typedef ulong iovalue_t;
+
+enum iotrace_flags {
+	IOT_8 = 0,
+	IOT_16,
+	IOT_32,
+
+	IOT_READ = 0 << 3,
+	IOT_WRITE = 1 << 3,
+};
+
+/**
+ * struct iotrace_record - Holds a single I/O trace record
+ *
+ * @flags: I/O access type
+ * @timestamp: Timestamp of access
+ * @addr: Address of access
+ * @value: Value written or read
+ */
+struct iotrace_record {
+	enum iotrace_flags flags;
+	u64 timestamp;
+	phys_addr_t addr;
+	iovalue_t value;
+};
 
 /*
  * This file is designed to be included in arch/<arch>/include/asm/io.h.
-- 
2.17.0

[U-Boot] [PATCH 4/6] iotrace: move record definitons to header file

[Simon Glass]

On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
> The header definitions are needed for reading
> record information in cmd/iotrace.c
>
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> ---
>  common/iotrace.c  | 27 ---------------------------
>  include/iotrace.h | 28 ++++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 27 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 5/6] cmd: iotrace: add dump trace command

[Ramon Fried]

Add dump trace command which dump all trace
buffer content in a much more readable fashion
than md.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
---
v2: fixed printf zero field width.

 cmd/iotrace.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/cmd/iotrace.c b/cmd/iotrace.c
index 601b8c8e32..f50a3c3a1a 100644
--- a/cmd/iotrace.c
+++ b/cmd/iotrace.c
@@ -24,6 +24,36 @@ static void do_print_stats(void)
 	printf("CRC32:  %08lx\n", (ulong)iotrace_get_checksum());
 }
 
+static void do_print_trace(void)
+{
+	ulong start, size, offset, count;
+
+	struct iotrace_record *cur_record;
+
+	iotrace_get_buffer(&start, &size, &offset, &count);
+
+	if (!start || !size || !count)
+		return;
+
+	printf("Timestamp  Value          Address\n");
+
+	cur_record = (struct iotrace_record *)start;
+	for (int i = 0; i < count; i++) {
+		if (cur_record->flags & IOT_WRITE)
+			printf("%08llu: 0x%08lx --> 0x%08lx\n",
+			       cur_record->timestamp,
+					cur_record->value,
+					cur_record->addr);
+		else
+			printf("%08llu: 0x%08lx <-- 0x%08lx\n",
+			       cur_record->timestamp,
+					cur_record->value,
+					cur_record->addr);
+
+		cur_record++;
+	}
+}
+
 static int do_set_buffer(int argc, char * const argv[])
 {
 	ulong addr = 0, size = 0;
@@ -76,6 +106,9 @@ int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	case 's':
 		do_print_stats();
 		break;
+	case 'd':
+		do_print_trace();
+		break;
 	default:
 		return CMD_RET_USAGE;
 	}
@@ -90,5 +123,6 @@ U_BOOT_CMD(
 	"iotrace buffer <address> <size>      - set iotrace buffer\n"
 	"iotrace limit <address> <size>       - set iotrace region limit\n"
 	"iotrace pause                        - pause tracing\n"
-	"iotrace resume                       - resume tracing"
+	"iotrace resume                       - resume tracing\n"
+	"iotrace dump                         - dump iotrace buffer"
 );
-- 
2.17.0

[U-Boot] [PATCH v2 5/6] cmd: iotrace: add dump trace command

[Simon Glass]

On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
> Add dump trace command which dump all trace
> buffer content in a much more readable fashion
> than md.
>
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> ---
> v2: fixed printf zero field width.
>
>  cmd/iotrace.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 6/6] common: iotrace: fix behaviour when buffer is full

[Ramon Fried]

When the buffer is full, there supposed to be no more
writes, the code however misses the else statement and
subsequently writes to arbitrary pointer location and increases
the offset.
This patch fixes that by returning immediately.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
---
 common/iotrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/iotrace.c b/common/iotrace.c
index 74408a5dbb..5f06d2b250 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -55,6 +55,8 @@ static void add_record(int flags, const void *ptr, ulong value)
 		rec = (struct iotrace_record *)map_sysmem(
 					iotrace.start + iotrace.offset,
 					sizeof(value));
+	} else {
+		return;
 	}
 
 	rec->timestamp = get_ticks();
-- 
2.17.0

[U-Boot] [PATCH 6/6] common: iotrace: fix behaviour when buffer is full

[Simon Glass]

Hi Ramon,

On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
> When the buffer is full, there supposed to be no more
> writes, the code however misses the else statement and
> subsequently writes to arbitrary pointer location and increases
> the offset.

I don't think so. It writes to a local variable in this case. The
point of this is to detect how much space would be needed to hold the
I/O trace. Unless the pointer is incremented, there is no way to know.

Perhaps instead, iotrace_get_buffer() should be updated to also return
the number of valid records, as well as the pointer value?

> This patch fixes that by returning immediately.
>
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> ---
>  common/iotrace.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/common/iotrace.c b/common/iotrace.c
> index 74408a5dbb..5f06d2b250 100644
> --- a/common/iotrace.c
> +++ b/common/iotrace.c
> @@ -55,6 +55,8 @@ static void add_record(int flags, const void *ptr, ulong value)
>                 rec = (struct iotrace_record *)map_sysmem(
>                                         iotrace.start + iotrace.offset,
>                                         sizeof(value));
> +       } else {
> +               return;
>         }
>
>         rec->timestamp = get_ticks();
> --
> 2.17.0
>

Regards,
Simon

[U-Boot] [PATCH 6/6] common: iotrace: fix behaviour when buffer is full

[Ramon Fried]

On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Ramon,
>
> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
>> When the buffer is full, there supposed to be no more
>> writes, the code however misses the else statement and
>> subsequently writes to arbitrary pointer location and increases
>> the offset.
>
> I don't think so. It writes to a local variable in this case. The
> point of this is to detect how much space would be needed to hold the
> I/O trace. Unless the pointer is incremented, there is no way to know.
You're right. I missed the initial assignment to rec.

>
> Perhaps instead, iotrace_get_buffer() should be updated to also return
> the number of valid records, as well as the pointer value?
>
It's a valid option, another one I have in mind is that
we can return immediately like I suggested but add one time warning
that states that the
buffer is full ?

>> This patch fixes that by returning immediately.
>>
>> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>> ---
>>  common/iotrace.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/common/iotrace.c b/common/iotrace.c
>> index 74408a5dbb..5f06d2b250 100644
>> --- a/common/iotrace.c
>> +++ b/common/iotrace.c
>> @@ -55,6 +55,8 @@ static void add_record(int flags, const void *ptr, ulong value)
>>                 rec = (struct iotrace_record *)map_sysmem(
>>                                         iotrace.start + iotrace.offset,
>>                                         sizeof(value));
>> +       } else {
>> +               return;
>>         }
>>
>>         rec->timestamp = get_ticks();
>> --
>> 2.17.0
>>
>
> Regards,
> Simon

[U-Boot] [PATCH 6/6] common: iotrace: fix behaviour when buffer is full

[Simon Glass]

Hi Ramon,

On 26 May 2018 at 00:05, Ramon Fried <ramon.fried@gmail.com> wrote:
> On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Ramon,
>>
>> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
>>> When the buffer is full, there supposed to be no more
>>> writes, the code however misses the else statement and
>>> subsequently writes to arbitrary pointer location and increases
>>> the offset.
>>
>> I don't think so. It writes to a local variable in this case. The
>> point of this is to detect how much space would be needed to hold the
>> I/O trace. Unless the pointer is incremented, there is no way to know.
> You're right. I missed the initial assignment to rec.
>
>>
>> Perhaps instead, iotrace_get_buffer() should be updated to also return
>> the number of valid records, as well as the pointer value?
>>
> It's a valid option, another one I have in mind is that
> we can return immediately like I suggested but add one time warning
> that states that the
> buffer is full ?

The problem is that people want to be able to resize the buffer so
that they can try again, so they need to know the correct size.

Can you make it report that it overflowed, and print the correct size?

Regards,
Simon

[U-Boot] [PATCH 6/6] common: iotrace: fix behaviour when buffer is full

[Ramon Fried]

On Sun, May 27, 2018 at 1:18 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Ramon,
>
> On 26 May 2018 at 00:05, Ramon Fried <ramon.fried@gmail.com> wrote:
>> On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Ramon,
>>>
>>> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
>>>> When the buffer is full, there supposed to be no more
>>>> writes, the code however misses the else statement and
>>>> subsequently writes to arbitrary pointer location and increases
>>>> the offset.
>>>
>>> I don't think so. It writes to a local variable in this case. The
>>> point of this is to detect how much space would be needed to hold the
>>> I/O trace. Unless the pointer is incremented, there is no way to know.
>> You're right. I missed the initial assignment to rec.
>>
>>>
>>> Perhaps instead, iotrace_get_buffer() should be updated to also return
>>> the number of valid records, as well as the pointer value?
>>>
>> It's a valid option, another one I have in mind is that
>> we can return immediately like I suggested but add one time warning
>> that states that the
>> buffer is full ?
>
> The problem is that people want to be able to resize the buffer so
> that they can try again, so they need to know the correct size.
>
> Can you make it report that it overflowed, and print the correct size?
The correct size can only be printed in the end of the tracing.
Maybe we can use WARN_ONCE when it first occurs and later when the user
type "iotrace stats" it will state the required buffer size to
accommodate all of the
entries.
What do you think ?
>
> Regards,
> Simon

[U-Boot] [PATCH 6/6] common: iotrace: fix behaviour when buffer is full

[Simon Glass]

Hi Ramon,

On 27 May 2018 at 02:35, Ramon Fried <ramon.fried@gmail.com> wrote:
> On Sun, May 27, 2018 at 1:18 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Ramon,
>>
>> On 26 May 2018 at 00:05, Ramon Fried <ramon.fried@gmail.com> wrote:
>>> On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Ramon,
>>>>
>>>> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote:
>>>>> When the buffer is full, there supposed to be no more
>>>>> writes, the code however misses the else statement and
>>>>> subsequently writes to arbitrary pointer location and increases
>>>>> the offset.
>>>>
>>>> I don't think so. It writes to a local variable in this case. The
>>>> point of this is to detect how much space would be needed to hold the
>>>> I/O trace. Unless the pointer is incremented, there is no way to know.
>>> You're right. I missed the initial assignment to rec.
>>>
>>>>
>>>> Perhaps instead, iotrace_get_buffer() should be updated to also return
>>>> the number of valid records, as well as the pointer value?
>>>>
>>> It's a valid option, another one I have in mind is that
>>> we can return immediately like I suggested but add one time warning
>>> that states that the
>>> buffer is full ?
>>
>> The problem is that people want to be able to resize the buffer so
>> that they can try again, so they need to know the correct size.
>>
>> Can you make it report that it overflowed, and print the correct size?
> The correct size can only be printed in the end of the tracing.
> Maybe we can use WARN_ONCE when it first occurs and later when the user
> type "iotrace stats" it will state the required buffer size to
> accommodate all of the
> entries.
> What do you think ?

Sounds good to me.

Regards,
Simon

[U-Boot] [PATCH v3 0/6] Iotrace improvements

[Ramon Fried]

These set of patches add few improvements to iotrace.
* Region limiting - allows setting an address and size where only
					io operations that falls into that address are
					logged.
* Timestamping - Timestamp every iotrace record with current timestamp
* dumping - iotrace dump command for dumping all records from buffer
			in a readable fashion.

In terms of backwards compatibility, the timestamp is not backward
compatible as it changes the iotrace record. so if one developed an
offline parsing tool it will be broken.
I though of adding #ifdef specific for that, but eventually I didn't.

v2:
	* fixed printf format
	* added a fix when the buffer is full
v3:
	* changed the "buffer full" scenario as
	recommended by Simon Glass.
	This fix  is dependand on a sperate patch that introduces
	WARN_ONCE definition in bug.h (https://patchwork.ozlabs.org/patch/921391/)

Ramon Fried (6):
  cmd: iotrace: add set region command
  iotrace: add IO region limit
  common: iotrace: add timestamp to iotrace records
  iotrace: move record definitons to header file
  cmd: iotrace: add dump trace command
  iotrace: fix behaviour when buffer is full

 cmd/iotrace.c     | 63 ++++++++++++++++++++++++++++++++++++++++++---
 common/iotrace.c  | 65 +++++++++++++++++++++++++++--------------------
 include/iotrace.h | 57 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 153 insertions(+), 32 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[U-Boot] [PATCH 4/6] iotrace: move record definitons to header file

[Ramon Fried]

The header definitions are needed for reading
record information in cmd/iotrace.c

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/iotrace.c  | 27 ---------------------------
 include/iotrace.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/common/iotrace.c b/common/iotrace.c
index 2f03a6082e8..83691b1dba9 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -11,33 +11,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/* Support up to the machine word length for now */
-typedef ulong iovalue_t;
-
-enum iotrace_flags {
-	IOT_8 = 0,
-	IOT_16,
-	IOT_32,
-
-	IOT_READ = 0 << 3,
-	IOT_WRITE = 1 << 3,
-};
-
-/**
- * struct iotrace_record - Holds a single I/O trace record
- *
- * @flags: I/O access type
- * @timestamp: Timestamp of access
- * @addr: Address of access
- * @value: Value written or read
- */
-struct iotrace_record {
-	enum iotrace_flags flags;
-	u64 timestamp;
-	phys_addr_t addr;
-	iovalue_t value;
-};
-
 /**
  * struct iotrace - current trace status and checksum
  *
diff --git a/include/iotrace.h b/include/iotrace.h
index 1efb117343a..b6e006ced06 100644
--- a/include/iotrace.h
+++ b/include/iotrace.h
@@ -7,6 +7,34 @@
 #define __IOTRACE_H
 
 #include <linux/types.h>
+#include <common.h>
+
+/* Support up to the machine word length for now */
+typedef ulong iovalue_t;
+
+enum iotrace_flags {
+	IOT_8 = 0,
+	IOT_16,
+	IOT_32,
+
+	IOT_READ = 0 << 3,
+	IOT_WRITE = 1 << 3,
+};
+
+/**
+ * struct iotrace_record - Holds a single I/O trace record
+ *
+ * @flags: I/O access type
+ * @timestamp: Timestamp of access
+ * @addr: Address of access
+ * @value: Value written or read
+ */
+struct iotrace_record {
+	enum iotrace_flags flags;
+	u64 timestamp;
+	phys_addr_t addr;
+	iovalue_t value;
+};
 
 /*
  * This file is designed to be included in arch/<arch>/include/asm/io.h.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project