[PATCH v2 0/4] hw/i3c: Trace fixes and readability improvements from review feedback

[PATCH v2 0/4] hw/i3c: Trace fixes and readability improvements from review feedback

[Jamin Lin]

This small series applies a few follow-up cleanups and bug fixes to the I3C
subsystem based on recent review feedback.

The series is based on the aspeed-next branch from:
  https://github.com/legoater/qemu.git

v1:
 1. Improve trace correctness/determinism by ensuring num_sent is always
  initialized and reported properly.
 2. Replace a couple of less readable expressions with clearer helpers/logic,
  without changing behavior.

v2:
  1. Fix review issue.

Jamin Lin (4):
  hw/i3c/dw-i3c: Use ROUND_UP() for RX buffer allocation alignment
  hw/i3c/mock-i3c-target: Set num_sent in TX callback to fix trace
    reporting
  hw/i3c/core: Initialize num_sent in i3c_send_byte()
  hw/i3c/mock-i3c-target: Simplify GETMRL byte extraction logic

 hw/i3c/core.c            |  2 +-
 hw/i3c/dw-i3c.c          |  2 +-
 hw/i3c/mock-i3c-target.c | 11 ++++++++---
 3 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] hw/i3c/dw-i3c: Use ROUND_UP() for RX buffer allocation alignment

[Jamin Lin]

The RX temporary buffer allocation manually aligned the size using:

    num + (4 - (num & 0x03))

Replace this with ROUND_UP(num, 4) for better readability and
consistency with common QEMU coding style.

No functional change.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Jithu Joseph <jithu.joseph@oss.qualcomm.com>
---
 hw/i3c/dw-i3c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i3c/dw-i3c.c b/hw/i3c/dw-i3c.c
index d742458129..3d8b95a14c 100644
--- a/hw/i3c/dw-i3c.c
+++ b/hw/i3c/dw-i3c.c
@@ -1282,7 +1282,7 @@ static uint16_t dw_i3c_rx(DWI3C *s, uint16_t num, bool is_i2c)
      * Allocate a temporary buffer to read data from the target.
      * Zero it and word-align it as well in case we're reading unaligned data.
      */
-    g_autofree uint8_t *data = g_new0(uint8_t, num + (4 - (num & 0x03)));
+    g_autofree uint8_t *data = g_new0(uint8_t, ROUND_UP(num, 4));
     uint32_t *data32 = (uint32_t *)data;
     /*
      * 32-bits since the I3C API wants a 32-bit number, even though the
-- 
2.43.0

[PATCH v2 2/4] hw/i3c/mock-i3c-target: Set num_sent in TX callback to fix trace reporting

[Jamin Lin]

mock_i3c_target_tx() did not update *num_sent before returning.

Although some callers may not directly use this value, i3c_send()
passes num_sent to trace_i3c_send(). If the target TX callback does
not initialize *num_sent, the trace output may report an incorrect
or uninitialized value, leading to confusing debugging information.

For example, the following trace was observed:

  mock_i3c_target_tx I3C mock target write 0x12
  i3c_send I3C send 0/1 bytes, ack=1    (expected 1/1 bytes)

This happens because *num_sent was never set by the TX callback.

Fix this by setting *num_sent in all return paths,
including the IBI magic handling case, to accurately reflect
the number of bytes consumed.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i3c/mock-i3c-target.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i3c/mock-i3c-target.c b/hw/i3c/mock-i3c-target.c
index 875cd7c7d0..b99709a08b 100644
--- a/hw/i3c/mock-i3c-target.c
+++ b/hw/i3c/mock-i3c-target.c
@@ -70,6 +70,7 @@ static int mock_i3c_target_tx(I3CTarget *i3c, const uint8_t *data,
 
     if (s->cfg.ibi_magic && num_to_send == 1 && s->cfg.ibi_magic == *data) {
         mock_i3c_target_ibi_timer_start(s);
+        *num_sent = 1;
         return 0;
     }
 
@@ -86,6 +87,7 @@ static int mock_i3c_target_tx(I3CTarget *i3c, const uint8_t *data,
         s->buf[s->p_buf] = data[i];
         s->p_buf++;
     }
+    *num_sent = to_write;
     return ret;
 }
 
-- 
2.43.0

Re: [PATCH v2 2/4] hw/i3c/mock-i3c-target: Set num_sent in TX callback to fix trace reporting

[Jithu Joseph]

On 3/2/2026 5:33 PM, Jamin Lin wrote:
> mock_i3c_target_tx() did not update *num_sent before returning.
> 
> Although some callers may not directly use this value, i3c_send()
> passes num_sent to trace_i3c_send(). If the target TX callback does
> not initialize *num_sent, the trace output may report an incorrect
> or uninitialized value, leading to confusing debugging information.
> 
> For example, the following trace was observed:
> 
>   mock_i3c_target_tx I3C mock target write 0x12
>   i3c_send I3C send 0/1 bytes, ack=1    (expected 1/1 bytes)
> 
> This happens because *num_sent was never set by the TX callback.
> 
> Fix this by setting *num_sent in all return paths,
> including the IBI magic handling case, to accurately reflect
> the number of bytes consumed.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  hw/i3c/mock-i3c-target.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/i3c/mock-i3c-target.c b/hw/i3c/mock-i3c-target.c
> index 875cd7c7d0..b99709a08b 100644
> --- a/hw/i3c/mock-i3c-target.c
> +++ b/hw/i3c/mock-i3c-target.c
> @@ -70,6 +70,7 @@ static int mock_i3c_target_tx(I3CTarget *i3c, const uint8_t *data,
>  
>      if (s->cfg.ibi_magic && num_to_send == 1 && s->cfg.ibi_magic == *data) {
>          mock_i3c_target_ibi_timer_start(s);
> +        *num_sent = 1;
>          return 0;
>      }
>  
> @@ -86,6 +87,7 @@ static int mock_i3c_target_tx(I3CTarget *i3c, const uint8_t *data,
>          s->buf[s->p_buf] = data[i];
>          s->p_buf++;
>      }
> +    *num_sent = to_write;
>      return ret;
>  }
>  

Looks good to me, Thanks for the quick re-spin

Reviewed-by: Jithu Joseph <jithu.joseph@oss.qualcomm.com>


Thanks
Jithu

[PATCH v2 3/4] hw/i3c/core: Initialize num_sent in i3c_send_byte()

[Jamin Lin]

i3c_send_byte() declared num_sent without initializing it before
passing its address to i3c_send().

Although i3c_send_byte() itself ignores num_sent after the call,
i3c_send() forwards it to trace_i3c_send(). If the target send
callback does not set *num_sent, the trace may print an
uninitialized value, leading to misleading or garbage output.

Example concern from review:
  trace_i3c_send(*num_sent, num_to_send, ret == 0);

If *num_sent is not written by the callback, this trace can report
an incorrect number of transmitted bytes.

Initialize num_sent to 0 to ensure deterministic and predictable
trace output, even if the callback fails to update it.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Jithu Joseph <jithu.joseph@oss.qualcomm.com>
---
 hw/i3c/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i3c/core.c b/hw/i3c/core.c
index 0a266e00a2..168526003d 100644
--- a/hw/i3c/core.c
+++ b/hw/i3c/core.c
@@ -325,7 +325,7 @@ int i3c_send_byte(I3CBus *bus, uint8_t data)
      * Ignored, the caller can determine how many were sent based on if this was
      * ACKed/NACKed.
      */
-    uint32_t num_sent;
+    uint32_t num_sent = 0;
     return i3c_send(bus, &data, 1, &num_sent);
 }
 
-- 
2.43.0

[PATCH v2 4/4] hw/i3c/mock-i3c-target: Simplify GETMRL byte extraction logic

[Jamin Lin]

The GETMRL handling logic extracted MSB/LSB bytes from
s->cfg.buf_size using a mask-and-shift expression:

  (buf_size & (0xff00 >> (offset * 8))) >>
  (8 - (offset * 8))

While functionally correct, the expression is difficult to read
and obscures the intent, which is simply to return a 16-bit value
in MSB-first order.

Replace the mask/shift formula with explicit MSB/LSB extraction:

  offset == 0 -> buf_size >> 8
  offset == 1 -> buf_size & 0xff

This makes the code clearer and easier to review without altering
behavior or data ordering.

No functional change.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Jithu Joseph <jithu.joseph@oss.qualcomm.com>
---
 hw/i3c/mock-i3c-target.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/i3c/mock-i3c-target.c b/hw/i3c/mock-i3c-target.c
index b99709a08b..1ae2cf9e1d 100644
--- a/hw/i3c/mock-i3c-target.c
+++ b/hw/i3c/mock-i3c-target.c
@@ -146,9 +146,12 @@ static int mock_i3c_target_handle_ccc_read(I3CTarget *i3c, uint8_t *data,
             if (s->ccc_byte_offset >= 2) {
                 break;
             }
-            data[s->ccc_byte_offset] = (s->cfg.buf_size &
-                                        (0xff00 >> (s->ccc_byte_offset * 8))) >>
-                                        (8 - (s->ccc_byte_offset * 8));
+            if (s->ccc_byte_offset == 0) {
+                data[s->ccc_byte_offset] = (uint8_t)(s->cfg.buf_size >> 8);
+            } else {
+                data[s->ccc_byte_offset] = (uint8_t)s->cfg.buf_size;
+            }
+
             s->ccc_byte_offset++;
             *num_read = num_to_read;
         }
-- 
2.43.0