Change in coreboot[master]: drivers/ipmi: Handle the condition when (dev->chip_info == NULL)
Bill XIE has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
drivers/ipmi: Handle the condition when (dev->chip_info == NULL)
Some former commits (e.g. Ieb41771c75aae902191bba5d220796e6c343f8e0) blindly assume that dev->chip_info is capable to be dereferenced, making at least compilers complain about potential null pointer dereference. They might cause crash if truly (dev->chip_info == NULL).
Their code should be adjusted to be runnable even if dev->chip_info is NULL.
Signed-off-by: Bill XIE persmule@hardenedlinux.org Change-Id: I1d694b12f6c42961c104fe839d4ee46c0f111197 --- M src/drivers/ipmi/ipmi_kcs_ops.c 1 file changed, 14 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/47387/1
diff --git a/src/drivers/ipmi/ipmi_kcs_ops.c b/src/drivers/ipmi/ipmi_kcs_ops.c index 362f17a..3af44ef 100644 --- a/src/drivers/ipmi/ipmi_kcs_ops.c +++ b/src/drivers/ipmi/ipmi_kcs_ops.c @@ -77,8 +77,9 @@ struct ipmi_devid_rsp rsp; uint32_t man_id = 0, prod_id = 0; struct drivers_ipmi_config *conf = NULL; - struct ipmi_selftest_rsp selftestrsp; + struct ipmi_selftest_rsp selftestrsp = {{ 0, 0, 0 }, 0, 0}; uint8_t retry_count; + uint16_t bmc_boot_timeout = 0;
if (!dev->enabled) return; @@ -90,8 +91,9 @@
/* Get IPMI version for ACPI and SMBIOS */ if (conf && conf->wait_for_bmc && conf->bmc_boot_timeout) { + bmc_boot_timeout = conf->bmc_boot_timeout; struct stopwatch sw; - stopwatch_init_msecs_expire(&sw, conf->bmc_boot_timeout * 1000); + stopwatch_init_msecs_expire(&sw, bmc_boot_timeout * 1000); printk(BIOS_INFO, "IPMI: Waiting for BMC...\n");
while (!stopwatch_expired(&sw)) { @@ -108,7 +110,7 @@ }
printk(BIOS_INFO, "Get BMC self test result..."); - for (retry_count = 0; retry_count < conf->bmc_boot_timeout; retry_count++) { + for (retry_count = 0; retry_count < bmc_boot_timeout; retry_count++) { if (!ipmi_get_bmc_self_test_result(dev, &selftestrsp)) break;
@@ -175,6 +177,7 @@ struct acpi_rsdp *rsdp) { struct drivers_ipmi_config *conf = NULL; + uint32_t uid = 0; struct acpi_spmi *spmi; s8 gpe_interrupt = -1; u32 apic_interrupt = 0; @@ -214,11 +217,12 @@ apic_interrupt = conf->apic_interrupt; }
- /* Use command to get UID from ipmi_ssdt */ + if (uid_cnt > 0) + uid = uid_cnt - 1; acpi_create_ipmi(dev, spmi, (ipmi_revision_major << 8) | (ipmi_revision_minor << 4), &addr, IPMI_INTERFACE_KCS, gpe_interrupt, apic_interrupt, - conf->uid); + uid);
acpi_add_table(rsdp, spmi);
@@ -241,15 +245,12 @@ if (dev->chip_info) conf = dev->chip_info;
- /* Use command to pass UID to ipmi_write_acpi_tables */ - conf->uid = uid_cnt++; - /* write SPMI device */ acpigen_write_scope(scope); acpigen_write_device("SPMI"); acpigen_write_name_string("_HID", "IPI0001"); acpigen_write_name_unicode("_STR", "IPMI_KCS"); - acpigen_write_name_byte("_UID", conf->uid); + acpigen_write_name_byte("_UID", uid_cnt); acpigen_write_STA(0xf); acpigen_write_name("_CRS"); acpigen_write_resourcetemplate_header(); @@ -261,8 +262,12 @@ // FIXME: is that correct? if (conf->have_apic) acpigen_write_irq(1 << conf->apic_interrupt); + + conf->uid = uid_cnt; }
+ uid_cnt++; + acpigen_write_resourcetemplate_footer();
acpigen_write_method("_IFT", 0);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
dev->chip_info should never be NULL under normal circumstances, though. Trying to work around the problem feels unnecessarily complex. I'd just throw an error and call it a day.
https://review.coreboot.org/c/coreboot/+/47387/1/src/drivers/ipmi/ipmi_kcs_o... File src/drivers/ipmi/ipmi_kcs_ops.c:
https://review.coreboot.org/c/coreboot/+/47387/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 80: {{ 0, 0, 0 }, 0, 0} Just use a { 0 } initializer.
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47387
to look at the new patch set (#2).
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
drivers/ipmi: Handle the condition when (dev->chip_info == NULL)
Some former commits (e.g. Ieb41771c75aae902191bba5d220796e6c343f8e0) blindly assume that dev->chip_info is capable to be dereferenced, making at least compilers complain about potential null pointer dereference. They might cause crash if truly (dev->chip_info == NULL).
Their code should be adjusted to be runnable even if dev->chip_info is NULL.
Signed-off-by: Bill XIE persmule@hardenedlinux.org Change-Id: I1d694b12f6c42961c104fe839d4ee46c0f111197 --- M src/drivers/ipmi/ipmi_kcs_ops.c 1 file changed, 28 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/47387/2
Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 2:
(2 comments)
Patch Set 1: Code-Review+1
(1 comment)
dev->chip_info should never be NULL under normal circumstances, though. Trying to work around the problem feels unnecessarily complex. I'd just throw an error and call it a day.
We all know that, but the compiler do not, and (dev->chip_info == NULL) may not be fatal, but anyway, I could also print an error when dev->chip_info is really NULL.
https://review.coreboot.org/c/coreboot/+/47387/1/src/drivers/ipmi/ipmi_kcs_o... File src/drivers/ipmi/ipmi_kcs_ops.c:
https://review.coreboot.org/c/coreboot/+/47387/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 79: str Besides,
struct drivers_ipmi_config *conf = NULL; ... if (dev->chip_info) conf = dev->chip_info;
should be replaced with
struct drivers_ipmi_config *conf = dev->chip_info;
since they have nearly identical effect, but the latter is obviously more elegant.
https://review.coreboot.org/c/coreboot/+/47387/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 80: {{ 0, 0, 0 }, 0, 0}
Just use a { 0 } initializer.
Done
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47387
to look at the new patch set (#4).
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
drivers/ipmi: Handle the condition when (dev->chip_info == NULL)
Some former commits (e.g. Ieb41771c75aae902191bba5d220796e6c343f8e0) blindly assume that dev->chip_info is capable to be dereferenced, making at least compilers complain about potential null pointer dereference. They might cause crash if truly (dev->chip_info == NULL).
Signed-off-by: Bill XIE persmule@hardenedlinux.org Change-Id: I1d694b12f6c42961c104fe839d4ee46c0f111197 --- M src/drivers/ipmi/ipmi_kcs_ops.c 1 file changed, 29 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/47387/4
Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 4:
I'd just throw an error and call it a day.
How should we "throw an error" in coreboot? Use printk() like this revision? Or use config_of() to "get conf or devtree_die()"?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 4:
Patch Set 4:
I'd just throw an error and call it a day.
How should we "throw an error" in coreboot? Use printk() like this revision? Or use config_of() to "get conf or devtree_die()"?
config_of is only useful when you can't successfully boot if chip_info happens to be null.
It's been so long since I last looked at this change that my views have changed. For the BMC, I'd just skip initialization and not even bother printing any error message (if anything, just print one error or make them debug messages).
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 4: Code-Review+1
Hello build bot (Jenkins), Frans Hendriks, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47387
to look at the new patch set (#5).
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
drivers/ipmi: Handle the condition when (dev->chip_info == NULL)
Some former commits (e.g. Ieb41771c75aae902191bba5d220796e6c343f8e0) blindly assume that dev->chip_info is capable to be dereferenced, making at least compilers complain about potential null pointer dereference. They might cause crash if truly (dev->chip_info == NULL).
Signed-off-by: Bill XIE persmule@hardenedlinux.org Change-Id: I1d694b12f6c42961c104fe839d4ee46c0f111197 --- M src/drivers/ipmi/ipmi_kcs_ops.c 1 file changed, 29 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/47387/5
Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 5:
For the BMC, I'd just skip initialization and not even bother printing any error message (if anything, just print one error or make them debug messages).
I believe BIOS_WARNING is more suitable.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 5: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
Patch Set 5: Code-Review+2
Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47387 )
Change subject: drivers/ipmi: Handle the condition when (dev->chip_info == NULL) ......................................................................
drivers/ipmi: Handle the condition when (dev->chip_info == NULL)
Some former commits (e.g. Ieb41771c75aae902191bba5d220796e6c343f8e0) blindly assume that dev->chip_info is capable to be dereferenced, making at least compilers complain about potential null pointer dereference. They might cause crash if truly (dev->chip_info == NULL).
Signed-off-by: Bill XIE persmule@hardenedlinux.org Change-Id: I1d694b12f6c42961c104fe839d4ee46c0f111197 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47387 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Michael Niewöhner foss@mniewoehner.de --- M src/drivers/ipmi/ipmi_kcs_ops.c 1 file changed, 29 insertions(+), 31 deletions(-)
Approvals: build bot (Jenkins): Verified Frans Hendriks: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/drivers/ipmi/ipmi_kcs_ops.c b/src/drivers/ipmi/ipmi_kcs_ops.c index 362f17a..640bfa1 100644 --- a/src/drivers/ipmi/ipmi_kcs_ops.c +++ b/src/drivers/ipmi/ipmi_kcs_ops.c @@ -76,8 +76,8 @@ { struct ipmi_devid_rsp rsp; uint32_t man_id = 0, prod_id = 0; - struct drivers_ipmi_config *conf = NULL; - struct ipmi_selftest_rsp selftestrsp; + struct drivers_ipmi_config *conf = dev->chip_info; + struct ipmi_selftest_rsp selftestrsp = {0}; uint8_t retry_count;
if (!dev->enabled) @@ -85,11 +85,13 @@
printk(BIOS_DEBUG, "IPMI: PNP KCS 0x%x\n", dev->path.pnp.port);
- if (dev->chip_info) - conf = dev->chip_info; + if (!conf) { + printk(BIOS_WARNING, "IPMI: chip_info is missing! Skip init.\n"); + return; + }
/* Get IPMI version for ACPI and SMBIOS */ - if (conf && conf->wait_for_bmc && conf->bmc_boot_timeout) { + if (conf->wait_for_bmc && conf->bmc_boot_timeout) { struct stopwatch sw; stopwatch_init_msecs_expire(&sw, conf->bmc_boot_timeout * 1000); printk(BIOS_INFO, "IPMI: Waiting for BMC...\n"); @@ -174,7 +176,7 @@ ipmi_write_acpi_tables(const struct device *dev, unsigned long current, struct acpi_rsdp *rsdp) { - struct drivers_ipmi_config *conf = NULL; + struct drivers_ipmi_config *conf = dev->chip_info; struct acpi_spmi *spmi; s8 gpe_interrupt = -1; u32 apic_interrupt = 0; @@ -204,33 +206,32 @@ printk(BIOS_DEBUG, "ACPI: * SPMI at %lx\n", current); spmi = (struct acpi_spmi *)current;
- if (dev->chip_info) - conf = dev->chip_info; - if (conf) { if (conf->have_gpe) gpe_interrupt = conf->gpe_interrupt; if (conf->have_apic) apic_interrupt = conf->apic_interrupt; + + /* Use command to get UID from ipmi_ssdt */ + acpi_create_ipmi(dev, spmi, (ipmi_revision_major << 8) | + (ipmi_revision_minor << 4), &addr, + IPMI_INTERFACE_KCS, gpe_interrupt, apic_interrupt, + conf->uid); + + acpi_add_table(rsdp, spmi); + + current += spmi->header.length; + } else { + printk(BIOS_WARNING, "IPMI: chip_info is missing!\n"); }
- /* Use command to get UID from ipmi_ssdt */ - acpi_create_ipmi(dev, spmi, (ipmi_revision_major << 8) | - (ipmi_revision_minor << 4), &addr, - IPMI_INTERFACE_KCS, gpe_interrupt, apic_interrupt, - conf->uid); - - acpi_add_table(rsdp, spmi); - - current += spmi->header.length; - return current; }
static void ipmi_ssdt(const struct device *dev) { const char *scope = acpi_device_scope(dev); - struct drivers_ipmi_config *conf = NULL; + struct drivers_ipmi_config *conf = dev->chip_info;
if (!scope) { printk(BIOS_ERR, "IPMI: Missing ACPI scope for %s\n", @@ -238,8 +239,10 @@ return; }
- if (dev->chip_info) - conf = dev->chip_info; + if (!conf) { + printk(BIOS_WARNING, "IPMI: chip_info is missing!\n"); + return; + }
/* Use command to pass UID to ipmi_write_acpi_tables */ conf->uid = uid_cnt++; @@ -257,11 +260,9 @@ acpigen_write_io16(dev->path.pnp.port + CONFIG_IPMI_KCS_REGISTER_SPACING, dev->path.pnp.port + CONFIG_IPMI_KCS_REGISTER_SPACING, 1, 1, 1);
- if (conf) { - // FIXME: is that correct? - if (conf->have_apic) - acpigen_write_irq(1 << conf->apic_interrupt); - } + // FIXME: is that correct? + if (conf->have_apic) + acpigen_write_irq(1 << conf->apic_interrupt);
acpigen_write_resourcetemplate_footer();
@@ -295,16 +296,13 @@ static int ipmi_smbios_data(struct device *dev, int *handle, unsigned long *current) { - struct drivers_ipmi_config *conf = NULL; + struct drivers_ipmi_config *conf = dev->chip_info; u8 nv_storage = 0xff; u8 i2c_address = 0; u8 register_spacing;
int len = 0;
- if (dev->chip_info) - conf = dev->chip_info; - if (conf) { if (conf->have_nv_storage) nv_storage = conf->nv_storage_device_address;
-
Angel Pons (Code Review) -
Bill XIE (Code Review)
-
Frans Hendriks (Code Review)
-
Michael Niewöhner (Code Review)