Change in coreboot[master]: nb/intel/ironlake: Avoid casting pointers to structs

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

nb/intel/ironlake: Avoid casting pointers to structs

Instead, convert the struct to a union and pass in a pointer to it.

Without the initialization of `reply.command`, Packard Bell MS2290 does
not change. This might be due to incorrect compiler optimizations.

Change-Id: I60e3dca7ad101d840759bdc0c88c50d9f07d65e2
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/ironlake/raminit.c
1 file changed, 16 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/45367/1

diff --git a/src/northbridge/intel/ironlake/raminit.c b/src/northbridge/intel/ironlake/raminit.c
index aa73d99..9ee2253 100644
--- a/src/northbridge/intel/ironlake/raminit.c
+++ b/src/northbridge/intel/ironlake/raminit.c
@@ -1724,8 +1724,20 @@
    return 0;
 }

+union uma_reply {
+	struct {
+		u8 group_id;
+		u8 command;
+		u8 reserved;
+		u8 result;
+		u8 field2;
+		u8 unk3[0x48 - 4 - 1];
+	};
+	u32 dwords[0x48 / sizeof(u32)];
+} __packed;
+
 /* FIXME: Add timeout.  */
-static int recv_heci_message(u32 *message, u32 *message_size)
+static int recv_heci_message(union uma_reply *message, u32 *message_size)
 {
    struct mei_header head;
    int current_position;
@@ -1735,7 +1747,7 @@
    	u32 current_size;
    	current_size = *message_size - current_position;
    	if (recv_heci_packet
-		    (&head, message + (current_position >> 2),
+		    (&head, message->dwords + (current_position >> 2),
    	     &current_size) == -1)
    		break;
    	if (!current_size)
@@ -1755,14 +1767,7 @@

static void send_heci_uma_message(const u64 heci_uma_addr, const unsigned int heci_uma_size)
 {
-	struct uma_reply {
-		u8 group_id;
-		u8 command;
-		u8 reserved;
-		u8 result;
-		u8 field2;
-		u8 unk3[0x48 - 4 - 1];
-	} __packed reply;
+	union uma_reply reply;

/* FIXME: recv_heci_message() does not always initialize 'reply' */
    reply.command = 0;
@@ -1791,7 +1796,7 @@
    send_heci_message((u8 *) &msg, sizeof(msg), 0, 7);

reply_size = sizeof(reply);
-	if (recv_heci_message((u32 *) &reply, &reply_size) == -1)
+	if (recv_heci_message(&reply, &reply_size) == -1)
    	return;

if (reply.command != (MKHI_SET_UMA | (1 << 7)))

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
File src/northbridge/intel/ironlake/raminit.c:

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
PS2, Line 1750: message->dwords + (current_position >> 2),
just a thought, the compiler might be able to "see" it more clearly if array indexing is used (message->dwords[current_position>>2]).

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
File src/northbridge/intel/ironlake/raminit.c:

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
PS2, Line 1750: message->dwords + (current_position >> 2),
> just a thought, the compiler might be able to "see" it more clearly if array indexing is used (messa […]

It does see something more clearly, and the binary changes. I'd like to have this tested on real hardware just to make sure it works.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
File src/northbridge/intel/ironlake/raminit.c:

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
PS2, Line 1750: message->dwords + (current_position >> 2),
> It does see something more clearly, and the binary changes. […]

Did you check the assembly to see what changed?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
File src/northbridge/intel/ironlake/raminit.c:

https://review.coreboot.org/c/coreboot/+/45367/2/src/northbridge/intel/ironl... 
PS2, Line 1750: message->dwords + (current_position >> 2),
> Did you check the assembly to see what changed?

No, I didn't. In any case, I'd like to boot-test this to be sure it works as intended. This change can wait.

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Tim Wawrzynczak, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45367

to look at the new patch set (#3).

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

nb/intel/ironlake: Avoid casting pointers to structs

Instead, convert the struct to a union and pass in a pointer to it.

Tested on out-of-tree HP ProBook 6550b, still boots.

Change-Id: I60e3dca7ad101d840759bdc0c88c50d9f07d65e2
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/ironlake/raminit.c
1 file changed, 16 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/45367/3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 2:

(1 comment)

File src/northbridge/intel/ironlake/raminit.c:

https://review.coreboot.org/c/coreboot/+/45367/comment/e1bc924c_68213107 
PS2, Line 1750: message->dwords + (current_position >> 2),
> No, I didn't. In any case, I'd like to boot-test this to be sure it works as intended. […]

Done

[Tim Wawrzynczak (Code Review)]

Attention is currently required from: Angel Pons.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45367 )

Change subject: nb/intel/ironlake: Avoid casting pointers to structs
......................................................................

nb/intel/ironlake: Avoid casting pointers to structs

Instead, convert the struct to a union and pass in a pointer to it.

Tested on out-of-tree HP ProBook 6550b, still boots.

Change-Id: I60e3dca7ad101d840759bdc0c88c50d9f07d65e2
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45367
Reviewed-by: Tim Wawrzynczak 
Tested-by: build bot (Jenkins) 
---
M src/northbridge/intel/ironlake/raminit.c
1 file changed, 16 insertions(+), 14 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/northbridge/intel/ironlake/raminit.c b/src/northbridge/intel/ironlake/raminit.c
index a5de86a..f073dda 100644
--- a/src/northbridge/intel/ironlake/raminit.c
+++ b/src/northbridge/intel/ironlake/raminit.c
@@ -1649,8 +1649,20 @@
    return 0;
 }

+union uma_reply {
+	struct {
+		u8 group_id;
+		u8 command;
+		u8 reserved;
+		u8 result;
+		u8 field2;
+		u8 unk3[0x48 - 4 - 1];
+	};
+	u32 dwords[0x48 / sizeof(u32)];
+} __packed;
+
 /* FIXME: Add timeout.  */
-static int recv_heci_message(u32 *message, u32 *message_size)
+static int recv_heci_message(union uma_reply *message, u32 *message_size)
 {
    struct mei_header head;
    int current_position;
@@ -1660,7 +1672,7 @@
    	u32 current_size;
    	current_size = *message_size - current_position;
    	if (recv_heci_packet
-		    (&head, message + (current_position >> 2),
+		    (&head, &message->dwords[current_position / sizeof(u32)],
    	     &current_size) == -1)
    		break;
    	if (!current_size)
@@ -1680,17 +1692,7 @@

static void send_heci_uma_message(const u64 heci_uma_addr, const unsigned int heci_uma_size)
 {
-	volatile struct uma_reply {
-		u8 group_id;
-		u8 command;
-		u8 reserved;
-		u8 result;
-		u8 field2;
-		u8 unk3[0x48 - 4 - 1];
-	} __packed reply;
-
-	/* FIXME: recv_heci_message() does not always initialize 'reply' */
-	reply.command = 0;
+	union uma_reply reply;

struct uma_message {
    	u8 group_id;
@@ -1716,7 +1718,7 @@
    send_heci_message((u8 *) &msg, sizeof(msg), 0, 7);

reply_size = sizeof(reply);
-	if (recv_heci_message((u32 *) &reply, &reply_size) == -1)
+	if (recv_heci_message(&reply, &reply_size) == -1)
    	return;

if (reply.command != (MKHI_SET_UMA | (1 << 7)))