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?