From 67b5526d5c46bfc4d70fb288b9227097fc113e30 Mon Sep 17 00:00:00 2001 From: Edward O'Callaghan Date: Mon, 6 Mar 2023 11:25:52 +1100 Subject: 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 Reviewed-on: https://review.coreboot.org/c/flashrom/+/73456 Tested-by: build bot (Jenkins) Reviewed-by: Sam McNally --- board_enable.c | 2 +- chipset_enable.c | 8 +++---- flashrom.c | 4 ---- include/programmer.h | 8 +++++-- internal.c | 67 ++++++++++++++++++++++++++-------------------------- 5 files changed, 45 insertions(+), 44 deletions(-) diff --git a/board_enable.c b/board_enable.c index f95430a8..d9af44e9 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2293,7 +2293,7 @@ static int p2_not_a_laptop(struct board_cfg *cfg) 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 37a48fc9..16ef0f46 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -826,7 +826,7 @@ static int enable_flash_ich_spi(const struct programmer_cfg *cfg, struct pci_dev /* 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 @@ static int enable_flash_pch100_or_c620(const struct programmer_cfg *cfg, /* 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 @@ static int enable_flash_silvermont(const struct programmer_cfg *cfg, struct pci_ /* 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 @@ static int enable_flash_mcp6x_7x(const struct programmer_cfg *cfg, struct pci_de /* 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 b135e580..01c78fd5 100644 --- a/flashrom.c +++ b/flashrom.c @@ -65,10 +65,6 @@ static struct shutdown_func_data { */ 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 157de2e2..5b304f55 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -30,7 +30,11 @@ enum programmer_type { 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 @@ enum board_match_phase { struct board_cfg { int is_laptop; + bool laptop_ok; }; struct board_match { @@ -267,7 +272,6 @@ extern int superio_count; #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 1f0e6eef..faeb4635 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 @@ static int get_params(const struct programmer_cfg *cfg, 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 @@ static int internal_init(const struct programmer_cfg *cfg) 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 @@ static int internal_init(const struct programmer_cfg *cfg) * 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 @@ static int internal_init(const struct programmer_cfg *cfg) 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; -- cgit v1.2.3