The target_info union in dw_i3c_addr_assign_cmd() declares pid, bcr,
and dcr as separate union members, causing them to all alias b[0]
rather than their correct positions in the ENTDAA response buffer.
This results in dw_i3c_update_char_table() being called with BCR and
DCR both read from b[0] instead of b[6] and b[7] respectively,
corrupting the device characteristics table on every ENTDAA operation.

Fixed this by replacing the broken members with a nested struct using
uint8_t fields, matching the I3C spec wire format: six bytes of PID
followed by BCR and DCR.

Signed-off-by: Ashish Anand <ashish.a6@samsung.com>
---

The nested struct approach avoids bitfields and __attribute__((packed)),
both of which introduce implementation-defined behavior for this layout.

This bug was discovered during end-to-end ENTDAA testing where BCR and
DCR values observed in the char table register did not match the target's
actual BCR and DCR values.

A follow-up patch adding a standalone DW I3C test machine and qtest
coverage for ENTDAA, private transfers, and IBI is planned.

 hw/i3c/dw-i3c.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/i3c/dw-i3c.c b/hw/i3c/dw-i3c.c
index d87d42be89..1c5e09ccbb 100644
--- a/hw/i3c/dw-i3c.c
+++ b/hw/i3c/dw-i3c.c
@@ -1507,9 +1507,11 @@ static void dw_i3c_addr_assign_cmd(DWI3C *s, DWI3CAddrAssignCmd cmd)
     for (i = 0; i < cmd.dev_count; i++) {
         uint8_t addr = dw_i3c_target_addr(s, cmd.dev_index + i);
         union {
-            uint64_t pid:48;
-            uint8_t bcr;
-            uint8_t dcr;
+            struct {
+                uint8_t pid[6];
+                uint8_t bcr;
+                uint8_t dcr;
+            };
             uint32_t w[2];
             uint8_t b[8];
         } target_info;
@@ -1544,9 +1546,12 @@ static void dw_i3c_addr_assign_cmd(DWI3C *s, DWI3CAddrAssignCmd cmd)
             err = DW_I3C_RESP_QUEUE_ERR_DAA_NACK;
             break;
         }
-        dw_i3c_update_char_table(s, cmd.dev_index + i,
-                                            target_info.pid, target_info.bcr,
-                                            target_info.dcr, addr);
+        uint64_t pid = 0;
+        for (int j = 0; j < 6; j++) {
+            pid |= (uint64_t)target_info.pid[j] << (j * 8);
+        }
+        dw_i3c_update_char_table(s, cmd.dev_index + i, pid, target_info.bcr,
+                                 target_info.dcr, addr);
 
         /* Push the PID, BCR, and DCR to the RX queue. */
         dw_i3c_push_rx(s, target_info.w[0]);
-- 
2.43.0

Hi Ashish,

On 28/4/26 12:25, Ashish Anand wrote:
> The target_info union in dw_i3c_addr_assign_cmd() declares pid, bcr,
> and dcr as separate union members, causing them to all alias b[0]
> rather than their correct positions in the ENTDAA response buffer.
> This results in dw_i3c_update_char_table() being called with BCR and
> DCR both read from b[0] instead of b[6] and b[7] respectively,
> corrupting the device characteristics table on every ENTDAA operation.
> 
> Fixed this by replacing the broken members with a nested struct using
> uint8_t fields, matching the I3C spec wire format: six bytes of PID
> followed by BCR and DCR.
> 
> Signed-off-by: Ashish Anand <ashish.a6@samsung.com>
> ---
> 
> The nested struct approach avoids bitfields and __attribute__((packed)),
> both of which introduce implementation-defined behavior for this layout.
> 
> This bug was discovered during end-to-end ENTDAA testing where BCR and
> DCR values observed in the char table register did not match the target's
> actual BCR and DCR values.
> 
> A follow-up patch adding a standalone DW I3C test machine and qtest
> coverage for ENTDAA, private transfers, and IBI is planned.
> 
>   hw/i3c/dw-i3c.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i3c/dw-i3c.c b/hw/i3c/dw-i3c.c
> index d87d42be89..1c5e09ccbb 100644
> --- a/hw/i3c/dw-i3c.c
> +++ b/hw/i3c/dw-i3c.c
> @@ -1507,9 +1507,11 @@ static void dw_i3c_addr_assign_cmd(DWI3C *s, DWI3CAddrAssignCmd cmd)
>       for (i = 0; i < cmd.dev_count; i++) {
>           uint8_t addr = dw_i3c_target_addr(s, cmd.dev_index + i);
>           union {

                uint64_t d;

> -            uint64_t pid:48;
> -            uint8_t bcr;
> -            uint8_t dcr;
> +            struct {
> +                uint8_t pid[6];
> +                uint8_t bcr;
> +                uint8_t dcr;
> +            };
>               uint32_t w[2];
>               uint8_t b[8];
>           } target_info;
> @@ -1544,9 +1546,12 @@ static void dw_i3c_addr_assign_cmd(DWI3C *s, DWI3CAddrAssignCmd cmd)
>               err = DW_I3C_RESP_QUEUE_ERR_DAA_NACK;
>               break;
>           }
> -        dw_i3c_update_char_table(s, cmd.dev_index + i,
> -                                            target_info.pid, target_info.bcr,
> -                                            target_info.dcr, addr);
> +        uint64_t pid = 0;
> +        for (int j = 0; j < 6; j++) {
> +            pid |= (uint64_t)target_info.pid[j] << (j * 8);

Directly use .b[] instead of .pid[]?

> +        }

Or (untested):

            dcr = target_info.b[0];
            bcr = target_info.b[1];
            pid = extract64(be64_to_cpu(target_info.d), 16, 48);

or:
            pid = be64_to_cpu(target_info.d) >> 16;

> +        dw_i3c_update_char_table(s, cmd.dev_index + i, pid, target_info.bcr,
> +                                 target_info.dcr, addr);
>   
>           /* Push the PID, BCR, and DCR to the RX queue. */
>           dw_i3c_push_rx(s, target_info.w[0]);

On 28/04/26 01:54PM, Philippe Mathieu-Daudé wrote:

Hi Philippe,
Thanks for the review.

>Directly use .b[] instead of .pid[]?
Will drop the anonymous struct and use .b[] directly in v2.

>Or (untested):
>
>           dcr = target_info.b[0];
>           bcr = target_info.b[1];

On BCR/DCR indices: using b[6] for BCR and b[7] for DCR per the ENTDAA frame diagram in the I3C v1.1.1 databook.


>           pid = extract64(be64_to_cpu(target_info.d), 16, 48);
>
>or:
>           pid = be64_to_cpu(target_info.d) >> 16;

On PID assembly and the be64_to_cpu suggestion:
The current loop (j * 8) was written assuming t->pid holds the logical
value (e.g. 0x0000AABBCCDDEEFF with AA as MSB). core.c sends using
(offset * 8) which puts the LSB on wire first, so the loop recovered the
original logical value end to end.

For spec compliance (pid[47:40] on wire first), given that core.c uses
(offset * 8), t->pid must be stored pre-reversed by the target modeller.
With that assumption be64_to_cpu >> 16 correctly recovers the logical pid
on the controller side on both LE and BE hosts. Will adopt this in v2 and add
a comment to i3c.h documenting this requirement for t->pid.

>
>>+        dw_i3c_update_char_table(s, cmd.dev_index + i, pid, target_info.bcr,
>>+                                 target_info.dcr, addr);

Also found a pre-existing bug in dw_i3c_update_char_table() - PID is
split at bit 32 but spec DCT layout (verified from databook) requires the
split at bit 16:
     LOC1 = pid[47:16]    (pid >> 16) & 0xffffffff
     LOC2 = pid[15:0]     pid & 0xffff
Will fold this fix into v2.

>

Will send v2 shortly.

Thanks,
Ashish