Change in ...coreboot[master]: drivers/ipmi: Fix multiple issues

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33488

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

drivers/ipmi: Fix multiple issues

* Set abort command define
* Set debug level to SPEW
* Support zero length data packet in ipmi_kcs_send_message
  That's required for commands like GET_DEVICE_ID, which have no
  additional data to send.
* Read reply even if given no receive buffer
* Prevent buffer overflow in read reply processing

Tested on Wedge100s.

Change-Id: Iefddd88a744c3b96751d3fe8c2951ca2115548ce
Signed-off-by: Patrick Rudolph 
---
M src/drivers/ipmi/ipmi_kcs.c
1 file changed, 40 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/33488/1

diff --git a/src/drivers/ipmi/ipmi_kcs.c b/src/drivers/ipmi/ipmi_kcs.c
index 8d106837..bde18d2 100644
--- a/src/drivers/ipmi/ipmi_kcs.c
+++ b/src/drivers/ipmi/ipmi_kcs.c
@@ -22,7 +22,7 @@

#define IPMI_KCS_STATE(_x)	((_x) >> 6)

-#define IPMI_KCS_GET_STATUS_ABORT
+#define IPMI_KCS_GET_STATUS_ABORT 0x60
 #define IPMI_KCS_START_WRITE 0x61
 #define IPMI_KCS_END_WRITE 0x62
 #define IPMI_KCS_READ_BYTE 0x68
@@ -43,7 +43,7 @@
 static unsigned char ipmi_kcs_status(int port)
 {
    unsigned char status = inb(IPMI_STAT(port));
-	printk(BIOS_DEBUG, "%s: 0x%02x\n", __func__, status);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, status);
    return status;
 }

@@ -77,7 +77,7 @@
 {
    unsigned char status;

-	printk(BIOS_DEBUG, "%s: %02x\n", __func__, byte);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, byte);

outb(byte, IPMI_DATA(port));

@@ -100,7 +100,7 @@
 {
    unsigned char status;

-	printk(BIOS_DEBUG, "%s: %02x\n", __func__, byte);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, byte);

if (wait_ibf_timeout(port))
    	return 1;
@@ -121,7 +121,7 @@

static int ipmi_kcs_send_cmd_byte(int port, const unsigned char byte)
 {
-	printk(BIOS_DEBUG, "%s: 0x%02x\n", __func__, byte);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, byte);

if (wait_ibf_timeout(port))
    	return 1;
@@ -154,27 +154,43 @@
    	return ret;
    }

-	if ((ret = ipmi_kcs_send_data_byte(port, cmd))) {
-		printk(BIOS_ERR, "IPMI CMD failed\n");
-		return ret;
+	if (!len) {
+		if ((ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE))) {
+			printk(BIOS_ERR, "IPMI END WRITE failed\n");
+			return ret;
+		}
+
+		if ((ret = ipmi_kcs_send_last_data_byte(port, cmd))) {
+			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
+			return ret;
+		}
+	} else {
+		if ((ret = ipmi_kcs_send_data_byte(port, cmd))) {
+			printk(BIOS_ERR, "IPMI CMD failed\n");
+			return ret;
+		}
    }

-	while (len-- > 1) {
+	while (len > 1) {
    	if ((ret = ipmi_kcs_send_data_byte(port, *msg++))) {
    		printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
    		return ret;
    	}
+		len--;
+	}
+
+	if (len) {
+		if ((ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE))) {
+			printk(BIOS_ERR, "IPMI END WRITE failed\n");
+			return ret;
+		}
+
+		if ((ret = ipmi_kcs_send_last_data_byte(port, *msg++))) {
+			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
+			return ret;
+		}
    }

-	if ((ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE))) {
-		printk(BIOS_ERR, "IPMI END WRITE failed\n");
-		return ret;
-	}
-
-	if ((ret = ipmi_kcs_send_last_data_byte(port, *msg++))) {
-		printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
-		return ret;
-	}
    return 0;
 }

@@ -182,9 +198,6 @@
 {
    int status, ret = 0;

-	if (!msg)
-		return 0;
-
    if (wait_ibf_timeout(port))
    	return 1;

@@ -195,15 +208,18 @@
    		return ret;

if (IPMI_KCS_STATE(status) != IPMI_KCS_STATE_READ) {
-			printk(BIOS_ERR, "%s: wrong state: 0x%02x\n", __func__, status);
+			printk(BIOS_ERR, "%s: wrong state: 0x%02x\n", __func__,
+			       status);
    		return -1;
    	}

if (wait_obf_timeout(port))
    		return -1;

-		*msg++ = inb(IPMI_DATA(port));
-		ret++;
+		if (msg && (ret < len)) {
+			*msg++ = inb(IPMI_DATA(port));
+			ret++;
+		}

if (wait_ibf_timeout(port))
    		return -1;

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33488 )

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/33488/1/src/drivers/ipmi/ipmi_kcs.c 
File src/drivers/ipmi/ipmi_kcs.c:

https://review.coreboot.org/#/c/33488/1/src/drivers/ipmi/ipmi_kcs.c@158 
PS1, Line 158: 		if ((ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE))) {
do not use assignment in if condition

https://review.coreboot.org/#/c/33488/1/src/drivers/ipmi/ipmi_kcs.c@163 
PS1, Line 163: 		if ((ret = ipmi_kcs_send_last_data_byte(port, cmd))) {
do not use assignment in if condition

https://review.coreboot.org/#/c/33488/1/src/drivers/ipmi/ipmi_kcs.c@168 
PS1, Line 168: 		if ((ret = ipmi_kcs_send_data_byte(port, cmd))) {
do not use assignment in if condition

https://review.coreboot.org/#/c/33488/1/src/drivers/ipmi/ipmi_kcs.c@183 
PS1, Line 183: 		if ((ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE))) {
do not use assignment in if condition

https://review.coreboot.org/#/c/33488/1/src/drivers/ipmi/ipmi_kcs.c@188 
PS1, Line 188: 		if ((ret = ipmi_kcs_send_last_data_byte(port, *msg++))) {
do not use assignment in if condition

[Patrick Rudolph (Code Review)]

Hello Felix Held, Christian Walter, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

drivers/ipmi: Fix multiple issues

* Set abort command define
* Set debug level to SPEW
* Support zero length data packet in ipmi_kcs_send_message
  That's required for commands like GET_DEVICE_ID, which have no
  additional data to send.
* Read reply even if given no receive buffer
* Prevent buffer overflow in read reply processing

Tested on Wedge100s.

Change-Id: Iefddd88a744c3b96751d3fe8c2951ca2115548ce
Signed-off-by: Patrick Rudolph 
---
M src/drivers/ipmi/ipmi_kcs.c
1 file changed, 45 insertions(+), 27 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/33488/2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33488 )

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

Patch Set 2:

(4 comments)

the zero length packet send code seems to be a bit more convoluted than necessary to me

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c 
File src/drivers/ipmi/ipmi_kcs.c:

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@160 
PS2, Line 160: ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI END WRITE failed\n");
             : 			return ret;
             : 		}
             : 
             : 		ret = ipmi_kcs_send_last_data_byte(port, cmd);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
             : 			return ret;
             : 		}
this is basically a copy of the code beginning at line 189

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@172 
PS2, Line 172: ret = ipmi_kcs_send_data_byte(port, cmd);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI CMD failed\n");
             : 			return ret;
             : 		}
             : 	}
             : 
             : 	while (len > 1) {
             : 		ret = ipmi_kcs_send_data_byte(port, *msg++);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
             : 			return ret;
             : 		}
             : 		len--;
             : 	}
i'd just put this in a if(len) block

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@188 
PS2, Line 188: if (len) {
i'd remove the if(len) around this block; see my comment above

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@195 
PS2, Line 195: ++
the post-increment here isn't necessary, right?

[Patrick Rudolph (Code Review)]

Hello Felix Held, Sven Schnelle, Christian Walter, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

drivers/ipmi: Fix multiple issues

* Set abort command define
* Set debug level to SPEW
* Support zero length data packet in ipmi_kcs_send_message
  That's required for commands like GET_DEVICE_ID, which have no
  additional data to send.
* Read reply even if given no receive buffer
* Prevent buffer overflow in read reply processing

Tested on Wedge100s.

Change-Id: Iefddd88a744c3b96751d3fe8c2951ca2115548ce
Signed-off-by: Patrick Rudolph 
---
M src/drivers/ipmi/ipmi_kcs.c
1 file changed, 45 insertions(+), 29 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/33488/3

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33488 )

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c 
File src/drivers/ipmi/ipmi_kcs.c:

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@160 
PS2, Line 160: ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI END WRITE failed\n");
             : 			return ret;
             : 		}
             : 
             : 		ret = ipmi_kcs_send_last_data_byte(port, cmd);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
             : 			return ret;
             : 		}
> this is basically a copy of the code beginning at line 189

no, it has cmd after IPMI_KCS_END_WRITE, while the other block has *msg

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@172 
PS2, Line 172: ret = ipmi_kcs_send_data_byte(port, cmd);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI CMD failed\n");
             : 			return ret;
             : 		}
             : 	}
             : 
             : 	while (len > 1) {
             : 		ret = ipmi_kcs_send_data_byte(port, *msg++);
             : 		if (ret) {
             : 			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
             : 			return ret;
             : 		}
             : 		len--;
             : 	}
> i'd just put this in a if(len) block

Done

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@188 
PS2, Line 188: if (len) {
> i'd remove the if(len) around this block; see my comment above

Done

https://review.coreboot.org/#/c/33488/2/src/drivers/ipmi/ipmi_kcs.c@195 
PS2, Line 195: ++
> the post-increment here isn't necessary, right?

Done

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33488 )

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

Patch Set 3: Code-Review+2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33488 )

Change subject: drivers/ipmi: Fix multiple issues
......................................................................

drivers/ipmi: Fix multiple issues

* Set abort command define
* Set debug level to SPEW
* Support zero length data packet in ipmi_kcs_send_message
  That's required for commands like GET_DEVICE_ID, which have no
  additional data to send.
* Read reply even if given no receive buffer
* Prevent buffer overflow in read reply processing

Tested on Wedge100s.

Change-Id: Iefddd88a744c3b96751d3fe8c2951ca2115548ce
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33488
Reviewed-by: Felix Held 
Tested-by: build bot (Jenkins) 
---
M src/drivers/ipmi/ipmi_kcs.c
1 file changed, 45 insertions(+), 29 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved

diff --git a/src/drivers/ipmi/ipmi_kcs.c b/src/drivers/ipmi/ipmi_kcs.c
index d17a1f9..397a800 100644
--- a/src/drivers/ipmi/ipmi_kcs.c
+++ b/src/drivers/ipmi/ipmi_kcs.c
@@ -22,7 +22,7 @@

#define IPMI_KCS_STATE(_x)	((_x) >> 6)

-#define IPMI_KCS_GET_STATUS_ABORT
+#define IPMI_KCS_GET_STATUS_ABORT 0x60
 #define IPMI_KCS_START_WRITE 0x61
 #define IPMI_KCS_END_WRITE 0x62
 #define IPMI_KCS_READ_BYTE 0x68
@@ -43,7 +43,7 @@
 static unsigned char ipmi_kcs_status(int port)
 {
    unsigned char status = inb(IPMI_STAT(port));
-	printk(BIOS_DEBUG, "%s: 0x%02x\n", __func__, status);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, status);
    return status;
 }

@@ -77,7 +77,7 @@
 {
    unsigned char status;

-	printk(BIOS_DEBUG, "%s: %02x\n", __func__, byte);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, byte);

outb(byte, IPMI_DATA(port));

@@ -100,7 +100,7 @@
 {
    unsigned char status;

-	printk(BIOS_DEBUG, "%s: %02x\n", __func__, byte);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, byte);

if (wait_ibf_timeout(port))
    	return 1;
@@ -121,7 +121,7 @@

static int ipmi_kcs_send_cmd_byte(int port, const unsigned char byte)
 {
-	printk(BIOS_DEBUG, "%s: 0x%02x\n", __func__, byte);
+	printk(BIOS_SPEW, "%s: 0x%02x\n", __func__, byte);

if (wait_ibf_timeout(port))
    	return 1;
@@ -156,31 +156,47 @@
    	return ret;
    }

-	ret = ipmi_kcs_send_data_byte(port, cmd);
-	if (ret) {
-		printk(BIOS_ERR, "IPMI CMD failed\n");
-		return ret;
-	}
+	if (!len) {
+		ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE);
+		if (ret) {
+			printk(BIOS_ERR, "IPMI END WRITE failed\n");
+			return ret;
+		}

-	while (len-- > 1) {
-		ret = ipmi_kcs_send_data_byte(port, *msg++);
+		ret = ipmi_kcs_send_last_data_byte(port, cmd);
+		if (ret) {
+			printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
+			return ret;
+		}
+	} else {
+		ret = ipmi_kcs_send_data_byte(port, cmd);
+		if (ret) {
+			printk(BIOS_ERR, "IPMI CMD failed\n");
+			return ret;
+		}
+
+		while (len > 1) {
+			ret = ipmi_kcs_send_data_byte(port, *msg++);
+			if (ret) {
+				printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
+				return ret;
+			}
+			len--;
+		}
+
+		ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE);
+		if (ret) {
+			printk(BIOS_ERR, "IPMI END WRITE failed\n");
+			return ret;
+		}
+
+		ret = ipmi_kcs_send_last_data_byte(port, *msg);
    	if (ret) {
    		printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
    		return ret;
    	}
    }

-	ret = ipmi_kcs_send_cmd_byte(port, IPMI_KCS_END_WRITE);
-	if (ret) {
-		printk(BIOS_ERR, "IPMI END WRITE failed\n");
-		return ret;
-	}
-
-	ret = ipmi_kcs_send_last_data_byte(port, *msg++);
-	if (ret) {
-		printk(BIOS_ERR, "IPMI BYTE WRITE failed\n");
-		return ret;
-	}
    return 0;
 }

@@ -188,9 +204,6 @@
 {
    int status, ret = 0;

-	if (!msg)
-		return 0;
-
    if (wait_ibf_timeout(port))
    	return 1;

@@ -201,15 +214,18 @@
    		return ret;

if (IPMI_KCS_STATE(status) != IPMI_KCS_STATE_READ) {
-			printk(BIOS_ERR, "%s: wrong state: 0x%02x\n", __func__, status);
+			printk(BIOS_ERR, "%s: wrong state: 0x%02x\n", __func__,
+			       status);
    		return -1;
    	}

if (wait_obf_timeout(port))
    		return -1;

-		*msg++ = inb(IPMI_DATA(port));
-		ret++;
+		if (msg && (ret < len)) {
+			*msg++ = inb(IPMI_DATA(port));
+			ret++;
+		}

if (wait_ibf_timeout(port))
    		return -1;