April 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Anastasia Klimchuk (Code Review) April 5, 2021

April 5, 2021

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: This now has mocks in a separate compilation unit, thanks Nico and cmocka for inspiration. This is rebased on the top of memory checks from here https://review.coreboot.org/c/flashrom/+/51243 and everything works together nicely. Dummy has been discovered to leak memory on shutdown, so memory checks works. The patch is obviously too big and I will split it into three (one for each driver). But before splitting if you could tell me what do you think? Particularly about splitting hwaccess.h , is it an acceptable way to do it? -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Apr 2021 06:48:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Anastasia Klimchuk (Code Review) April 5, 2021

April 5, 2021

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/51487 to look at the new patch set (#2). Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Add unit test to run init/shutdown for enabled drivers I want to do this for all drivers in programmer table, this patch has 3 of them as a proof of concept. There is no hardware in unit test environment, all operations with hardware need to be mocked/redefined. Calls to __wrap functions are now logged to stdout, makes it easier to understand what’s happening and I find it really helps when writing tests (and can be useful when debugging tests). Extract from meson-logs/testlog.txt for new test: [ RUN ] linux_spi_init_and_shutdown_test_success Testing programmer_init for programmer=25 ... __wrap_open64 is called __wrap_ioctl is called __wrap_ioctl is called __wrap_ioctl is called __wrap_fopen64 is called ... programmer_init for programmer=25 successful Testing programmer_shutdown for programmer=25 ... ... programmer_shutdown for programmer=25 successful [ OK ] linux_spi_init_and_shutdown_test_success BUG=b:181803212 TEST=builds and ninja test Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c M hwaccess.h A hwaccess_io.h A hwaccess_io_unittest.h M meson.build A tests/init_shutdown.c M tests/meson.build M tests/tests.c M tests/tests.h 9 files changed, 374 insertions(+), 138 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/51487/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Makefile,meson.build: Fix dependency issues with raiden_debug_spi
by Edward O'Callaghan (Code Review) April 1, 2021

April 1, 2021

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/51930 ) Change subject: Makefile,meson.build: Fix dependency issues with raiden_debug_spi ...................................................................... Makefile,meson.build: Fix dependency issues with raiden_debug_spi The Raiden debug external spi programmer need only depend on libusb and is unrelated to libpci. Correct meson and gnu make builds where meson had configuration in the incorrect section and gnu make artifactually disabled the build on Windows/MinGW which doesn't have libpci. BUG=none TEST=still continues to build. Change-Id: I2d6a8c33a2228abf006a9b278bcb7133765c7074 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/51930 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M Makefile M meson.build 2 files changed, 1 insertion(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve diff --git a/Makefile b/Makefile index 6d37d55..0cfff55 100644 --- a/Makefile +++ b/Makefile @@ -248,11 +248,6 @@ else override CONFIG_RAYER_SPI = no endif -ifeq ($(CONFIG_RAIDEN_DEBUG_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_RAIDEN_DEBUG_SPI=yes -else -override CONFIG_RAIDEN_DEBUG_SPI = no -endif ifeq ($(CONFIG_NIC3COM), yes) UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes else diff --git a/meson.build b/meson.build index 5b339e0..db62541 100644 --- a/meson.build +++ b/meson.build @@ -99,6 +99,7 @@ config_digilent_spi = false config_developerbox_spi = false config_pickit2_spi = false + config_raiden_debug_spi = false endif # some programmers require libpci @@ -113,7 +114,6 @@ config_atavia = false config_drkaiser = false config_gfxnvidia = false - config_raiden_debug_spi = false config_internal = false config_it8212 = false config_nic3com = false -- To view, visit https://review.coreboot.org/c/flashrom/+/51930 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2d6a8c33a2228abf006a9b278bcb7133765c7074 Gerrit-Change-Number: 51930 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: [RFC] Makefile: Deflate dependency handling
by Angel Pons (Code Review) April 1, 2021

April 1, 2021

Attention is currently required from: Edward O'Callaghan, Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52024 ) Change subject: [RFC] Makefile: Deflate dependency handling ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52024 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I33049474a95d288c9a84e412633c99fbc251ac03 Gerrit-Change-Number: 52024 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Thu, 01 Apr 2021 12:41:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] Makefile: Deflate dependency handling
by Nico Huber (Code Review) April 1, 2021

April 1, 2021

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52024 ) Change subject: [RFC] Makefile: Deflate dependency handling ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: There are some more classes that could be extracted, e.g. NEEDS_RAW_ACCESS, NEEDS_TTY. -- To view, visit https://review.coreboot.org/c/flashrom/+/52024 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I33049474a95d288c9a84e412633c99fbc251ac03 Gerrit-Change-Number: 52024 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 01 Apr 2021 12:35:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] Makefile: Deflate dependency handling
by Nico Huber (Code Review) April 1, 2021

April 1, 2021

Attention is currently required from: Edward O'Callaghan, Angel Pons. Hello Edward O'Callaghan, Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/52024 to review the following change. Change subject: [RFC] Makefile: Deflate dependency handling ...................................................................... [RFC] Makefile: Deflate dependency handling Change-Id: I33049474a95d288c9a84e412633c99fbc251ac03 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M Makefile 1 file changed, 97 insertions(+), 487 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/52024/1 diff --git a/Makefile b/Makefile index 6d37d55..570383a 100644 --- a/Makefile +++ b/Makefile @@ -87,6 +87,70 @@ { $(1) ; } | tee -a $(BUILD_DETAILS_FILE) ; echo >&2 ; } 2>>$(BUILD_DETAILS_FILE)) ############################################################################### +# Dependency handling. + +NEEDS_LIBPCI := \ + CONFIG_INTERNAL \ + CONFIG_NIC3COM \ + CONFIG_GFXNVIDIA \ + CONFIG_SATASII \ + CONFIG_ATAHPT \ + CONFIG_ATAVIA \ + CONFIG_ATAPROMISE \ + CONFIG_ENE_LPC \ + CONFIG_IT8212 \ + CONFIG_DRKAISER \ + CONFIG_MEC1308 \ + CONFIG_NICREALTEK \ + CONFIG_NICNATSEMI \ + CONFIG_NICINTEL \ + CONFIG_NICINTEL_SPI \ + CONFIG_NICINTEL_EEPROM \ + CONFIG_OGP_SPI \ + CONFIG_SATAMV \ + +NEEDS_LIBUSB1 := \ + CONFIG_CH341A_SPI \ + CONFIG_DEDIPROG \ + CONFIG_DEVELOPERBOX_SPI \ + CONFIG_DIGILENT_SPI \ + CONFIG_PICKIT2_SPI \ + CONFIG_STLINKV3_SPI \ + +NEEDS_LIBFTDI := \ + CONFIG_FT2232_SPI \ + CONFIG_USBBLASTER_SPI \ + +NEEDS_LIBJAYLINK := \ + CONFIG_JLINK_SPI \ + +define process_unsupported +$(foreach p,$1, \ + $(if $(filter yes,$($(p))), \ + $(eval UNSUPPORTED_FEATURES += $(p)=yes), + $(eval override $(p) := no))) +endef + +define filter_deps +$(foreach p,$1, \ + $(if $(filter yes,$($(p))), \ + $(p))) +endef + +define disable_all +$(foreach p,$1, \ + $(eval override $(p) := no)) +endef + +ifeq ($(CONFIG_ENABLE_LIBUSB1_PROGRAMMERS), no) +$(call disable_all,$(NEEDS_LIBUSB1)) +endif + +ifeq ($(CONFIG_ENABLE_LIBPCI_PROGRAMMERS), no) +$(call disable_all,$(NEEDS_LIBPCI)) +endif + +############################################################################### # General OS-specific settings. # 1. Prepare for later by gathering information about host and target OS # 2. Set compiler flags and parameters according to OSes @@ -141,88 +205,11 @@ override CFLAGS += -Wno-format LIBS += -lgetopt # Bus Pirate, Serprog and PonyProg are not supported under DOS (missing serial support). -ifeq ($(CONFIG_BUSPIRATE_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_BUSPIRATE_SPI=yes -else -override CONFIG_BUSPIRATE_SPI = no -endif -ifeq ($(CONFIG_SERPROG), yes) -UNSUPPORTED_FEATURES += CONFIG_SERPROG=yes -else -override CONFIG_SERPROG = no -endif -ifeq ($(CONFIG_PONY_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes -else -override CONFIG_PONY_SPI = no -endif -# Digilent SPI, Dediprog, Developerbox, USB-Blaster, PICkit2, CH341A and FT2232 are not supported under DOS (missing USB support). -ifeq ($(CONFIG_DIGILENT_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_DIGILENT_SPI=yes -else -override CONFIG_DIGILENT_SPI = no -endif -ifeq ($(CONFIG_DEDIPROG), yes) -UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes -else -override CONFIG_DEDIPROG = no -endif -ifeq ($(CONFIG_DEVELOPERBOX_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_DEVELOPERBOX_SPI=yes -else -override CONFIG_DEVELOPERBOX_SPI = no -endif -ifeq ($(CONFIG_ENE_LPC), yes) -UNSUPPORTED_FEATURES += CONFIG_ENE_LPC=yes -else -override CONFIG_ENE_LPC = no -endif -ifeq ($(CONFIG_FT2232_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_FT2232_SPI=yes -else -override CONFIG_FT2232_SPI = no -endif -ifeq ($(CONFIG_MEC1308), yes) -UNSUPPORTED_FEATURES += CONFIG_MEC1308=yes -else -override CONFIG_MEC1308 = no -endif -ifeq ($(CONFIG_USBBLASTER_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_USBBLASTER_SPI=yes -else -override CONFIG_USBBLASTER_SPI = no -endif -ifeq ($(CONFIG_PICKIT2_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes -else -override CONFIG_PICKIT2_SPI = no -endif -ifeq ($(CONFIG_CH341A_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_CH341A_SPI=yes -else -override CONFIG_CH341A_SPI = no -endif -ifeq ($(CONFIG_STLINKV3_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_STLINKV3_SPI=yes -else -override CONFIG_STLINKV3_SPI = no -endif -ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes -else -override CONFIG_LSPCON_I2C_SPI = no -endif -ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes -else -override CONFIG_REALTEK_MST_I2C_SPI = no -endif -# libjaylink is also not available for DOS -ifeq ($(CONFIG_JLINK_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_JLINK_SPI=yes -else -override CONFIG_JLINK_SPI = no -endif +$(call process_unsupported,CONFIG_BUSPIRATE_SPI CONFIG_SERPROG CONFIG_PONY_SPI) +# Libraries not available for DOS +$(call process_unsupported,$(NEEDS_LIBUSB1) $(NEEDS_LIBFTDI) $(NEEDS_LIBJAYLINK)) +# Odd ones (FIXME: why are they unsupported?) +$(call process_unsupported,CONFIG_ENE_LPC CONFIG_MEC1308) endif # FIXME: Should we check for Cygwin/MSVC as well? @@ -238,125 +225,14 @@ CONFIG_NI845X_SPI ?= no # For now we disable all PCI-based programmers on Windows/MinGW (no libpci). -ifeq ($(CONFIG_INTERNAL), yes) -UNSUPPORTED_FEATURES += CONFIG_INTERNAL=yes -else -override CONFIG_INTERNAL = no -endif -ifeq ($(CONFIG_RAYER_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_RAYER_SPI=yes -else -override CONFIG_RAYER_SPI = no -endif -ifeq ($(CONFIG_RAIDEN_DEBUG_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_RAIDEN_DEBUG_SPI=yes -else -override CONFIG_RAIDEN_DEBUG_SPI = no -endif -ifeq ($(CONFIG_NIC3COM), yes) -UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes -else -override CONFIG_NIC3COM = no -endif -ifeq ($(CONFIG_GFXNVIDIA), yes) -UNSUPPORTED_FEATURES += CONFIG_GFXNVIDIA=yes -else -override CONFIG_GFXNVIDIA = no -endif -ifeq ($(CONFIG_SATASII), yes) -UNSUPPORTED_FEATURES += CONFIG_SATASII=yes -else -override CONFIG_SATASII = no -endif -ifeq ($(CONFIG_ATAHPT), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAHPT=yes -else -override CONFIG_ATAHPT = no -endif -ifeq ($(CONFIG_ATAVIA), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAVIA=yes -else -override CONFIG_ATAVIA = no -endif -ifeq ($(CONFIG_ATAPROMISE), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAPROMISE=yes -else -override CONFIG_ATAPROMISE = no -endif -ifeq ($(CONFIG_ENE_LPC), yes) -UNSUPPORTED_FEATURES += CONFIG_ENE_LPC=yes -else -override CONFIG_ENE_LPC = no -endif -ifeq ($(CONFIG_IT8212), yes) -UNSUPPORTED_FEATURES += CONFIG_IT8212=yes -else -override CONFIG_IT8212 = no -endif -ifeq ($(CONFIG_DRKAISER), yes) -UNSUPPORTED_FEATURES += CONFIG_DRKAISER=yes -else -override CONFIG_DRKAISER = no -endif -ifeq ($(CONFIG_MEC1308), yes) -UNSUPPORTED_FEATURES += CONFIG_MEC1308=yes -else -override CONFIG_MEC1308 = no -endif -ifeq ($(CONFIG_NICREALTEK), yes) -UNSUPPORTED_FEATURES += CONFIG_NICREALTEK=yes -else -override CONFIG_NICREALTEK = no -endif -ifeq ($(CONFIG_NICNATSEMI), yes) -UNSUPPORTED_FEATURES += CONFIG_NICNATSEMI=yes -else -override CONFIG_NICNATSEMI = no -endif -ifeq ($(CONFIG_NICINTEL), yes) -UNSUPPORTED_FEATURES += CONFIG_NICINTEL=yes -else -override CONFIG_NICINTEL = no -endif -ifeq ($(CONFIG_NICINTEL_EEPROM), yes) -UNSUPPORTED_FEATURES += CONFIG_NICINTEL_EEPROM=yes -else -override CONFIG_NICINTEL_EEPROM = no -endif -ifeq ($(CONFIG_NICINTEL_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_NICINTEL_SPI=yes -else -override CONFIG_NICINTEL_SPI = no -endif -ifeq ($(CONFIG_OGP_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_OGP_SPI=yes -else -override CONFIG_OGP_SPI = no -endif -ifeq ($(CONFIG_SATAMV), yes) -UNSUPPORTED_FEATURES += CONFIG_SATAMV=yes -else -override CONFIG_SATAMV = no -endif -ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes -else -override CONFIG_LSPCON_I2C_SPI = no -endif -ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes -else -override CONFIG_REALTEK_MST_I2C_SPI = no -endif +$(call process_unsupported,$(NEEDS_LIBPCI)) +# Odd ones (FIXME: why?) +$(call process_unsupported,CONFIG_RAYER_SPI CONFIG_RAIDEN_DEBUG_SPI) endif ifneq ($(TARGET_OS), MinGW) # NI USB-845x only supported on Windows at the moment -ifeq ($(CONFIG_NI845X_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_NI845X_SPI=yes -else -override CONFIG_NI845X_SPI = no -endif +$(call process_unsupported,CONFIG_NI845X_SPI) endif ifeq ($(TARGET_OS), libpayload) @@ -365,119 +241,28 @@ $(info Setting default goal to libflashrom.a) endif FLASHROM_CFLAGS += -DSTANDALONE -ifeq ($(CONFIG_DUMMY), yes) -UNSUPPORTED_FEATURES += CONFIG_DUMMY=yes -else -override CONFIG_DUMMY = no -endif +$(call process_unsupported,CONFIG_DUMMY) # libpayload does not provide the romsize field in struct pci_dev that the atapromise code requires. -ifeq ($(CONFIG_ATAPROMISE), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAPROMISE=yes -else -override CONFIG_ATAPROMISE = no -endif +$(call process_unsupported,CONFIG_ATAPROMISE) # Bus Pirate, Serprog and PonyProg are not supported with libpayload (missing serial support). -ifeq ($(CONFIG_BUSPIRATE_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_BUSPIRATE_SPI=yes -else -override CONFIG_BUSPIRATE_SPI = no -endif -ifeq ($(CONFIG_SERPROG), yes) -UNSUPPORTED_FEATURES += CONFIG_SERPROG=yes -else -override CONFIG_SERPROG = no -endif -ifeq ($(CONFIG_PONY_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes -else -override CONFIG_PONY_SPI = no -endif +$(call process_unsupported,CONFIG_BUSPIRATE_SPI CONFIG_SERPROG CONFIG_PONY_SPI) # Dediprog, Developerbox, USB-Blaster, PICkit2, CH341A and FT2232 are not supported with libpayload (missing libusb support). -ifeq ($(CONFIG_DEDIPROG), yes) -UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes -else -override CONFIG_DEDIPROG = no -endif -ifeq ($(CONFIG_DEVELOPERBOX_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_DEVELOPERBOX_SPI=yes -else -override CONFIG_DEVELOPERBOX_SPI = no -endif -ifeq ($(CONFIG_ENE_LPC), yes) -UNSUPPORTED_FEATURES += CONFIG_ENE_LPC=yes -else -override CONFIG_ENE_LPC = no -endif -ifeq ($(CONFIG_FT2232_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_FT2232_SPI=yes -else -override CONFIG_FT2232_SPI = no -endif -ifeq ($(CONFIG_MEC1308), yes) -UNSUPPORTED_FEATURES += CONFIG_MEC1308=yes -else -override CONFIG_MEC1308 = no -endif -ifeq ($(CONFIG_USBBLASTER_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_USBBLASTER_SPI=yes -else -override CONFIG_USBBLASTER_SPI = no -endif -ifeq ($(CONFIG_PICKIT2_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes -else -override CONFIG_PICKIT2_SPI = no -endif -ifeq ($(CONFIG_STLINKV3_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_STLINKV3_SPI=yes -else -override CONFIG_STLINKV3_SPI = no -endif -ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes -else -override CONFIG_LSPCON_I2C_SPI = no -endif -ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes -else -override CONFIG_REALTEK_MST_I2C_SPI = no -endif -ifeq ($(CONFIG_CH341A_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_CH341A_SPI=yes -else -override CONFIG_CH341A_SPI = no -endif +$(call process_unsupported,$(NEEDS_LIBUSB1) $(NEEDS_LIBFTDI) $(NEEDS_LIBJAYLINK)) +# Odd ones. (FIXME: why?) +$(call process_unsupported,CONFIG_ENE_LPC CONFIG_MEC1308) endif ifneq ($(TARGET_OS), Linux) -# Android is handled internally as separate OS, but it supports CONFIG_LINUX_SPI and CONFIG_MSTARDDC_SPI +# Android is handled internally as separate OS, but it supports about the same drivers. ifneq ($(TARGET_OS), Android) -ifeq ($(CONFIG_LINUX_MTD), yes) -UNSUPPORTED_FEATURES += CONFIG_LINUX_MTD=yes -else -override CONFIG_LINUX_MTD = no -endif -ifeq ($(CONFIG_LINUX_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LINUX_SPI=yes -else -override CONFIG_LINUX_SPI = no -endif -ifeq ($(CONFIG_MSTARDDC_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_MSTARDDC_SPI=yes -else -override CONFIG_MSTARDDC_SPI = no -endif +$(call process_unsupported,CONFIG_LINUX_MTD CONFIG_LINUX_SPI) +$(call process_unsupported,CONFIG_MSTARDDC_SPI CONFIG_LSPCON_I2C_SPI CONFIG_REALTEK_MST_I2C_SPI) endif endif ifeq ($(TARGET_OS), Android) # Android on x86 (currently) does not provide raw PCI port I/O operations -ifeq ($(CONFIG_RAYER_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_RAYER_SPI=yes -else -override CONFIG_RAYER_SPI = no -endif +$(call process_unsupported,CONFIG_RAYER_SPI) endif ifeq ($(TARGET_OS), Linux) @@ -499,147 +284,24 @@ # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel) ifneq ($(ARCH)-little, $(filter $(ARCH),x86 mips)-$(ENDIAN)) -ifeq ($(CONFIG_INTERNAL), yes) -UNSUPPORTED_FEATURES += CONFIG_INTERNAL=yes -else -override CONFIG_INTERNAL = no -endif +$(call process_unsupported,CONFIG_INTERNAL) endif # PCI port I/O support is unimplemented on PPC/MIPS/SPARC and unavailable on ARM. # Right now this means the drivers below only work on x86. ifneq ($(ARCH), x86) -ifeq ($(CONFIG_NIC3COM), yes) -UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes -else -override CONFIG_NIC3COM = no -endif -ifeq ($(CONFIG_NICREALTEK), yes) -UNSUPPORTED_FEATURES += CONFIG_NICREALTEK=yes -else -override CONFIG_NICREALTEK = no -endif -ifeq ($(CONFIG_NICNATSEMI), yes) -UNSUPPORTED_FEATURES += CONFIG_NICNATSEMI=yes -else -override CONFIG_NICNATSEMI = no -endif -ifeq ($(CONFIG_RAYER_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_RAYER_SPI=yes -else -override CONFIG_RAYER_SPI = no -endif -ifeq ($(CONFIG_ATAHPT), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAHPT=yes -else -override CONFIG_ATAHPT = no -endif -ifeq ($(CONFIG_ATAPROMISE), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAPROMISE=yes -else -override CONFIG_ATAPROMISE = no -endif -ifeq ($(CONFIG_SATAMV), yes) -UNSUPPORTED_FEATURES += CONFIG_SATAMV=yes -else -override CONFIG_SATAMV = no -endif -ifeq ($(CONFIG_ENE_LPC), yes) -UNSUPPORTED_FEATURES += CONFIG_ENE_LPC=yes -else -override CONFIG_ENE_LPC = no -endif -ifeq ($(CONFIG_MEC1308), yes) -UNSUPPORTED_FEATURES += CONFIG_MEC1308=yes -else -override CONFIG_MEC1308 = no -endif +$(call process_unsupported,CONFIG_NIC3COM CONFIG_NICREALTEK CONFIG_NICNATSEMI) +$(call process_unsupported,CONFIG_RAYER_SPI CONFIG_ATAHPT CONFIG_ATAPROMISE) +$(call process_unsupported,CONFIG_SATAMV CONFIG_ENE_LPC CONFIG_MEC1308) endif -# Disable all drivers needing raw access (memory, PCI, port I/O) on -# architectures with unknown raw access properties. +# Additionally disable all drivers needing raw access (memory, PCI, port I/O) +# on architectures with unknown raw access properties. # Right now those architectures are alpha hppa m68k sh s390 ifneq ($(ARCH),$(filter $(ARCH),x86 mips ppc arm sparc arc)) -ifeq ($(CONFIG_RAYER_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_RAYER_SPI=yes -else -override CONFIG_RAYER_SPI = no -endif -ifeq ($(CONFIG_NIC3COM), yes) -UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes -else -override CONFIG_NIC3COM = no -endif -ifeq ($(CONFIG_GFXNVIDIA), yes) -UNSUPPORTED_FEATURES += CONFIG_GFXNVIDIA=yes -else -override CONFIG_GFXNVIDIA = no -endif -ifeq ($(CONFIG_SATASII), yes) -UNSUPPORTED_FEATURES += CONFIG_SATASII=yes -else -override CONFIG_SATASII = no -endif -ifeq ($(CONFIG_ATAHPT), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAHPT=yes -else -override CONFIG_ATAHPT = no -endif -ifeq ($(CONFIG_ATAVIA), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAVIA=yes -else -override CONFIG_ATAVIA = no -endif -ifeq ($(CONFIG_ATAPROMISE), yes) -UNSUPPORTED_FEATURES += CONFIG_ATAPROMISE=yes -else -override CONFIG_ATAPROMISE = no -endif -ifeq ($(CONFIG_DRKAISER), yes) -UNSUPPORTED_FEATURES += CONFIG_DRKAISER=yes -else -override CONFIG_DRKAISER = no -endif -ifeq ($(CONFIG_NICREALTEK), yes) -UNSUPPORTED_FEATURES += CONFIG_NICREALTEK=yes -else -override CONFIG_NICREALTEK = no -endif -ifeq ($(CONFIG_NICNATSEMI), yes) -UNSUPPORTED_FEATURES += CONFIG_NICNATSEMI=yes -else -override CONFIG_NICNATSEMI = no -endif -ifeq ($(CONFIG_NICINTEL), yes) -UNSUPPORTED_FEATURES += CONFIG_NICINTEL=yes -else -override CONFIG_NICINTEL = no -endif -ifeq ($(CONFIG_NICINTEL_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_NICINTEL_SPI=yes -else -override CONFIG_NICINTEL_SPI = no -endif -ifeq ($(CONFIG_NICINTEL_EEPROM), yes) -UNSUPPORTED_FEATURES += CONFIG_NICINTEL_EEPROM=yes -else -override CONFIG_NICINTEL_EEPROM = no -endif -ifeq ($(CONFIG_OGP_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_OGP_SPI=yes -else -override CONFIG_OGP_SPI = no -endif -ifeq ($(CONFIG_SATAMV), yes) -UNSUPPORTED_FEATURES += CONFIG_SATAMV=yes -else -override CONFIG_SATAMV = no -endif -ifeq ($(CONFIG_IT8212), yes) -UNSUPPORTED_FEATURES += CONFIG_IT8212=yes -else -override CONFIG_IT8212 = no -endif +$(call process_unsupported,CONFIG_GFXNVIDIA CONFIG_SATASII CONFIG_ATAVIA) +$(call process_unsupported,CONFIG_DRKAISER CONFIG_NICINTEL CONFIG_NICINTEL_SPI) +$(call process_unsupported,CONFIG_NICINTEL_EEPROM CONFIG_OGP_SPI CONFIG_IT8212) endif ############################################################################### @@ -814,36 +476,6 @@ $(eval $(var)=yes))) endif -ifeq ($(CONFIG_ENABLE_LIBUSB1_PROGRAMMERS), no) -override CONFIG_CH341A_SPI = no -override CONFIG_DEDIPROG = no -override CONFIG_DIGILENT_SPI = no -override CONFIG_DEVELOPERBOX_SPI = no -override CONFIG_PICKIT2_SPI = no -override CONFIG_RAIDEN_DEBUG_SPI = no -override CONFIG_STLINKV3_SPI = no -endif -ifeq ($(CONFIG_ENABLE_LIBPCI_PROGRAMMERS), no) -override CONFIG_INTERNAL = no -override CONFIG_NIC3COM = no -override CONFIG_GFXNVIDIA = no -override CONFIG_SATASII = no -override CONFIG_ATAHPT = no -override CONFIG_ATAVIA = no -override CONFIG_ATAPROMISE = no -override CONFIG_ENE_LPC = no -override CONFIG_IT8212 = no -override CONFIG_DRKAISER = no -override CONFIG_MEC1308 = no -override CONFIG_NICREALTEK = no -override CONFIG_NICNATSEMI = no -override CONFIG_NICINTEL = no -override CONFIG_NICINTEL_SPI = no -override CONFIG_NICINTEL_EEPROM = no -override CONFIG_OGP_SPI = no -override CONFIG_SATAMV = no -endif - # Bitbanging SPI infrastructure, default off unless needed. ifeq ($(CONFIG_RAYER_SPI), yes) override CONFIG_BITBANG_SPI = yes @@ -902,21 +534,18 @@ endif else endif -NEED_LIBPCI += CONFIG_INTERNAL endif ifeq ($(CONFIG_ENE_LPC), yes) FEATURE_CFLAGS += -D'CONFIG_ENE_LPC=1' PROGRAMMER_OBJS += ene_lpc.o NEED_RAW_ACCESS += CONFIG_ENE_LPC -NEED_LIBPCI += CONFIG_ENE_LPC endif ifeq ($(CONFIG_MEC1308), yes) FEATURE_CFLAGS += -D'CONFIG_MEC1308=1' PROGRAMMER_OBJS += mec1308.o NEED_RAW_ACCESS += CONFIG_MEC1308 -NEED_LIBPCI += CONFIG_MEC1308 endif ifeq ($(CONFIG_SERPROG), yes) @@ -951,69 +580,58 @@ ifeq ($(CONFIG_NIC3COM), yes) FEATURE_CFLAGS += -D'CONFIG_NIC3COM=1' PROGRAMMER_OBJS += nic3com.o -NEED_LIBPCI += CONFIG_NIC3COM endif ifeq ($(CONFIG_GFXNVIDIA), yes) FEATURE_CFLAGS += -D'CONFIG_GFXNVIDIA=1' PROGRAMMER_OBJS += gfxnvidia.o -NEED_LIBPCI += CONFIG_GFXNVIDIA endif ifeq ($(CONFIG_SATASII), yes) FEATURE_CFLAGS += -D'CONFIG_SATASII=1' PROGRAMMER_OBJS += satasii.o -NEED_LIBPCI += CONFIG_SATASII endif ifeq ($(CONFIG_ATAHPT), yes) FEATURE_CFLAGS += -D'CONFIG_ATAHPT=1' PROGRAMMER_OBJS += atahpt.o -NEED_LIBPCI += CONFIG_ATAHPT endif ifeq ($(CONFIG_ATAVIA), yes) FEATURE_CFLAGS += -D'CONFIG_ATAVIA=1' PROGRAMMER_OBJS += atavia.o -NEED_LIBPCI += CONFIG_ATAVIA endif ifeq ($(CONFIG_ATAPROMISE), yes) FEATURE_CFLAGS += -D'CONFIG_ATAPROMISE=1' PROGRAMMER_OBJS += atapromise.o -NEED_LIBPCI += CONFIG_ATAPROMISE endif ifeq ($(CONFIG_IT8212), yes) FEATURE_CFLAGS += -D'CONFIG_IT8212=1' PROGRAMMER_OBJS += it8212.o -NEED_LIBPCI += CONFIG_IT8212 endif ifeq ($(CONFIG_FT2232_SPI), yes) # This is a totally ugly hack. FEATURE_CFLAGS += $(call debug_shell,grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'CONFIG_FT2232_SPI=1'") -NEED_LIBFTDI += CONFIG_FT2232_SPI PROGRAMMER_OBJS += ft2232_spi.o endif ifeq ($(CONFIG_USBBLASTER_SPI), yes) # This is a totally ugly hack. FEATURE_CFLAGS += $(call debug_shell,grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'CONFIG_USBBLASTER_SPI=1'") -NEED_LIBFTDI += CONFIG_USBBLASTER_SPI PROGRAMMER_OBJS += usbblaster_spi.o endif ifeq ($(CONFIG_PICKIT2_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_PICKIT2_SPI=1' PROGRAMMER_OBJS += pickit2_spi.o -NEED_LIBUSB1 += CONFIG_PICKIT2_SPI endif ifeq ($(CONFIG_STLINKV3_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_STLINKV3_SPI=1' PROGRAMMER_OBJS += stlinkv3_spi.o -NEED_LIBUSB1 += CONFIG_STLINKV3_SPI endif ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) @@ -1044,43 +662,36 @@ ifeq ($(CONFIG_DRKAISER), yes) FEATURE_CFLAGS += -D'CONFIG_DRKAISER=1' PROGRAMMER_OBJS += drkaiser.o -NEED_LIBPCI += CONFIG_DRKAISER endif ifeq ($(CONFIG_NICREALTEK), yes) FEATURE_CFLAGS += -D'CONFIG_NICREALTEK=1' PROGRAMMER_OBJS += nicrealtek.o -NEED_LIBPCI += CONFIG_NICREALTEK endif ifeq ($(CONFIG_NICNATSEMI), yes) FEATURE_CFLAGS += -D'CONFIG_NICNATSEMI=1' PROGRAMMER_OBJS += nicnatsemi.o -NEED_LIBPCI += CONFIG_NICNATSEMI endif ifeq ($(CONFIG_NICINTEL), yes) FEATURE_CFLAGS += -D'CONFIG_NICINTEL=1' PROGRAMMER_OBJS += nicintel.o -NEED_LIBPCI += CONFIG_NICINTEL endif ifeq ($(CONFIG_NICINTEL_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_NICINTEL_SPI=1' PROGRAMMER_OBJS += nicintel_spi.o -NEED_LIBPCI += CONFIG_NICINTEL_SPI endif ifeq ($(CONFIG_NICINTEL_EEPROM), yes) FEATURE_CFLAGS += -D'CONFIG_NICINTEL_EEPROM=1' PROGRAMMER_OBJS += nicintel_eeprom.o -NEED_LIBPCI += CONFIG_NICINTEL_EEPROM endif ifeq ($(CONFIG_OGP_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_OGP_SPI=1' PROGRAMMER_OBJS += ogp_spi.o -NEED_LIBPCI += CONFIG_OGP_SPI endif ifeq ($(CONFIG_BUSPIRATE_SPI), yes) @@ -1092,19 +703,16 @@ ifeq ($(CONFIG_DEDIPROG), yes) FEATURE_CFLAGS += -D'CONFIG_DEDIPROG=1' PROGRAMMER_OBJS += dediprog.o -NEED_LIBUSB1 += CONFIG_DEDIPROG endif ifeq ($(CONFIG_DEVELOPERBOX_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_DEVELOPERBOX_SPI=1' PROGRAMMER_OBJS += developerbox_spi.o -NEED_LIBUSB1 += CONFIG_DEVELOPERBOX_SPI endif ifeq ($(CONFIG_SATAMV), yes) FEATURE_CFLAGS += -D'CONFIG_SATAMV=1' PROGRAMMER_OBJS += satamv.o -NEED_LIBPCI += CONFIG_SATAMV endif ifeq ($(CONFIG_LINUX_MTD), yes) @@ -1129,17 +737,14 @@ ifeq ($(CONFIG_CH341A_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_CH341A_SPI=1' PROGRAMMER_OBJS += ch341a_spi.o -NEED_LIBUSB1 += CONFIG_CH341A_SPI endif ifeq ($(CONFIG_DIGILENT_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_DIGILENT_SPI=1' PROGRAMMER_OBJS += digilent_spi.o -NEED_LIBUSB1 += CONFIG_DIGILENT_SPI endif ifeq ($(CONFIG_JLINK_SPI), yes) -NEED_LIBJAYLINK += CONFIG_JLINK_SPI FEATURE_CFLAGS += -D'CONFIG_JLINK_SPI=1' PROGRAMMER_OBJS += jlink_spi.o endif @@ -1179,6 +784,11 @@ endif endif +NEED_LIBPCI := $(call filter_deps,$(NEEDS_LIBPCI)) +NEED_LIBUSB1 := $(call filter_deps,$(NEEDS_LIBUSB1)) +NEED_LIBFTDI := $(call filter_deps,$(NEEDS_LIBFTDI)) +NEED_LIBJAYLINK := $(call filter_deps,$(NEEDS_LIBJAYLINK)) + ifneq ($(NEED_LIBPCI), ) CHECK_LIBPCI = yes # This is a dirty hack, but it saves us from checking all PCI drivers and all platforms manually. -- To view, visit https://review.coreboot.org/c/flashrom/+/52024 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I33049474a95d288c9a84e412633c99fbc251ac03 Gerrit-Change-Number: 52024 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Nico Huber (Code Review) April 1, 2021

April 1, 2021

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 1: (1 comment) File mec1308.c: https://review.coreboot.org/c/flashrom/+/51487/comment/8440124f_ff22057f PS1, Line 115: #endif /* UNIT_TEST_ENV */ > I was thinking about separate compilation units for a while, how exactly to do this. […] I guess we could have something like a `hwaccess_unittest.h`, yes. It could be hooked up via `-include` and `#define __HW_ACCESS_H__`. This way the original `hwaccess.h` would be bypassed. It could provide a single definition for the used macros, e.g. #define OUTB(v, p) test_outb(v, p) #define INB(p) test_inb(p) etc. However, the original `hwaccess.h` provides more than the I/O macros. Maybe we should split that out? There are probably many ways to solve the per-driver mocking issue. Two ideas from the top of my head: 1. Build multiple test binaries, each with its own versions of test_outb() etc. 2. Provide some abstraction via function pointers. e.g. a struct per test with struct test_io { void (*outb)(unsigned char value, unsigned short port); unsigned char (*inb)(unsigned short port); /* etc. */ } extern const struct test_io *test_io; And then before each test, set test_io = test_io_mec1308; The test_*() functions would then simply call the respective function in `test_io`, e.g.: void test_outb(unsigned char value, unsigned short port) { test_io->outb(value, port); } -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 01 Apr 2021 10:32:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Enable dynamic memory allocation checks for cmocka unit tests
by Nico Huber (Code Review) April 1, 2021

April 1, 2021

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/51243 ) Change subject: Enable dynamic memory allocation checks for cmocka unit tests ...................................................................... Enable dynamic memory allocation checks for cmocka unit tests This commit enables the feature and makes changes to existing files and tests. I am writing more new tests with this. Commit includes tests/flashrom.c because after enabling memory checks the test started to fail (it used to leak memory indeed). If you are wondering how to verify it works (because at the moment all tests [still] pass so it’s not obvious that anything has changed), then for example: 1) Remove free’s in flashbuses_to_text_test_success test, and it will fail with message similar to this (line numbers from your local source) [ ERROR ] --- Blocks allocated... ../flashrom.c:1239: note: block 0x55f42304b640 allocated here ../flashrom.c:1239: note: block 0x55f42304b5c0 allocated here ../flashrom.c:1239: note: block 0x55f42304b3d0 allocated here ../flashrom.c:1239: note: block 0x55f42304b700 allocated here ../flashrom.c:1239: note: block 0x55f42304b780 allocated here ../flashrom.c:1239: note: block 0x55f42304bb00 allocated here ../flashrom.c:1239: note: block 0x55f42304b810 allocated here ERROR: flashbuses_to_text_test_success leaked 7 block(s) 2) Add char *temp = malloc just before return from strcat_realloc [ ERROR ] --- Blocks allocated... ../helpers.c:88: note: block 0x55a51307b6c0 allocated here ../helpers.c:88: note: block 0x55a51307b9e0 allocated here ERROR: strcat_realloc_test_success leaked 2 block(s) BUG=b:181803212 TEST=builds and ninja test nm builddir/tests/flashrom_unit_tests.p/.._flashrom.c.o nm builddir/tests/flashrom_unit_tests.p/flashrom.c.o nm builddir/flashrom.p/flashrom.c.o Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/51243 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M meson.build M tests/flashrom.c M tests/helpers.c A unittest_env.h 4 files changed, 81 insertions(+), 12 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/meson.build b/meson.build index d5895d8..5b339e0 100644 --- a/meson.build +++ b/meson.build @@ -472,6 +472,10 @@ 'cli_output.c', 'flashrom.c', ], + compile_args : [ + '-includestdlib.h', + '-includeunittest_env.h', + ], dependencies : [ deps, ], diff --git a/tests/flashrom.c b/tests/flashrom.c index 50464dd..c3508c5 100644 --- a/tests/flashrom.c +++ b/tests/flashrom.c @@ -17,35 +17,54 @@ #include "programmer.h" +#define assert_equal_and_free(text, expected) \ + do { \ + assert_string_equal(text, expected); \ + free(text); \ + } while (0) + +#define assert_not_equal_and_free(text, expected) \ + do { \ + assert_string_not_equal(text, expected); \ + free(text); \ + } while (0) + + void flashbuses_to_text_test_success(void **state) { (void) state; /* unused */ enum chipbustype bustype; + char *text; bustype = BUS_NONSPI; - assert_string_equal(flashbuses_to_text(bustype), "Non-SPI"); + text = flashbuses_to_text(bustype); + assert_equal_and_free(text, "Non-SPI"); bustype |= BUS_PARALLEL; - assert_string_not_equal(flashbuses_to_text(bustype), "Non-SPI, Parallel"); + text = flashbuses_to_text(bustype); + assert_not_equal_and_free(text, "Non-SPI, Parallel"); bustype = BUS_PARALLEL; bustype |= BUS_LPC; - assert_string_equal(flashbuses_to_text(bustype), "Parallel, LPC"); + text = flashbuses_to_text(bustype); + assert_equal_and_free(text, "Parallel, LPC"); bustype |= BUS_FWH; //BUS_NONSPI = BUS_PARALLEL | BUS_LPC | BUS_FWH, - assert_string_equal(flashbuses_to_text(bustype), "Non-SPI"); + text = flashbuses_to_text(bustype); + assert_equal_and_free(text, "Non-SPI"); bustype |= BUS_SPI; - assert_string_equal(flashbuses_to_text(bustype), "Parallel, LPC, FWH, SPI"); + text = flashbuses_to_text(bustype); + assert_equal_and_free(text, "Parallel, LPC, FWH, SPI"); bustype |= BUS_PROG; - assert_string_equal( - flashbuses_to_text(bustype), - "Parallel, LPC, FWH, SPI, Programmer-specific" - ); + text = flashbuses_to_text(bustype); + assert_equal_and_free(text, + "Parallel, LPC, FWH, SPI, Programmer-specific"); bustype = BUS_NONE; - assert_string_equal(flashbuses_to_text(bustype), "None"); + text = flashbuses_to_text(bustype); + assert_equal_and_free(text, "None"); } diff --git a/tests/helpers.c b/tests/helpers.c index a920c15..4376eee 100644 --- a/tests/helpers.c +++ b/tests/helpers.c @@ -18,8 +18,6 @@ #include "flash.h" #include <stdint.h> -#include <stdlib.h> - void address_to_bits_test_success(void **state) { diff --git a/unittest_env.h b/unittest_env.h new file mode 100644 index 0000000..9bd7509 --- /dev/null +++ b/unittest_env.h @@ -0,0 +1,48 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2021 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * This header is included in all files when they are built for unit test + * environment, for all the files to be covered by dynamic memory allocation + * checks (checks for memory leaks, buffer overflows and underflows). + * + * See flashrom_test_dep in meson.build for more details. + * + * https://api.cmocka.org/group__cmocka__alloc.html + */ + +extern void* _test_malloc(const size_t size, const char* file, const int line); +extern void* _test_realloc(void *ptr, const size_t size, const char* file, const int line); +extern void* _test_calloc(const size_t number_of_elements, const size_t size, + const char* file, const int line); +extern void _test_free(void* const ptr, const char* file, const int line); + +#ifdef malloc +#undef malloc +#endif +#ifdef calloc +#undef calloc +#endif +#ifdef realloc +#undef realloc +#endif +#ifdef free +#undef free +#endif + +#define malloc(size) _test_malloc(size, __FILE__, __LINE__) +#define realloc(ptr, size) _test_realloc(ptr, size, __FILE__, __LINE__) +#define calloc(num, size) _test_calloc(num, size, __FILE__, __LINE__) +#define free(ptr) _test_free(ptr, __FILE__, __LINE__) -- To view, visit https://review.coreboot.org/c/flashrom/+/51243 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f Gerrit-Change-Number: 51243 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-CC: Victor Ding <victording(a)chromium.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: Makefile,meson.build: Fix dependency issues with raiden_debug_spi
by Nico Huber (Code Review) April 1, 2021

April 1, 2021

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51930 ) Change subject: Makefile,meson.build: Fix dependency issues with raiden_debug_spi ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > No worries, luckily I saw the email in my uncontrollable corp inbox. […] Yes, a line like this. I couldn't find it. -- To view, visit https://review.coreboot.org/c/flashrom/+/51930 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2d6a8c33a2228abf006a9b278bcb7133765c7074 Gerrit-Change-Number: 51930 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Thu, 01 Apr 2021 10:09:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi: support a devpath option
by Angel Pons (Code Review) April 1, 2021

April 1, 2021

Attention is currently required from: Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51967 ) Change subject: lspcon_i2c_spi: support a devpath option ...................................................................... Patch Set 2: Code-Review+1 (5 comments) File lspcon_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/51967/comment/c7b42129_979dcfe0 PS1, Line 486: msg_perr("%s: one of devpath or bus must be specified\n", __func__); > Revised further so invalid bus values are distinct from unspecified, allowing the two options to be […] Looks pretty good, thanks! File lspcon_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/51967/comment/1d761af6_038e166e PS2, Line 457: __func__); nit: `__func__` probably fits on the previous line https://review.coreboot.org/c/flashrom/+/51967/comment/1ed569e8_6344b581 PS2, Line 463: __func__); nit: `__func__` most likely fits on the previous line https://review.coreboot.org/c/flashrom/+/51967/comment/595e213a_008dcb85 PS2, Line 489: nit: replace 8 spaces with a tab https://review.coreboot.org/c/flashrom/+/51967/comment/9d92aef5_92a9d149 PS2, Line 492: } else if (device_path != NULL) { The first two cases are error paths that return an error, whereas the last two cases are the "normal" paths. I wouldn't use `else` with the former, in order to differentiate both kinds of paths: if (device_path != NULL && bus_number >= 0) { msg_perr("%s: only one of bus and devpath may be specified\n", __func__); free(device_path); return SPI_GENERIC_ERROR; } if (device_path == NULL && bus_number < 0) { msg_perr("%s: one of bus and devpath must be specified\n", __func__); return SPI_GENERIC_ERROR; } if (device_path != NULL) { fd = i2c_open_path(device_path, REGISTER_ADDRESS, 0); free(device_path); } else { fd = i2c_open(bus_number, REGISTER_ADDRESS, 0); } -- To view, visit https://review.coreboot.org/c/flashrom/+/51967 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 Gerrit-Change-Number: 51967 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 01 Apr 2021 07:35:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0