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

Change in flashrom[master]: dediprog.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 31 Aug '21

31 Aug '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/56414 to look at the new patch set (#3). Change subject: dediprog.c: Refactor singleton states into reentrant pattern ...................................................................... dediprog.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". BUG=b:185191942 TEST=builds and ninja test Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dediprog.c 1 file changed, 159 insertions(+), 114 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/56414/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/56414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d Gerrit-Change-Number: 56414 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> 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]: tests: Add NOT_ZERO and NOT_NULL macros to the header file
by Anastasia Klimchuk (Code Review) 31 Aug '21

31 Aug '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/57269 ) Change subject: tests: Add NOT_ZERO and NOT_NULL macros to the header file ...................................................................... tests: Add NOT_ZERO and NOT_NULL macros to the header file This patch adds two macros into io_mock.h, so that they can be used anywhere in tests. Common usage is to indicate a valid pointer, where it doesn't matter what the pointer is, only matters it is not null. Another case is to indicate a valid file descriptor, where it only matters the descriptor is not zero integer. New macros replace existing MOCK_HANDLE. This patch corrects return value from __wrap_ioctl to be successful by default. It used to be MOCK_HANDLE, but should be 0. BUG=b:181803212 TEST=builds and ninja test Change-Id: I5ad6ee4aa9091447c6c9108c92bf7f6e755fca48 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/init_shutdown.c M tests/io_mock.h M tests/tests.c 3 files changed, 11 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/69/57269/1 diff --git a/tests/init_shutdown.c b/tests/init_shutdown.c index 3236b22..d1e99cb 100644 --- a/tests/init_shutdown.c +++ b/tests/init_shutdown.c @@ -19,8 +19,6 @@ #include "io_mock.h" #include "programmer.h" -#define NOT_NULL ((void *)0xf000baaa) - static void run_lifecycle(void **state, const struct programmer_entry *prog, const char *param) { (void) state; /* unused */ diff --git a/tests/io_mock.h b/tests/io_mock.h index adb5f3b..18eaac8 100644 --- a/tests/io_mock.h +++ b/tests/io_mock.h @@ -34,6 +34,9 @@ /* Required for `FILE *` */ #include <stdio.h> +#define NOT_ZERO (0xf000baaa) +#define NOT_NULL ((void *)NOT_ZERO) + /* Define libusb symbols to avoid dependency on libusb.h */ struct libusb_device_handle; typedef struct libusb_device_handle libusb_device_handle; diff --git a/tests/tests.c b/tests/tests.c index 4965fe1..cd50bdf 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -23,7 +23,6 @@ /* redefinitions/wrapping */ #define LOG_ME printf("%s is called\n", __func__) -#define MOCK_HANDLE 2021 static const struct io_mock *current_io = NULL; @@ -54,7 +53,7 @@ } struct pci_dev mock_pci_dev = { - .device_id = MOCK_HANDLE, + .device_id = NOT_ZERO, }; struct pci_dev *__wrap_pcidev_init(void *devs, int bar) @@ -66,7 +65,7 @@ uintptr_t __wrap_pcidev_readbar(void *dev, int bar) { LOG_ME; - return MOCK_HANDLE; + return NOT_ZERO; } void __wrap_sio_write(uint16_t port, uint8_t reg, uint8_t data) @@ -85,7 +84,7 @@ LOG_ME; if (current_io && current_io->open) return current_io->open(current_io->state, pathname, flags); - return MOCK_HANDLE; + return NOT_ZERO; } int __wrap_open64(const char *pathname, int flags) @@ -93,7 +92,7 @@ LOG_ME; if (current_io && current_io->open) return current_io->open(current_io->state, pathname, flags); - return MOCK_HANDLE; + return NOT_ZERO; } int __wrap_ioctl(int fd, unsigned long int request, ...) @@ -107,7 +106,7 @@ va_end(args); return out; } - return MOCK_HANDLE; + return 0; } int __wrap_write(int fd, const void *buf, size_t sz) @@ -131,7 +130,7 @@ LOG_ME; if (current_io && current_io->fopen) return current_io->fopen(current_io->state, pathname, mode); - return (void *)MOCK_HANDLE; + return NOT_NULL; } FILE *__wrap_fopen64(const char *pathname, const char *mode) @@ -139,7 +138,7 @@ LOG_ME; if (current_io && current_io->fopen) return current_io->fopen(current_io->state, pathname, mode); - return (void *)MOCK_HANDLE; + return NOT_NULL; } int __wrap_stat(const char *path, void *buf) @@ -256,7 +255,7 @@ libusb_context *usb_ctx, uint16_t vid, uint16_t pid, unsigned int num) { LOG_ME; - return (void *)MOCK_HANDLE; + return NOT_NULL; } int __wrap_libusb_set_configuration(libusb_device_handle *devh, int config) -- To view, visit https://review.coreboot.org/c/flashrom/+/57269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ad6ee4aa9091447c6c9108c92bf7f6e755fca48 Gerrit-Change-Number: 57269 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: layout/fmap: reimplement for endian independence
by Thomas Heijligen (Code Review) 30 Aug '21

30 Aug '21

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/57265 ) Change subject: layout/fmap: reimplement for endian independence ...................................................................... layout/fmap: reimplement for endian independence Change-Id: I6d38649327d2021a9917d1294f1541376ed6717d Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de> --- M fmap.c M layout.h M libflashrom.c 3 files changed, 177 insertions(+), 375 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/57265/1 diff --git a/fmap.c b/fmap.c index b18cbf7..903ffc2 100644 --- a/fmap.c +++ b/fmap.c @@ -1,331 +1,158 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + /* - * Copyright 2010, Google LLC. - * Copyright 2018-present, Facebook Inc. - * All rights reserved. + * This file is part of flashrom project. For the full license text see LICENSE.txt. * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * - * Alternatively, this software may be distributed under the terms of the - * GNU General Public License ("GPL") version 2 as published by the Free - * Software Foundation. + * Copyright (C) 2021, secunet Secure Networks AG, + * written by Thomas Heijligen <thomas.heijligen(a)secunet.com> */ -#include <ctype.h> -#include <stdlib.h> +/* + * This file contais functions to find and read layout information from a fmap structure. + */ + +#include <stddef.h> +#include <stdint.h> +#include <stdio.h> #include <string.h> -#include <sys/types.h> -#include "flash.h" -#include "fmap.h" +#include "layout.h" +#include "libflashrom.h" +#include "hwaccess.h" -static size_t fmap_size(const struct fmap *fmap) +#define FMAP_SIGNATURE "__FMAP__" +#define FMAP_NAME_SIZE 32 +#define FMAP_VERSION_MAJOR 1 +#define FMAP_VERSION_MINOR 1 + +#define FMAP_HEADER_SIZE 56 +#define FMAP_HEADER_OFFSET_SIGNATURE 0 +#define FMAP_HEADER_OFFSET_VER_MAJOR 8 +#define FMAP_HEADER_OFFSET_VER_MINOR 9 +#define FMAP_HEADER_OFFSET_BASE 10 +#define FMAP_HEADER_OFFSET_SIZE 18 +#define FMAP_HEADER_OFFSET_NAME 22 +#define FMAP_HEADER_OFFSET_NAREAS 54 + +#define FMAP_AREA_SIZE 42 +#define FMAP_AREA_OFFSET_OFFSET 0 +#define FMAP_AERA_OFFSET_SIZE 4 +#define FMAP_AREA_OFFSET_NAME 8 +#define FMAP_AREA_OFFSET_FLAGS 40 + +/** + * @brief find the signature of a FMAP in a buffer, uses linear search + * @param[in] buffer + * @param[in] buffer_size + * @param[out] fmap_offset, on SUCCESS the offset where to find the fmap, if NULL or FAILURE don't change + * @return 0 SUCCESS + * 1 UNEXPECTED_NULL_POINTER + * 2 NOT_FOUND - FMAP signature not found + */ +int find_fmap_in_buffer(const void *const buffer, const uint64_t buffer_size, uint64_t *const fmap_offset) { - return sizeof(*fmap) + (fmap->nareas * sizeof(struct fmap_area)); -} + /* test for unexpected NULL pointer */ + if (buffer == NULL) { + return 1; /* UNEXPECTED_NULL_POINTER */ + } -static int is_valid_fmap(const struct fmap *fmap) -{ - if (memcmp(fmap, FMAP_SIGNATURE, strlen(FMAP_SIGNATURE)) != 0) - return 0; - /* strings containing the magic tend to fail here */ - if (fmap->ver_major > FMAP_VER_MAJOR) - return 0; - if (fmap->ver_minor > FMAP_VER_MINOR) - return 0; - /* a basic consistency check: flash address space size should be larger - * than the size of the fmap data structure */ - if (fmap->size < fmap_size(fmap)) - return 0; - - /* fmap-alikes along binary data tend to fail on having a valid, - * null-terminated string in the name field.*/ - int i; - for (i = 0; i < FMAP_STRLEN; i++) { - if (fmap->name[i] == 0) - break; - if (!isgraph(fmap->name[i])) - return 0; - if (i == FMAP_STRLEN - 1) { - /* name is specified to be null terminated single-word string - * without spaces. We did not break in the 0 test, we know it - * is a printable spaceless string but we're seeing FMAP_STRLEN - * symbols, which is one too many. - */ - return 0; + for (uint64_t offset = 0; (offset + FMAP_HEADER_SIZE) < buffer_size; offset++) { + if (memcmp(buffer + offset + FMAP_HEADER_OFFSET_SIGNATURE, FMAP_SIGNATURE, strlen(FMAP_SIGNATURE)) == 0) { + if (fmap_offset != NULL) { + *fmap_offset = offset; + } + return 0; /* SUCCESS */ } } - return 1; - + return 2; /* NOT_FOUND */ } /** - * @brief Do a brute-force linear search for fmap in provided buffer + * TODO: implement * - * @param[in] buffer The buffer to search - * @param[in] len Length (in bytes) to search - * - * @return offset in buffer where fmap is found if successful - * -1 to indicate that fmap was not found - * -2 to indicate fmap is truncated or exceeds buffer + len + * @brief find the signature of a FMAP on the flash chip + * @param[in] flashctx + * @param[out] fmap_offset on SUCCESS offset of the FMAP, else not changed + * @param[out] fmap_size on SUCCESS size of the FMAP, else not changed + * @return 0 SUCESS + * 2 NOT_FOUND - FMAP signature not found */ -static off_t fmap_lsearch(const uint8_t *buf, size_t len) +int find_fmap_in_rom(const struct flashrom_flashctx *const flashctx, uint64_t *const fmap_offset, uint64_t *const fmap_size) { - off_t offset; - bool fmap_found = 0; + return 2; +} - for (offset = 0; offset <= (off_t)(len - sizeof(struct fmap)); offset++) { - if (is_valid_fmap((struct fmap *)&buf[offset])) { - fmap_found = 1; - break; + +/** + * @brief read out the layout information from a FMAP in buffer + * @param[out] layout + * @param[in] buffer + * @param[in] buffer_size + * @param[in] fmap_offset + * @return 0 SUCCESS + * 1 UNEXPECTED_NULL_POINTER + * 3 BUFFER_TO_SMALL + * 4 INVALID_SIGNATURE + * 5 INVALID_VERSION + * 6 MEMORY_ALLOCATION_FAILED + */ +int read_layout_from_fmap(struct flashrom_layout **layout, const void *const buffer, const uint64_t buffer_size, const uint64_t fmap_offset) +{ + /* buffer must not be NULL */ + if (buffer == NULL) { + return 1; /* FLASHROM_UNEXPECTED_NULL_POINTER */ + } + /* buffer_size must be larger than fmap_offset + FMAP_HEADER_SIZE */ + if (buffer_size < fmap_offset + FMAP_HEADER_SIZE) { + return 3; /* FLASHROM_BUFFER_TO_SMALL */ + } + /* fmap signature check */ + if (memcmp(buffer + fmap_offset + FMAP_HEADER_OFFSET_SIGNATURE, FMAP_SIGNATURE, strlen(FMAP_SIGNATURE)) != 0) { + return 4; /* FLASHROM_INVALID_SIGNATURE */ + } + /* fmap version check */ + if ((read_le8(buffer + fmap_offset + FMAP_HEADER_OFFSET_VER_MAJOR) != FMAP_VERSION_MAJOR) && + (read_le8(buffer + fmap_offset + FMAP_HEADER_OFFSET_VER_MINOR) != FMAP_VERSION_MINOR)) { + return 5; /* FLASHROM_INVALID_VERSION */ + } + + const void* const fmap = buffer + fmap_offset; + + struct flashrom_layout *l = NULL; + if (flashrom_layout_new(&l) != 0) { + return 6; /* FLASHROM_MEMORY_ALLOCATION_FAILED */ + } + uint64_t firmware_base = read_le64(fmap + FMAP_HEADER_OFFSET_BASE); + /* uint32_t firmware_size = read_le32(fmap + FMAP_HEADER_OFFSET_SIZE); TODO: mode checks */ + char firmware_name[FMAP_NAME_SIZE + 1] = {'\0'}; + memcpy(firmware_name, fmap + FMAP_HEADER_OFFSET_NAME, FMAP_NAME_SIZE); + uint16_t firmware_nareas = read_le16(fmap + FMAP_HEADER_OFFSET_NAREAS); + + /* check if buffer includes all fmap areas */ + if (buffer_size < (fmap_offset + FMAP_HEADER_SIZE + (firmware_nareas * FMAP_AREA_SIZE))) { + return 3; /* FLASHROM_BUFFER_TO_SMALL */ + } + + for (unsigned int i = 0; i < firmware_nareas; i++) { + const void* const region = fmap + FMAP_HEADER_SIZE + (i * FMAP_AREA_SIZE); + + uint32_t area_offset = read_le32(region + FMAP_AREA_OFFSET_OFFSET); + uint32_t area_size = read_le32(region + FMAP_AERA_OFFSET_SIZE); + char area_name[FMAP_NAME_SIZE + 1] = {'\0'}; + char combined_name[(2 * FMAP_NAME_SIZE) + 2]; + memcpy(area_name, region + FMAP_AREA_OFFSET_NAME, FMAP_NAME_SIZE); + snprintf(combined_name, sizeof(combined_name), "%s_%s", firmware_name, area_name); + + int ret = flashrom_layout_add_region(l, + firmware_base + area_offset, + firmware_base + area_offset + area_size, + combined_name ); + + if (ret != 0) { + flashrom_layout_release(l); + return 6; /* FLASHROM_MEMORY_ALLOCATION_FAILED */ } } - if (!fmap_found) - return -1; - - if (offset + fmap_size((struct fmap *)&buf[offset]) > len) { - msg_gerr("fmap size exceeds buffer boundary.\n"); - return -2; - } - - return offset; -} - -/** - * @brief Read fmap from provided buffer and copy it to fmap_out - * - * @param[out] fmap_out Double-pointer to location to store fmap contents. - * Caller must free allocated fmap contents. - * @param[in] buf Buffer to search - * @param[in] len Length (in bytes) to search - * - * @return 0 if successful - * 1 to indicate error - * 2 to indicate fmap is not found - */ -int fmap_read_from_buffer(struct fmap **fmap_out, const uint8_t *const buf, size_t len) -{ - off_t offset = fmap_lsearch(buf, len); - if (offset < 0) { - msg_gdbg("Unable to find fmap in provided buffer.\n"); - return 2; - } - msg_gdbg("Found fmap at offset 0x%06zx\n", (size_t)offset); - - const struct fmap *fmap = (const struct fmap *)(buf + offset); - *fmap_out = malloc(fmap_size(fmap)); - if (*fmap_out == NULL) { - msg_gerr("Out of memory.\n"); - return 1; - } - - memcpy(*fmap_out, fmap, fmap_size(fmap)); - return 0; -} - -static int fmap_lsearch_rom(struct fmap **fmap_out, - struct flashctx *const flashctx, size_t rom_offset, size_t len) -{ - int ret = -1; - uint8_t *buf; - - if (prepare_flash_access(flashctx, true, false, false, false)) - goto _finalize_ret; - - /* likely more memory than we need, but it simplifies handling and - * printing offsets to keep them uniform with what's on the ROM */ - buf = malloc(rom_offset + len); - if (!buf) { - msg_gerr("Out of memory.\n"); - goto _finalize_ret; - } - - ret = flashctx->chip->read(flashctx, buf + rom_offset, rom_offset, len); - if (ret) { - msg_pdbg("Cannot read ROM contents.\n"); - goto _free_ret; - } - - ret = fmap_read_from_buffer(fmap_out, buf + rom_offset, len); -_free_ret: - free(buf); -_finalize_ret: - finalize_flash_access(flashctx); - return ret; -} - -static int fmap_bsearch_rom(struct fmap **fmap_out, struct flashctx *const flashctx, - size_t rom_offset, size_t len, size_t min_stride) -{ - size_t stride, fmap_len = 0; - int ret = 1, fmap_found = 0, check_offset_0 = 1; - struct fmap *fmap; - const unsigned int chip_size = flashctx->chip->total_size * 1024; - const int sig_len = strlen(FMAP_SIGNATURE); - - if (rom_offset + len > flashctx->chip->total_size * 1024) - return 1; - - if (len < sizeof(*fmap)) - return 1; - - if (prepare_flash_access(flashctx, true, false, false, false)) - return 1; - - fmap = malloc(sizeof(*fmap)); - if (!fmap) { - msg_gerr("Out of memory.\n"); - goto _free_ret; - } - - /* - * For efficient operation, we start with the largest stride possible - * and then decrease the stride on each iteration. Also, check for a - * remainder when modding the offset with the previous stride. This - * makes it so that each offset is only checked once. - * - * Zero (rom_offset == 0) is a special case and is handled using a - * variable to track whether or not we've checked it. - */ - size_t offset; - for (stride = chip_size / 2; stride >= min_stride; stride /= 2) { - if (stride > len) - continue; - - for (offset = rom_offset; - offset <= rom_offset + len - sizeof(struct fmap); - offset += stride) { - if ((offset % (stride * 2) == 0) && (offset != 0)) - continue; - if (offset == 0 && !check_offset_0) - continue; - check_offset_0 = 0; - - /* Read errors are considered non-fatal since we may - * encounter locked regions and want to continue. */ - if (flashctx->chip->read(flashctx, (uint8_t *)fmap, offset, sig_len)) { - /* - * Print in verbose mode only to avoid excessive - * messages for benign errors. Subsequent error - * prints should be done as usual. - */ - msg_cdbg("Cannot read %d bytes at offset %zu\n", sig_len, offset); - continue; - } - - if (memcmp(fmap, FMAP_SIGNATURE, sig_len) != 0) - continue; - - if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sig_len, - offset + sig_len, sizeof(*fmap) - sig_len)) { - msg_cerr("Cannot read %zu bytes at offset %06zx\n", - sizeof(*fmap) - sig_len, offset + sig_len); - continue; - } - - if (is_valid_fmap(fmap)) { - msg_gdbg("fmap found at offset 0x%06zx\n", offset); - fmap_found = 1; - break; - } - msg_gerr("fmap signature found at %zu but header is invalid.\n", offset); - ret = 2; - } - - if (fmap_found) - break; - } - - if (!fmap_found) - goto _free_ret; - - fmap_len = fmap_size(fmap); - struct fmap *tmp = fmap; - fmap = realloc(fmap, fmap_len); - if (!fmap) { - msg_gerr("Failed to realloc.\n"); - free(tmp); - goto _free_ret; - } - - if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap), - offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) { - msg_cerr("Cannot read %zu bytes at offset %06zx\n", - fmap_len - sizeof(*fmap), offset + sizeof(*fmap)); - /* Treat read failure to be fatal since this - * should be a valid, usable fmap. */ - ret = 2; - goto _free_ret; - } - - *fmap_out = fmap; - ret = 0; -_free_ret: - if (ret) - free(fmap); - finalize_flash_access(flashctx); - return ret; -} - -/** - * @brief Read fmap from ROM - * - * @param[out] fmap_out Double-pointer to location to store fmap contents. - * Caller must free allocated fmap contents. - * @param[in] flashctx Flash context - * @param[in] rom_offset Offset in ROM to begin search - * @param[in] len Length to search relative to rom_offset - * - * @return 0 on success, - * 2 if the fmap couldn't be read, - * 1 on any other error. - */ -int fmap_read_from_rom(struct fmap **fmap_out, - struct flashctx *const flashctx, size_t rom_offset, size_t len) -{ - int ret; - - if (!flashctx || !flashctx->chip) - return 1; - - /* - * Binary search is used at first to see if we can find an fmap quickly - * in a usual location (often at a power-of-2 offset). However, once we - * reach a small enough stride the transaction overhead will reverse the - * speed benefit of using bsearch at which point we need to use brute- - * force instead. - * - * TODO: Since flashrom is often used with high-latency external - * programmers we should not be overly aggressive with bsearch. - */ - ret = fmap_bsearch_rom(fmap_out, flashctx, rom_offset, len, 256); - if (ret) { - msg_gdbg("Binary search failed, trying linear search...\n"); - ret = fmap_lsearch_rom(fmap_out, flashctx, rom_offset, len); - } - - return ret; -} + *layout = l; + return 0; /* FLASHROM_SUCCESS */ +} \ No newline at end of file diff --git a/layout.h b/layout.h index 713241f..6e084ae 100644 --- a/layout.h +++ b/layout.h @@ -68,4 +68,8 @@ void prepare_layout_for_extraction(struct flashrom_flashctx *); int layout_sanity_checks(const struct flashrom_flashctx *); +int find_fmap_in_buffer(const void *const buffer, const uint64_t buffer_size, uint64_t *const fmap_offset); +int find_fmap_in_rom(const struct flashrom_flashctx *const flashctx, uint64_t *const fmap_offset, uint64_t *const fmap_size); +int read_layout_from_fmap(struct flashrom_layout **layout, const void *const buffer, const uint64_t buffer_size, const uint64_t fmap_offset); + #endif /* !__LAYOUT_H__ */ diff --git a/libflashrom.c b/libflashrom.c index 1ecf650..ae8cd21 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -495,31 +495,6 @@ #endif } -#ifdef __FLASHROM_LITTLE_ENDIAN__ -static int flashrom_layout_parse_fmap(struct flashrom_layout **layout, - struct flashctx *const flashctx, const struct fmap *const fmap) -{ - int i; - char name[FMAP_STRLEN + 1]; - const struct fmap_area *area; - struct flashrom_layout *l; - - if (!fmap || flashrom_layout_new(&l)) - return 1; - - for (i = 0, area = fmap->areas; i < fmap->nareas; i++, area++) { - snprintf(name, sizeof(name), "%s", area->name); - if (flashrom_layout_add_region(l, area->offset, area->offset + area->size - 1, name)) { - flashrom_layout_release(l); - return 1; - } - } - - *layout = l; - return 0; -} -#endif /* __FLASHROM_LITTLE_ENDIAN__ */ - /** * @brief Read a layout by searching the flash chip for fmap. * @@ -537,27 +512,35 @@ int flashrom_layout_read_fmap_from_rom(struct flashrom_layout **const layout, struct flashctx *const flashctx, off_t offset, size_t len) { -#ifndef __FLASHROM_LITTLE_ENDIAN__ - return 3; -#else - struct fmap *fmap = NULL; - int ret = 0; + (void)(offset); // TODO: remove unused parameter - msg_gdbg("Attempting to read fmap from ROM content.\n"); - if (fmap_read_from_rom(&fmap, flashctx, offset, len)) { - msg_gerr("Failed to read fmap from ROM.\n"); - return 1; + int ret; + uint64_t fmap_offset, fmap_size; + + if(flashctx == NULL || flashctx->chip == NULL || flashctx->chip->read == NULL) { + return 2; // UNEXPECTED_NULL_POINTER } - msg_gdbg("Adding fmap layout to global layout.\n"); - if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { - msg_gerr("Failed to add fmap regions to layout.\n"); - ret = 1; + ret = find_fmap_in_rom(flashctx, &fmap_offset, &fmap_size); + if (ret != 0) { + return ret; } + void *fmap_buffer = malloc(fmap_size); + if (fmap_buffer == NULL) { + return 1; // MEMORY_ALLOCATION_FAILED + } + // prepare flash access? + // use flashrom_image_read? + ret = flashctx->chip->read(flashctx, fmap_buffer, fmap_offset, fmap_size); + if (ret != 0) { + free(fmap_buffer); + return ret; + } + ret = read_layout_from_fmap(layout, fmap_buffer, fmap_size, 0); + free(fmap_buffer); - free(fmap); + // TODO add layout to flashctx? return ret; -#endif } /** @@ -577,33 +560,21 @@ int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **const layout, struct flashctx *const flashctx, const uint8_t *const buf, size_t size) { -#ifndef __FLASHROM_LITTLE_ENDIAN__ - return 3; -#else - struct fmap *fmap = NULL; - int ret = 1; + (void)(flashctx); // Unused + int ret; + uint64_t fmap_offset; - if (!buf || !size) - goto _ret; - - msg_gdbg("Attempting to read fmap from buffer.\n"); - if (fmap_read_from_buffer(&fmap, buf, size)) { - msg_gerr("Failed to read fmap from buffer.\n"); - goto _ret; + ret = find_fmap_in_buffer(buf, size, &fmap_offset); + if (ret != 0) { + return ret; + } + ret = read_layout_from_fmap(layout, buf, size, fmap_offset); + if( ret != 0) { + return ret; } - msg_gdbg("Adding fmap layout to global layout.\n"); - if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { - msg_gerr("Failed to add fmap regions to layout.\n"); - goto _free_ret; - } - - ret = 0; -_free_ret: - free(fmap); -_ret: - return ret; -#endif + // TODO add layout to flashctx? + return 0; } /** -- To view, visit https://review.coreboot.org/c/flashrom/+/57265 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6d38649327d2021a9917d1294f1541376ed6717d Gerrit-Change-Number: 57265 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Edward O'Callaghan (Code Review) 30 Aug '21

30 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56413 ) Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... tests: Mock file i/o for linux_mtd and linux_spi tests This patch adds an init-shutdown test for linux_mtd. Since linux_mtd is using file i/o operations, those are added to the framework and mocked. Another driver linux_spi which is also using file i/o, got an upgrade in this patch, and it is now reading max buffer size from sysfs (using mocked file i/o). A good side-effect is that linux_mtd is the first test for opaque masters, which is great to have in preparation for a change like CB:56103 but for opaque masters. BUG=b:181803212 TEST=builds and ninja test Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/56413 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M tests/init_shutdown.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 180 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve Edward O'Callaghan: Looks good to me, approved diff --git a/tests/init_shutdown.c b/tests/init_shutdown.c index d1d0c7f..3236b22 100644 --- a/tests/init_shutdown.c +++ b/tests/init_shutdown.c @@ -19,6 +19,8 @@ #include "io_mock.h" #include "programmer.h" +#define NOT_NULL ((void *)0xf000baaa) + static void run_lifecycle(void **state, const struct programmer_entry *prog, const char *param) { (void) state; /* unused */ @@ -195,16 +197,107 @@ #endif } +struct linux_mtd_io_state { + char *fopen_path; +}; + +FILE *linux_mtd_fopen(void *state, const char *pathname, const char *mode) +{ + struct linux_mtd_io_state *io_state = state; + + io_state->fopen_path = strdup(pathname); + + return NOT_NULL; +} + +size_t linux_mtd_fread(void *state, void *buf, size_t size, size_t len, FILE *fp) +{ + struct linux_mtd_fread_mock_entry { + const char *path; + const char *data; + }; + const struct linux_mtd_fread_mock_entry fread_mock_map[] = { + { "/sys/class/mtd/mtd0//type", "nor" }, + { "/sys/class/mtd/mtd0//name", "Device" }, + { "/sys/class/mtd/mtd0//flags", "" }, + { "/sys/class/mtd/mtd0//size", "1024" }, + { "/sys/class/mtd/mtd0//erasesize", "512" }, + { "/sys/class/mtd/mtd0//numeraseregions", "0" }, + }; + + struct linux_mtd_io_state *io_state = state; + unsigned int i; + + if (!io_state->fopen_path) + return 0; + + for (i = 0; i < ARRAY_SIZE(fread_mock_map); i++) { + const struct linux_mtd_fread_mock_entry *entry = &fread_mock_map[i]; + + if (!strcmp(io_state->fopen_path, entry->path)) { + size_t data_len = min(size * len, strlen(entry->data)); + memcpy(buf, entry->data, data_len); + return data_len; + } + } + + return 0; +} + +int linux_mtd_fclose(void *state, FILE *fp) +{ + struct linux_mtd_io_state *io_state = state; + + free(io_state->fopen_path); + + return 0; +} + +void linux_mtd_init_and_shutdown_test_success(void **state) +{ +#if CONFIG_LINUX_MTD == 1 + struct linux_mtd_io_state linux_mtd_io_state = { NULL }; + const struct io_mock linux_mtd_io = { + .state = &linux_mtd_io_state, + .fopen = linux_mtd_fopen, + .fread = linux_mtd_fread, + .fclose = linux_mtd_fclose, + }; + + io_mock_register(&linux_mtd_io); + + run_lifecycle(state, &programmer_linux_mtd, ""); + + io_mock_register(NULL); +#else + skip(); +#endif +} + +char *linux_spi_fgets(void *state, char *buf, int len, FILE *fp) +{ + /* Emulate reading max buffer size from sysfs. */ + const char *max_buf_size = "1048576"; + + return memcpy(buf, max_buf_size, min(len, strlen(max_buf_size) + 1)); +} + void linux_spi_init_and_shutdown_test_success(void **state) { /* * Current implementation tests a particular path of the init procedure. - * There are two ways for it to succeed: reading the buffer size from sysfs - * and the fallback to getpagesize(). This test does the latter (fallback to - * getpagesize). + * Specifically, it is reading the buffer size from sysfs. */ #if CONFIG_LINUX_SPI == 1 + const struct io_mock linux_spi_io = { + .fgets = linux_spi_fgets, + }; + + io_mock_register(&linux_spi_io); + run_lifecycle(state, &programmer_linux_spi, "dev=/dev/null"); + + io_mock_register(NULL); #else skip(); #endif diff --git a/tests/io_mock.h b/tests/io_mock.h index c69cd38..adb5f3b 100644 --- a/tests/io_mock.h +++ b/tests/io_mock.h @@ -31,6 +31,9 @@ #ifndef _IO_MOCK_H_ #define _IO_MOCK_H_ +/* Required for `FILE *` */ +#include <stdio.h> + /* Define libusb symbols to avoid dependency on libusb.h */ struct libusb_device_handle; typedef struct libusb_device_handle libusb_device_handle; @@ -79,6 +82,12 @@ int (*ioctl)(void *state, int fd, unsigned long request, va_list args); int (*read)(void *state, int fd, void *buf, size_t sz); int (*write)(void *state, int fd, const void *buf, size_t sz); + + /* Standard I/O */ + FILE* (*fopen)(void *state, const char *pathname, const char *mode); + char* (*fgets)(void *state, char *buf, int len, FILE *fp); + size_t (*fread)(void *state, void *buf, size_t size, size_t len, FILE *fp); + int (*fclose)(void *state, FILE *fp); }; void io_mock_register(const struct io_mock *io); diff --git a/tests/meson.build b/tests/meson.build index 63fec5a..53885a8 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -39,6 +39,15 @@ '-Wl,--wrap=write', '-Wl,--wrap=fopen', '-Wl,--wrap=fopen64', + '-Wl,--wrap=stat', + '-Wl,--wrap=stat64', + '-Wl,--wrap=fread', + '-Wl,--wrap=fgets', + '-Wl,--wrap=fclose', + '-Wl,--wrap=feof', + '-Wl,--wrap=ferror', + '-Wl,--wrap=clearerr', + '-Wl,--wrap=setvbuf', '-Wl,--wrap=rget_io_perms', '-Wl,--wrap=test_outb', '-Wl,--wrap=test_inb', diff --git a/tests/tests.c b/tests/tests.c index bfc0e53..4965fe1 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -129,15 +129,78 @@ FILE *__wrap_fopen(const char *pathname, const char *mode) { LOG_ME; - return NULL; + if (current_io && current_io->fopen) + return current_io->fopen(current_io->state, pathname, mode); + return (void *)MOCK_HANDLE; } FILE *__wrap_fopen64(const char *pathname, const char *mode) { LOG_ME; + if (current_io && current_io->fopen) + return current_io->fopen(current_io->state, pathname, mode); + return (void *)MOCK_HANDLE; +} + +int __wrap_stat(const char *path, void *buf) +{ + LOG_ME; + return 0; +} + +int __wrap_stat64(const char *path, void *buf) +{ + LOG_ME; + return 0; +} + +char *__wrap_fgets(char *buf, int len, FILE *fp) +{ + LOG_ME; + if (current_io && current_io->fgets) + return current_io->fgets(current_io->state, buf, len, fp); return NULL; } +size_t __wrap_fread(void *ptr, size_t size, size_t len, FILE *fp) +{ + LOG_ME; + if (current_io && current_io->fread) + return current_io->fread(current_io->state, ptr, size, len, fp); + return 0; +} + +int __wrap_setvbuf(FILE *fp, char *buf, int type, size_t size) +{ + LOG_ME; + return 0; +} + +int __wrap_fclose(FILE *fp) +{ + LOG_ME; + if (current_io && current_io->fclose) + return current_io->fclose(current_io->state, fp); + return 0; +} + +int __wrap_feof(FILE *fp) +{ + /* LOG_ME; */ + return 0; +} + +int __wrap_ferror(FILE *fp) +{ + /* LOG_ME; */ + return 0; +} +void __wrap_clearerr(FILE *fp) +{ + /* LOG_ME; */ + return; +} + int __wrap_rget_io_perms(void) { LOG_ME; @@ -277,6 +340,7 @@ cmocka_unit_test(nicrealtek_init_and_shutdown_test_success), cmocka_unit_test(dediprog_init_and_shutdown_test_success), cmocka_unit_test(ene_lpc_init_and_shutdown_test_success), + cmocka_unit_test(linux_mtd_init_and_shutdown_test_success), cmocka_unit_test(linux_spi_init_and_shutdown_test_success), cmocka_unit_test(realtek_mst_init_and_shutdown_test_success), }; diff --git a/tests/tests.h b/tests/tests.h index 512b72d..df4a41c 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -46,6 +46,7 @@ void nicrealtek_init_and_shutdown_test_success(void **state); void dediprog_init_and_shutdown_test_success(void **state); void ene_lpc_init_and_shutdown_test_success(void **state); +void linux_mtd_init_and_shutdown_test_success(void **state); void linux_spi_init_and_shutdown_test_success(void **state); void realtek_mst_init_and_shutdown_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 12 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: par_master: Fix propagation of register_par_master() return values
by Edward O'Callaghan (Code Review) 30 Aug '21

30 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57192 ) Change subject: par_master: Fix propagation of register_par_master() return values ...................................................................... par_master: Fix propagation of register_par_master() return values This patch checks return value of register_par_master() so that in case of an error this error is not ignored anymore. BUG=b:185191942 TEST=builds and ninja test Change-Id: I377afae41708c7433a56615e2f096bce9c5349f1 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57192 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 atahpt.c M atapromise.c M atavia.c M drkaiser.c M gfxnvidia.c M it8212.c M nicintel.c M nicnatsemi.c M satamv.c M satasii.c 10 files changed, 10 insertions(+), 28 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/atahpt.c b/atahpt.c index 0da39e0..c627e0b 100644 --- a/atahpt.c +++ b/atahpt.c @@ -85,9 +85,7 @@ reg32 |= (1 << 24); rpci_write_long(dev, REG_FLASH_ACCESS, reg32); - register_par_master(&par_master_atahpt, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_atahpt, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_atahpt = { diff --git a/atapromise.c b/atapromise.c index 171b22f..e3009e4 100644 --- a/atapromise.c +++ b/atapromise.c @@ -152,14 +152,13 @@ } max_rom_decode.parallel = rom_size; - register_par_master(&par_master_atapromise, BUS_PARALLEL, NULL); msg_pwarn("Do not use this device as a generic programmer. It will leave anything outside\n" "the first %zu kB of the flash chip in an undefined state. It works fine for the\n" "purpose of updating the firmware of this device (padding may necessary).\n", rom_size / 1024); - return 0; + return register_par_master(&par_master_atapromise, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_atapromise = { diff --git a/atavia.c b/atavia.c index f8b9738..b110d54 100644 --- a/atavia.c +++ b/atavia.c @@ -184,9 +184,7 @@ return 1; } - register_par_master(&lpc_master_atavia, BUS_LPC, NULL); - - return 0; + return register_par_master(&lpc_master_atavia, BUS_LPC, NULL); } const struct programmer_entry programmer_atavia = { diff --git a/drkaiser.c b/drkaiser.c index 111bb97..27c15fb 100644 --- a/drkaiser.c +++ b/drkaiser.c @@ -85,9 +85,7 @@ return 1; max_rom_decode.parallel = 128 * 1024; - register_par_master(&par_master_drkaiser, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_drkaiser, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_drkaiser = { diff --git a/gfxnvidia.c b/gfxnvidia.c index 468fc6c..760b76b 100644 --- a/gfxnvidia.c +++ b/gfxnvidia.c @@ -111,9 +111,7 @@ /* Write/erase doesn't work. */ programmer_may_write = 0; - register_par_master(&par_master_gfxnvidia, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_gfxnvidia, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_gfxnvidia = { diff --git a/it8212.c b/it8212.c index 620a15c..e47f6f8 100644 --- a/it8212.c +++ b/it8212.c @@ -75,8 +75,7 @@ rpci_write_long(dev, PCI_ROM_ADDRESS, io_base_addr | 0x01); max_rom_decode.parallel = IT8212_MEMMAP_SIZE; - register_par_master(&par_master_it8212, BUS_PARALLEL, NULL); - return 0; + return register_par_master(&par_master_it8212, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_it8212 = { .name = "it8212", diff --git a/nicintel.c b/nicintel.c index ada615e..c5b9012 100644 --- a/nicintel.c +++ b/nicintel.c @@ -107,9 +107,7 @@ pci_rmmio_writew(0x0001, nicintel_control_bar + CSR_FCR); max_rom_decode.parallel = NICINTEL_MEMMAP_SIZE; - register_par_master(&par_master_nicintel, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_nicintel, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_nicintel = { diff --git a/nicnatsemi.c b/nicnatsemi.c index 17e05c8..b2f381a 100644 --- a/nicnatsemi.c +++ b/nicnatsemi.c @@ -97,9 +97,7 @@ * functions below wants to be 0x0000FFFF. */ max_rom_decode.parallel = 131072; - register_par_master(&par_master_nicnatsemi, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_nicnatsemi, BUS_PARALLEL, NULL); } diff --git a/satamv.c b/satamv.c index 3996a4b..32ac11a 100644 --- a/satamv.c +++ b/satamv.c @@ -179,9 +179,7 @@ /* 512 kByte with two 8-bit latches, and * 4 MByte with additional 3-bit latch. */ max_rom_decode.parallel = 4 * 1024 * 1024; - register_par_master(&par_master_satamv, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_satamv, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_satamv = { diff --git a/satasii.c b/satasii.c index fe7ac15..321ef7f 100644 --- a/satasii.c +++ b/satasii.c @@ -129,9 +129,7 @@ if ((id != 0x0680) && (!(pci_mmio_readl(sii_bar) & (1 << 26)))) msg_pwarn("Warning: Flash seems unconnected.\n"); - register_par_master(&par_master_satasii, BUS_PARALLEL, NULL); - - return 0; + return register_par_master(&par_master_satasii, BUS_PARALLEL, NULL); } const struct programmer_entry programmer_satasii = { .name = "satasii", -- To view, visit https://review.coreboot.org/c/flashrom/+/57192 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I377afae41708c7433a56615e2f096bce9c5349f1 Gerrit-Change-Number: 57192 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: par_master: Use new API to register shutdown function
by Edward O'Callaghan (Code Review) 30 Aug '21

30 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57156 ) Change subject: par_master: Use new API to register shutdown function ...................................................................... par_master: Use new API to register shutdown function This allows par masters to register shutdown function in par_master struct, which means there is no need to call register_shutdown in init function, since this call is now a part of register_par_master. As a consequence of using new API, this patch also fixes propagation of register_par_master() return values. BUG=b:185191942 TEST=builds and ninja test Change-Id: Ief7be907f53878b4b6567b52889735e5fff64ead Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57156 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M nic3com.c M nicrealtek.c 2 files changed, 4 insertions(+), 15 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/nic3com.c b/nic3com.c index dd3d0cb..2af0a52 100644 --- a/nic3com.c +++ b/nic3com.c @@ -99,6 +99,7 @@ .chip_writew = fallback_chip_writew, .chip_writel = fallback_chip_writel, .chip_writen = fallback_chip_writen, + .shutdown = nic3com_shutdown, }; static int nic3com_init(void) @@ -150,13 +151,7 @@ max_rom_decode.parallel = 128 * 1024; - if (register_shutdown(nic3com_shutdown, data)) { - free(data); - goto init_err_cleanup_exit; - } - register_par_master(&par_master_nic3com, BUS_PARALLEL, data); - - return 0; + return register_par_master(&par_master_nic3com, BUS_PARALLEL, data); init_err_cleanup_exit: /* 3COM 3C90xB cards need a special fixup. */ diff --git a/nicrealtek.c b/nicrealtek.c index b8125f4..ec821b4 100644 --- a/nicrealtek.c +++ b/nicrealtek.c @@ -95,6 +95,7 @@ .chip_writew = fallback_chip_writew, .chip_writel = fallback_chip_writel, .chip_writen = fallback_chip_writen, + .shutdown = nicrealtek_shutdown, }; static int nicrealtek_init(void) @@ -138,14 +139,7 @@ data->bios_rom_addr = bios_rom_addr; data->bios_rom_data = bios_rom_data; - if (register_shutdown(nicrealtek_shutdown, data)) { - free(data); - return 1; - } - - register_par_master(&par_master_nicrealtek, BUS_PARALLEL, data); - - return 0; + return register_par_master(&par_master_nicrealtek, BUS_PARALLEL, data); } const struct programmer_entry programmer_nicrealtek = { -- To view, visit https://review.coreboot.org/c/flashrom/+/57156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ief7be907f53878b4b6567b52889735e5fff64ead Gerrit-Change-Number: 57156 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: par_master: Move shutdown function above par_master struct
by Edward O'Callaghan (Code Review) 30 Aug '21

30 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57155 ) Change subject: par_master: Move shutdown function above par_master struct ...................................................................... par_master: Move shutdown function above par_master struct This patch prepares par masters to use new API which allows to register shutdown function in par_master struct. See also later patch in this chain, where par masters are converted to new API. BUG=b:185191942 TEST=builds and ninja test Comparing flashrom binary before and after the patch, make clean && make CONFIG_EVERYTHING=yes VERSION=none binary is the same Change-Id: I87e9ce0ad9b39b39645dc24cb0d75d1e7a6d9047 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57155 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M nic3com.c M nicrealtek.c 2 files changed, 18 insertions(+), 18 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/nic3com.c b/nic3com.c index 14108f0..dd3d0cb 100644 --- a/nic3com.c +++ b/nic3com.c @@ -73,17 +73,6 @@ return INB(data->io_base_addr + BIOS_ROM_DATA); } -static const struct par_master par_master_nic3com = { - .chip_readb = nic3com_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, - .chip_writeb = nic3com_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, -}; - static int nic3com_shutdown(void *par_data) { struct nic3com_data *data = par_data; @@ -101,6 +90,17 @@ return 0; } +static const struct par_master par_master_nic3com = { + .chip_readb = nic3com_chip_readb, + .chip_readw = fallback_chip_readw, + .chip_readl = fallback_chip_readl, + .chip_readn = fallback_chip_readn, + .chip_writeb = nic3com_chip_writeb, + .chip_writew = fallback_chip_writew, + .chip_writel = fallback_chip_writel, + .chip_writen = fallback_chip_writen, +}; + static int nic3com_init(void) { struct pci_dev *dev = NULL; diff --git a/nicrealtek.c b/nicrealtek.c index 81d305b..b8125f4 100644 --- a/nicrealtek.c +++ b/nicrealtek.c @@ -79,6 +79,13 @@ return val; } +static int nicrealtek_shutdown(void *data) +{ + /* FIXME: We forgot to disable software access again. */ + free(data); + return 0; +} + static const struct par_master par_master_nicrealtek = { .chip_readb = nicrealtek_chip_readb, .chip_readw = fallback_chip_readw, @@ -90,13 +97,6 @@ .chip_writen = fallback_chip_writen, }; -static int nicrealtek_shutdown(void *data) -{ - /* FIXME: We forgot to disable software access again. */ - free(data); - return 0; -} - static int nicrealtek_init(void) { struct pci_dev *dev = NULL; -- To view, visit https://review.coreboot.org/c/flashrom/+/57155 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I87e9ce0ad9b39b39645dc24cb0d75d1e7a6d9047 Gerrit-Change-Number: 57155 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: par_master: Add shutdown function in par_master struct
by Edward O'Callaghan (Code Review) 30 Aug '21

30 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57154 ) Change subject: par_master: Add shutdown function in par_master struct ...................................................................... par_master: Add shutdown function in par_master struct With this, register_par_master can take care of register_shutdown as well, and every par master only needs to call register_par_master instead of calling both register_par_master and register_shutdown. Next patches in the chain convert par masters to use new API. BUG=b:185191942 TEST=builds and ninja test Change-Id: I0fee15d548cdd16678e551eeb351e659812ddf76 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57154 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M programmer.c M programmer.h 2 files changed, 8 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/programmer.c b/programmer.c index c276a69..9a67157 100644 --- a/programmer.c +++ b/programmer.c @@ -86,6 +86,13 @@ { struct registered_master rmst = {0}; + if (mst->shutdown) { + if (register_shutdown(mst->shutdown, data)) { + mst->shutdown(data); /* cleanup */ + return 1; + } + } + if (!mst->chip_writeb || !mst->chip_writew || !mst->chip_writel || !mst->chip_writen || !mst->chip_readb || !mst->chip_readw || !mst->chip_readl || !mst->chip_readn) { diff --git a/programmer.h b/programmer.h index 3de87e5..982343a 100644 --- a/programmer.h +++ b/programmer.h @@ -466,6 +466,7 @@ uint16_t (*chip_readw) (const struct flashctx *flash, const chipaddr addr); uint32_t (*chip_readl) (const struct flashctx *flash, const chipaddr addr); void (*chip_readn) (const struct flashctx *flash, uint8_t *buf, const chipaddr addr, size_t len); + int (*shutdown)(void *data); void *data; }; int register_par_master(const struct par_master *mst, const enum chipbustype buses, void *data); -- To view, visit https://review.coreboot.org/c/flashrom/+/57154 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0fee15d548cdd16678e551eeb351e659812ddf76 Gerrit-Change-Number: 57154 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: par_master: Use new API to register shutdown function
by Nico Huber (Code Review) 27 Aug '21

27 Aug '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57156 ) Change subject: par_master: Use new API to register shutdown function ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/57156/comment/520f56fe_5fc71295 PS1, Line 17: not ignored anymore. > You are right, I agree! I initially did "the same thing as before" but actually par masters are diff […] Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/57156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ief7be907f53878b4b6567b52889735e5fff64ead Gerrit-Change-Number: 57156 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 27 Aug 2021 16:35:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: par_master: Fix propagation of register_par_master() return values
by Nico Huber (Code Review) 27 Aug '21

27 Aug '21

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57192 ) Change subject: par_master: Fix propagation of register_par_master() return values ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/57192 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I377afae41708c7433a56615e2f096bce9c5349f1 Gerrit-Change-Number: 57192 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 27 Aug 2021 16:33:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit August 2021