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

[PATCH v1 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.

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 | 10 +++++++---
 3 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v1 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>
---
 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

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

[Jithu Joseph]

On 3/1/2026 7:26 PM, Jamin Lin wrote:
> 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>
> ---
>  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

Looks good to me

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



Thanks
Jithu

[PATCH v1 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 = to_write;

so that the actual number of transmitted bytes is correctly
propagated back to i3c_send() and reflected in trace output.

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

diff --git a/hw/i3c/mock-i3c-target.c b/hw/i3c/mock-i3c-target.c
index 875cd7c7d0..5c913ee49b 100644
--- a/hw/i3c/mock-i3c-target.c
+++ b/hw/i3c/mock-i3c-target.c
@@ -86,6 +86,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 v1 2/4] hw/i3c/mock-i3c-target: Set num_sent in TX callback to fix trace reporting

[Jithu Joseph]

On 3/1/2026 7:26 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 = to_write;
> 
> so that the actual number of transmitted bytes is correctly
> propagated back to i3c_send() and reflected in trace output.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  hw/i3c/mock-i3c-target.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i3c/mock-i3c-target.c b/hw/i3c/mock-i3c-target.c
> index 875cd7c7d0..5c913ee49b 100644
> --- a/hw/i3c/mock-i3c-target.c
> +++ b/hw/i3c/mock-i3c-target.c
> @@ -86,6 +86,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;

Thanks for fixing this... could you also set *num_sent to 1 in the ibi return path earlier in the function (snippet below), where it seems to consume exactly 1 byte of data
...

<snip>
    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;
    }


</snip>

>      return ret;
>  }
>  

Jithu

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

[Jamin Lin]

Hi Jithu Joseph

> Subject: Re: [PATCH v1 2/4] hw/i3c/mock-i3c-target: Set num_sent in TX
> callback to fix trace reporting
> 
> On 3/1/2026 7:26 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 = to_write;
> >
> > so that the actual number of transmitted bytes is correctly propagated
> > back to i3c_send() and reflected in trace output.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >  hw/i3c/mock-i3c-target.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/i3c/mock-i3c-target.c b/hw/i3c/mock-i3c-target.c index
> > 875cd7c7d0..5c913ee49b 100644
> > --- a/hw/i3c/mock-i3c-target.c
> > +++ b/hw/i3c/mock-i3c-target.c
> > @@ -86,6 +86,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;
> 
> Thanks for fixing this... could you also set *num_sent to 1 in the ibi return path
> earlier in the function (snippet below), where it seems to consume exactly 1
> byte of data ...
> 
> <snip>
>     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;
>     }
> 
> 
> </snip>
>

Thanks for the review and suggestion.
Will resend in v2.

Thanks,
Jamin

> >      return ret;
> >  }
> >
> 
> Jithu

[PATCH v1 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>
---
 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

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

[Jithu Joseph]

On 3/1/2026 7:26 PM, Jamin Lin wrote:
> 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>
> ---
>  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);
>  }
>  

Looks good to me

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


Thanks
Jithu

[PATCH v1 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>
---
 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 5c913ee49b..845c173427 100644
--- a/hw/i3c/mock-i3c-target.c
+++ b/hw/i3c/mock-i3c-target.c
@@ -145,9 +145,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

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

[Jithu Joseph]

On 3/1/2026 7:26 PM, Jamin Lin wrote:
> 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>
> ---
>  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 5c913ee49b..845c173427 100644
> --- a/hw/i3c/mock-i3c-target.c
> +++ b/hw/i3c/mock-i3c-target.c
> @@ -145,9 +145,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;
>          }

Looks way more readable

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


Thanks
Jithu