Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73456 )
Change subject: internal: Move laptop_ok into board_cfg ......................................................................
internal: Move laptop_ok into board_cfg
Due to how internal is structured around chipset_flash_enable() entry we need to prepare a crafted programmer_cfg that contains a board_enable substructure with data derived from the board_enable subsystem. While this is certainly not perfection, it does make clear the relationships between board_enable into chipset_flash_enable and subsequently the overall internal programmer initialisation in a RAII fashion at the type level over closure upon global state that is impossible to reason about.
Also flip predicate in report_nonwl_laptop_detected() and return early with the trivial base-case.
TEST=`$ sudo ./flashrom -p internal --flash-name`.
Change-Id: I459215253845c2af73262943ce91a36464e9eb06 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/73456 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Sam McNally sammc@google.com --- M board_enable.c M chipset_enable.c M flashrom.c M include/programmer.h M internal.c 5 files changed, 72 insertions(+), 44 deletions(-)
Approvals: build bot (Jenkins): Verified Sam McNally: Looks good to me, approved
diff --git a/board_enable.c b/board_enable.c index f95430a..d9af44e 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2293,7 +2293,7 @@ static int p2_whitelist_laptop(struct board_cfg *cfg) { cfg->is_laptop = 1; - g_laptop_ok = true; + cfg->laptop_ok = true; msg_pdbg("Whitelisted laptop detected.\n"); return 0; } diff --git a/chipset_enable.c b/chipset_enable.c index 37a48fc..16ef0f4 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -826,7 +826,7 @@
/* Suppress unknown laptop warning if we booted from SPI. */ if (boot_buses & BUS_SPI) - g_laptop_ok = true; + cfg->bcfg->laptop_ok = true;
return 0; } @@ -971,7 +971,7 @@
/* Suppress unknown laptop warning if we booted from SPI. */ if (!ret && (boot_buses & BUS_SPI)) - g_laptop_ok = true; + cfg->bcfg->laptop_ok = true;
_freepci_ret: pci_free_dev(spi_dev); @@ -1087,7 +1087,7 @@
/* Suppress unknown laptop warning if we booted from SPI. */ if (boot_buses & BUS_SPI) - g_laptop_ok = true; + cfg->bcfg->laptop_ok = true;
return 0; } @@ -1676,7 +1676,7 @@
/* Suppress unknown laptop warning if we booted from SPI. */ if (!ret && want_spi) - g_laptop_ok = true; + cfg->bcfg->laptop_ok = true;
return ret; } diff --git a/flashrom.c b/flashrom.c index b135e58..01c78fd 100644 --- a/flashrom.c +++ b/flashrom.c @@ -65,10 +65,6 @@ */ static bool may_register_shutdown = false;
-struct programmer_cfg { - char *params; -}; - /* Register a function to be executed on programmer shutdown. * The advantage over atexit() is that you can supply a void pointer which will * be used as parameter to the registered function upon programmer shutdown. diff --git a/include/programmer.h b/include/programmer.h index 157de2e..5b304f5 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -30,7 +30,11 @@ USB, OTHER, }; -struct programmer_cfg; +struct board_cfg; +struct programmer_cfg { + char *params; + struct board_cfg *bcfg; +};
struct dev_entry { uint16_t vendor_id; @@ -162,6 +166,7 @@
struct board_cfg { int is_laptop; + bool laptop_ok; };
struct board_match { @@ -267,7 +272,6 @@ #endif
#if CONFIG_INTERNAL == 1 -extern bool g_laptop_ok; extern bool force_boardmismatch; void probe_superio(void); int register_superio(struct superio s); diff --git a/internal.c b/internal.c index 1f0e6ee..faeb463 100644 --- a/internal.c +++ b/internal.c @@ -27,8 +27,6 @@ #include "hwaccess_x86_io.h" #endif
-bool g_laptop_ok = false; - bool force_boardmismatch = false;
enum chipbustype internal_buses_supported = BUS_NONE; @@ -107,33 +105,37 @@ return 0; }
-static void report_nonwl_laptop_detected(int is_laptop, bool laptop_ok) +static void report_nonwl_laptop_detected(const struct board_cfg *bcfg) { - if (is_laptop && !laptop_ok) { - msg_pinfo("========================================================================\n"); - if (is_laptop == 1) { - msg_pinfo("You seem to be running flashrom on an unknown laptop. Some\n" - "internal buses have been disabled for safety reasons.\n\n"); - } else { - msg_pinfo("You may be running flashrom on an unknown laptop. We could not\n" - "detect this for sure because your vendor has not set up the SMBIOS\n" - "tables correctly. Some internal buses have been disabled for\n" - "safety reasons. You can enforce using all buses by adding\n" - " -p internal:laptop=this_is_not_a_laptop\n" - "to the command line, but please read the following warning if you\n" - "are not sure.\n\n"); - } - msg_perr("Laptops, notebooks and netbooks are difficult to support and we\n" - "recommend to use the vendor flashing utility. The embedded controller\n" - "(EC) in these machines often interacts badly with flashing.\n" - "See the manpage and https://flashrom.org/Laptops for details.\n\n" - "If flash is shared with the EC, erase is guaranteed to brick your laptop\n" - "and write may brick your laptop.\n" - "Read and probe may irritate your EC and cause fan failure, backlight\n" - "failure and sudden poweroff.\n" - "You have been warned.\n" - "========================================================================\n"); + const int is_laptop = bcfg->is_laptop; + const bool laptop_ok = bcfg->laptop_ok; + + if (!is_laptop || laptop_ok) + return; + + msg_pinfo("========================================================================\n"); + if (is_laptop == 1) { + msg_pinfo("You seem to be running flashrom on an unknown laptop. Some\n" + "internal buses have been disabled for safety reasons.\n\n"); + } else { + msg_pinfo("You may be running flashrom on an unknown laptop. We could not\n" + "detect this for sure because your vendor has not set up the SMBIOS\n" + "tables correctly. Some internal buses have been disabled for\n" + "safety reasons. You can enforce using all buses by adding\n" + " -p internal:laptop=this_is_not_a_laptop\n" + "to the command line, but please read the following warning if you\n" + "are not sure.\n\n"); } + msg_perr("Laptops, notebooks and netbooks are difficult to support and we\n" + "recommend to use the vendor flashing utility. The embedded controller\n" + "(EC) in these machines often interacts badly with flashing.\n" + "See the manpage and https://flashrom.org/Laptops for details.\n\n" + "If flash is shared with the EC, erase is guaranteed to brick your laptop\n" + "and write may brick your laptop.\n" + "Read and probe may irritate your EC and cause fan failure, backlight\n" + "failure and sudden poweroff.\n" + "You have been warned.\n" + "========================================================================\n"); }
static int internal_init(const struct programmer_cfg *cfg) @@ -157,9 +159,6 @@ if (ret) return ret;
- /* Unconditionally reset global state from previous operation. */ - g_laptop_ok = false; - /* Default to Parallel/LPC/FWH flash devices. If a known host controller * is found, the host controller init routine sets the * internal_buses_supported bitfield. @@ -228,13 +227,15 @@ * this isn't a laptop. Board-enables may override this, * non-legacy buses (SPI and opaque atm) are probed anyway. */ - if (bcfg.is_laptop && !(g_laptop_ok || force_laptop || (not_a_laptop && bcfg.is_laptop == 2))) + if (bcfg.is_laptop && !(bcfg.laptop_ok || force_laptop || (not_a_laptop && bcfg.is_laptop == 2))) internal_buses_supported = BUS_NONE;
/* try to enable it. Failure IS an option, since not all motherboards * really need this to be done, etc., etc. */ - ret = chipset_flash_enable(cfg); + struct programmer_cfg icfg = *cfg; + icfg.bcfg = &bcfg; + ret = chipset_flash_enable(&icfg); if (ret == -2) { msg_perr("WARNING: No chipset found. Flash detection " "will most likely fail.\n"); @@ -258,7 +259,7 @@ internal_par_init(internal_buses_supported);
/* Report if a non-whitelisted laptop is detected that likely uses a legacy bus. */ - report_nonwl_laptop_detected(bcfg.is_laptop, g_laptop_ok); + report_nonwl_laptop_detected(&bcfg);
ret = 0;