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