Hello,
My name is Łukasz Dmitrowski and I am one of coreboot/flashrom GSoC students this year. You can read about my project here: http://blogs.coreboot.org/blog/author/dmitro/.
One of my first steps is updating and extending Nico Hubers libflashrom patch. Work is still in progress, but some changes are already made, so I thought that it will be good idea to send a patch just for review to know if I am going in a good direction. I want to improve my skills and provide best code quality as I can, so I am open for criticism - I am here to learn :)
Patch summary:
Makefile: I added cli_common.o and print.o to LIB_OBJS - it solves undefined references in current libflashrom version, but spoils standard flashrom build with 'make', I did not investigated it yet as my focus is to extend library with new functions and update already existing, need to discuss it with my mentor
cli_classic.c, cli_output.c, flash.h: Only Nico Hubers changes here, didn't touch it
libflashrom.c, libflashrom.h: I added 8 functions and made a few changes to existing ones, as some of used functions are static, or does not exist.
New functions:
int fl_supported_programmers(const char **supported_programmers); int fl_supported_flash_chips(fl_flashchip_info_t *fchips); int fl_supported_boards(fl_board_info_t *boards); int fl_supported_chipsets(fl_chipset_info_t *chipsets); int fl_supported_programmers_number(); int fl_supported_flash_chips_number(); int fl_supported_boards_number(); int fl_supported_chipsets_number();
New types: fl_flashchip_info_t - smaller version of struct flashchip fl_board_info_t - smaller version of struct board_info fl_chipset_info_t - smaller version of struct penable fl_test_state - copy of enum test_state
So we have 4 functions that return a list of supported programmers, chips, boards and chipsets. Next 4 functions return number of supported hardware of certain type. For example, to obtain a list of supported boards, you can create an array of structures fl_board_info of suitable size (the size is known - fl_supported_boards_number()), then pass it to fl_supported_boards(fl_board_info_t *boards) and you will have it filled with data.
Regarding changes in Nico Hubers functions: I made temporary comments for every change and marked it with LD_CHANGE_NOTE. Every comment contains previous code and reason why it has been commented out.
Thanks in advance for your insights!
Regards, Lukasz Dmitrowski
---
Signed-off-by: Łukasz Dmitrowski lukasz.dmitrowski@gmail.com
Index: Makefile =================================================================== --- Makefile (revision 1891) +++ Makefile (working copy) @@ -374,7 +374,7 @@ ############################################################################### # Library code.
-LIB_OBJS = layout.o flashrom.o udelay.o programmer.o helpers.o +LIB_OBJS = libflashrom.o layout.o flashrom.o udelay.o programmer.o helpers.o cli_common.o print.o
############################################################################### # Frontend related stuff. Index: cli_classic.c =================================================================== --- cli_classic.c (revision 1891) +++ cli_classic.c (working copy) @@ -30,6 +30,7 @@ #include "flash.h" #include "flashchips.h" #include "programmer.h" +#include "libflashrom.h"
static void cli_classic_usage(const char *name) { @@ -135,6 +136,8 @@ char *tempstr = NULL; char *pparam = NULL;
+ fl_set_log_callback((fl_log_callback_t *)&fl_print_cb); + print_version(); print_banner();
Index: cli_output.c =================================================================== --- cli_output.c (revision 1891) +++ cli_output.c (working copy) @@ -71,9 +71,8 @@ #endif /* !STANDALONE */
/* Please note that level is the verbosity, not the importance of the message. */ -int print(enum msglevel level, const char *fmt, ...) +int fl_print_cb(enum msglevel level, const char *fmt, va_list ap) { - va_list ap; int ret = 0; FILE *output_type = stdout;
@@ -81,9 +80,7 @@ output_type = stderr;
if (level <= verbose_screen) { - va_start(ap, fmt); ret = vfprintf(output_type, fmt, ap); - va_end(ap); /* msg_*spew often happens inside chip accessors in possibly * time-critical operations. Don't slow them down by flushing. */ if (level != MSG_SPEW) @@ -91,9 +88,7 @@ } #ifndef STANDALONE if ((level <= verbose_logfile) && logfile) { - va_start(ap, fmt); ret = vfprintf(logfile, fmt, ap); - va_end(ap); if (level != MSG_SPEW) fflush(logfile); } Index: flash.h =================================================================== --- flash.h (revision 1891) +++ flash.h (working copy) @@ -31,6 +31,7 @@ #include <stdint.h> #include <stddef.h> #include <stdbool.h> +#include <stdarg.h> #if IS_WINDOWS #include <windows.h> #undef min @@ -317,6 +318,7 @@ MSG_DEBUG2 = 4, MSG_SPEW = 5, }; +int fl_print_cb(enum msglevel level, const char *fmt, va_list ap); /* Let gcc and clang check for correct printf-style format strings. */ int print(enum msglevel level, const char *fmt, ...) #ifdef __MINGW32__ Index: libflashrom.c =================================================================== --- libflashrom.c (revision 0) +++ libflashrom.c (working copy) @@ -0,0 +1,603 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2012 secunet Security Networks AG + * (Written by Nico Huber nico.huber@secunet.com for secunet) + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ +/** + * @mainpage + * + * Have a look at the Modules section for a function reference. + */ + +#include <stdlib.h> +#include <string.h> +#include <stdarg.h> + +#include "flash.h" +#include "programmer.h" +#include "libflashrom.h" + +/** + * @defgroup fl-general General + * @{ + */ + +/** Pointer to log callback function. */ +static fl_log_callback_t *fl_log_callback = NULL; + +/** + * @brief Initialize libflashrom. + * + * @param perform_selfcheck If not zero, perform a self check. + * @return 0 on success + */ +int fl_init(const int perform_selfcheck) +{ + if (perform_selfcheck && selfcheck()) + return 1; + myusec_calibrate_delay(); + return 0; +} + +/** + * @brief Shut down libflashrom. + * @return 0 on success + */ +int fl_shutdown(void) +{ + return 0; /* TODO: nothing to do? */ +} + +/* TODO: fl_set_loglevel()? do we need it? + For now, let the user decide in his callback. */ + +/** + * @brief Set the log callback function. + * + * Set a callback function which will be invoked whenever libflashrom wants + * to output messages. This allows frontends to do whatever they see fit with + * such messages, e.g. write them to syslog, or to file, or print them in a + * GUI window, etc. + * + * @param log_callback Pointer to the new log callback function. + */ +void fl_set_log_callback(fl_log_callback_t *const log_callback) +{ + fl_log_callback = log_callback; +} +/** @private */ +int print(const enum msglevel level, const char *const fmt, ...) +{ + if (fl_log_callback) { + int ret; + va_list args; + va_start(args, fmt); + ret = fl_log_callback(level, fmt, args); + va_end(args); + return ret; + } + return 0; +} + +/** @} */ /* end fl-general */ + + + +/** + * @defgroup fl-query Querying + * @{ + */ + +int fl_supported_programmers(const char **supported_programmers) +{ + int ret = 0; + enum programmer p = 0; + + if (supported_programmers != NULL) { + for (; p < PROGRAMMER_INVALID; ++p) + supported_programmers[p] = programmer_table[p].name; + } else { + ret = 1; + } + + return ret; +} + +int fl_supported_programmers_number() +{ + return PROGRAMMER_INVALID; +} + +int fl_supported_flash_chips(fl_flashchip_info_t *fchips) +{ + int ret = 0; + int i = 0; + + if (fchips != NULL) { + for (; i < flashchips_size; ++i) { + fchips[i].vendor = flashchips[i].vendor; + fchips[i].name = flashchips[i].name; + fchips[i].tested.erase = flashchips[i].tested.erase; + fchips[i].tested.probe = flashchips[i].tested.probe; + fchips[i].tested.read = flashchips[i].tested.read; + fchips[i].tested.write = flashchips[i].tested.write; + fchips[i].total_size = flashchips[i].total_size; + } + } else { + ret = 1; + } + + return ret; +} + +int fl_supported_flash_chips_number() +{ + return flashchips_size; +} + +int fl_supported_boards(fl_board_info_t *boards) +{ + int ret = 0; + const struct board_info *binfo = boards_known; + + if (boards != NULL) { + while (binfo->vendor != NULL) { + boards->vendor = binfo->vendor; + boards->name = binfo->name; + boards->working = binfo->working; + ++binfo; + ++boards; + } + } else { + ret = 1; + } + + return ret; +} + +int fl_supported_boards_number() +{ + int boards_number = 0; + const struct board_info *binfo = boards_known; + + while (binfo->vendor != NULL) { + ++boards_number; + ++binfo; + } + + return boards_number; +} + +int fl_supported_chipsets(fl_chipset_info_t *chipsets) +{ + int ret = 0; + const struct penable *chipset = chipset_enables; + + if (chipsets != NULL) { + while (chipset->vendor_name != NULL) { + chipsets->vendor = chipset->vendor_name; + chipsets->chipset = chipset->device_name; + chipsets->status = chipset->status; + ++chipset; + ++chipsets; + } + } else { + return ret; + } + + return ret; +} + +int fl_supported_chipsets_number() +{ + int chipsets_number = 0; + const struct penable *chipset = chipset_enables; + + while (chipset->vendor_name != NULL) { + ++chipsets_number; + ++chipset; + } + + return chipsets_number; +} + +/** @} */ /* end fl-query */ + + + +/** + * @defgroup fl-prog Programmers + * @{ + */ + +/** + * @brief Initialize the specified programmer. + * + * @param prog_name Name of the programmer to initialize. + * @param prog_param Pointer to programmer specific parameters. + * @return 0 on success + */ +int fl_programmer_init(const char *const prog_name, const char *const prog_param) +{ + unsigned prog; + + for (prog = 0; prog < PROGRAMMER_INVALID; prog++) { + if (strcmp(prog_name, programmer_table[prog].name) == 0) + break; + } + if (prog >= PROGRAMMER_INVALID) { + msg_ginfo("Error: Unknown programmer "%s". Valid choices are:\n", prog_name); + list_programmers_linebreak(0, 80, 0); + return 1; + } + return programmer_init(prog, prog_param); +} + +/** + * @brief Shut down the initialized programmer. + * + * @return 0 on success + */ +int fl_programmer_shutdown(void) +{ + return programmer_shutdown(); +} + +/* TODO: fl_programmer_capabilities()? */ + +/** @} */ /* end fl-prog */ + + + +/** + * @defgroup fl-flash Flash chips + * @{ + */ + +/** + * @brief Probe for a flash chip. + * + * Probes for a flash chip and returns a flash context, that can be used + * later with flash chip and @ref fl-ops "image operations", if exactly one + * matching chip is found. + * + * @param[out] flashctx Points to a pointer of type fl_flashctx_t that will + * be set if exactly one chip is found. *flashctx has + * to be freed by the caller with @ref fl_flash_release. + * @param[in] chip_name Name of a chip to probe for, or NULL to probe for + * all known chips. + * @return 0 on success, + * 3 if multiple chips were found, + * 2 if no chip was found, + * or 1 on any other error. + */ +int fl_flash_probe(fl_flashctx_t **const flashctx, const char *const chip_name) +{ + int i, ret = 2; + fl_flashctx_t second_flashctx = { 0, }; + + chip_to_probe = chip_name; /* chip_to_probe is global in flashrom.c */ + + *flashctx = malloc(sizeof(**flashctx)); + if (!*flashctx) + return 1; + memset(*flashctx, 0, sizeof(**flashctx)); + + /* LD_CHANGE_NOTE for (i = 0; i < registered_programmer_count; ++i) { + * + * Reason of change: registered_programmer_count does not exist, I assume that a proper one is + * now registered_master_count - I will check and confirm + */ + for (i = 0; i < registered_master_count; ++i) { + int flash_idx = -1; + /* LD_CHANGE_NOTE if (!ret || (flash_idx = probe_flash(®istered_programmers[i], 0, *flashctx, 0)) != -1) { + * + * Reason of change: registered_programmers does not exist, I assume that a proper one is + * now registered_masters - I will check and confirm + */ + if (!ret || (flash_idx = probe_flash(®istered_masters[i], 0, *flashctx, 0)) != -1) { + ret = 0; + /* We found one chip, now check that there is no second match. */ + /* LD_CHANGE_NOTE if (probe_flash(®istered_programmers[i], flash_idx + 1, &second_flashctx, 0) != -1) { + * + * Reason of change: registered_programmers does not exist, I assume that a proper one is + * now registered_masters - I will check and confirm + * + */ + if (probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0) != -1) { + ret = 3; + break; + } + } + } + if (ret) { + free(*flashctx); + *flashctx = NULL; + } + return ret; +} + +/** + * @brief Returns the size of the specified flash chip in bytes. + * + * @param flashctx The queried flash context. + * @return Size of flash chip in bytes. + */ +size_t fl_flash_getsize(const fl_flashctx_t *const flashctx) +{ + return flashctx->chip->total_size << 10; +} + +/** @private */ +int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents); +/** @private */ +/* LD_CHANGE_NOTE void emergency_help_message(void); + * + * Reason of change: This has been commented out as emergency_help_message is static + */ +/** + * @brief Erase the specified ROM chip. + * + * @param flashctx The context of the flash chip to erase. + * @return 0 on success. + */ +int fl_flash_erase(fl_flashctx_t *const flashctx) +{ + const size_t flash_size = flashctx->chip->total_size * 1024; + + int ret = 0; + + uint8_t *const newcontents = malloc(flash_size); + if (!newcontents) { + msg_gerr("Out of memory!\n"); + return 1; + } + uint8_t *const oldcontents = malloc(flash_size); + if (!oldcontents) { + msg_gerr("Out of memory!\n"); + free(newcontents); + return 1; + } + + if (flashctx->chip->unlock) + flashctx->chip->unlock(flashctx); + + /* Assume worst case for old contents: All bits are 0. */ + memset(oldcontents, 0x00, flash_size); + /* Assume best case for new contents: All bits should be 1. */ + memset(newcontents, 0xff, flash_size); + /* Side effect of the assumptions above: Default write action is erase + * because newcontents looks like a completely erased chip, and + * oldcontents being completely 0x00 means we have to erase everything + * before we can write. + */ + + if (erase_and_write_flash(flashctx, oldcontents, newcontents)) { + /* FIXME: Do we really want the scary warning if erase failed? + * After all, after erase the chip is either blank or partially + * blank or it has the old contents. A blank chip won't boot, + * so if the user wanted erase and reboots afterwards, the user + * knows very well that booting won't work. + */ + /* LD_CHANGE_NOTE emergency_help_message(); + * + * Reason of change: This function is static + */ + ret = 1; + } + + free(oldcontents); + free(newcontents); + return ret; +} + +/** + * @brief Free a flash context. + * + * @param flashctx Flash context to free. + */ +void fl_flash_release(fl_flashctx_t *const flashctx) +{ + free(flashctx); +} + +/** @} */ /* end fl-flash */ + + + +/** + * @defgroup fl-ops Operations + * @{ + */ + +/** + * @brief Read the current image from the specified ROM chip. + * + * @param flashctx The context of the flash chip. + * @param buffer Target buffer to write image to. + * @param buffer_len Size of target buffer in bytes. + * @return 0 on success, + * 2 if buffer_len is to short for the flash chip's contents, + * or 1 on any other failure. + */ +int fl_image_read(fl_flashctx_t *const flashctx, void *const buffer, const size_t buffer_len) +{ + const size_t flash_size = flashctx->chip->total_size * 1024; + + int ret = 0; + + if (flashctx->chip->unlock) + flashctx->chip->unlock(flashctx); + + msg_cinfo("Reading flash... "); + if (flash_size > buffer_len) { + msg_cerr("Buffer to short for this flash chip (%u < %u).\n", + (unsigned int)buffer_len, (unsigned int)flash_size); + ret = 2; + goto _out; + } + if (!flashctx->chip->read) { + msg_cerr("No read function available for this flash chip.\n"); + ret = 1; + goto _out; + } + if (flashctx->chip->read(flashctx, buffer, 0, flash_size)) { + msg_cerr("Read operation failed!\n"); + ret = 1; + goto _out; + } +_out: + msg_cinfo("%s.\n", ret ? "FAILED" : "done"); + return ret; +} + +/** @private */ +/* LD_CHANGE_NOTE void nonfatal_help_message(void); + * + * Reason of change: This function is static + */ +/** + * @brief Write the specified image to the ROM chip. + * + * @param flashctx The context of the flash chip. + * @param buffer Source buffer to read image from. + * @param buffer_len Size of source buffer in bytes. + * @return 0 on success, + * 4 if buffer_len doesn't match the size of the flash chip, + * 3 if write was tried, but nothing has changed, + * 2 if write was tried, but flash contents changed, + * or 1 on any other failure. + */ +int fl_image_write(fl_flashctx_t *const flashctx, void *const buffer, const size_t buffer_len) +{ + const size_t flash_size = flashctx->chip->total_size * 1024; + + int ret = 0; + + if (buffer_len != flash_size) { + msg_cerr("Buffer size doesn't match size of flash chip (%u != %u)\n.", + (unsigned int)buffer_len, (unsigned int)flash_size); + return 4; + } + + uint8_t *const newcontents = buffer; + uint8_t *const oldcontents = malloc(flash_size); + if (!oldcontents) { + msg_gerr("Out of memory!\n"); + return 1; + } + if (fl_image_read(flashctx, oldcontents, flash_size)) { + ret = 1; + goto _free_out; + } + + /* LD_CHANGE_NOTE handle_romentries(flashctx, oldcontents, newcontents); + * + * Reason of change: This function does not exist. There is a need to investigate what was + * its purpose and how it can be replaced. + */ + + if (erase_and_write_flash(flashctx, oldcontents, newcontents)) { + msg_cerr("Uh oh. Erase/write failed. Checking if anything changed.\n"); + if (!flashctx->chip->read(flashctx, newcontents, 0, flash_size)) { + if (!memcmp(oldcontents, newcontents, flash_size)) { + msg_cinfo("Good. It seems nothing was changed.\n"); + /* LD_CHANGE_NOTE nonfatal_help_message(); + * + * Reason of change: This function is static + */ + ret = 3; + goto _free_out; + } + } + /* LD_CHANGE_NOTE emergency_help_message(); + * + * Reason of change: This function is static + */ + ret = 2; + goto _free_out; + } + +_free_out: + free(oldcontents); + return ret; +} + +/** @private */ +/* LD_CHANGE_NOTE int compare_range(const uint8_t *wantbuf, const uint8_t *havebuf, unsigned int start, unsigned int len); + * + * Reason of change: This function is static + */ +/** + * @brief Verify the ROM chip's contents with the specified image. + * + * @param flashctx The context of the flash chip. + * @param buffer Source buffer to verify with. + * @param buffer_len Size of source buffer in bytes. + * @return 0 on success, + * 2 if buffer_len doesn't match the size of the flash chip, + * or 1 on any other failure. + */ +int fl_image_verify(fl_flashctx_t *const flashctx, void *const buffer, const size_t buffer_len) +{ + const size_t flash_size = flashctx->chip->total_size * 1024; + + int ret = 0; + + if (buffer_len != flash_size) { + msg_cerr("Buffer size doesn't match size of flash chip (%u != %u)\n.", + (unsigned int)buffer_len, (unsigned int)flash_size); + return 2; + } + + /* LD_CHANGE_NOTE uint8_t *const newcontents = buffer; - used only in handle_romentries() function + * + * Reason of change: This pointer is used only in handle_romentries function, which has been + * commented out + */ + uint8_t *const oldcontents = malloc(flash_size); + if (!oldcontents) { + msg_gerr("Out of memory!\n"); + return 1; + } + if (fl_image_read(flashctx, oldcontents, flash_size)) { + ret = 1; + goto _free_out; + } + + /* LD_CHANGE_NOTE handle_romentries(flashctx, oldcontents, newcontents); + * + * Reason of change: This function does not exist + */ + + msg_cinfo("Verifying flash... "); + + /* LD_CHANGE_NOTE ret = compare_range(newcontents, oldcontents, 0, flash_size); + * + * Reason of change: This function is static. For now replaced with ret = 1 as ret must be used + */ + ret = 1; + if (!ret) + msg_cinfo("VERIFIED.\n"); + +_free_out: + free(oldcontents); + return ret; +} + +/** @} */ /* end fl-ops */ Index: libflashrom.h =================================================================== --- libflashrom.h (revision 0) +++ libflashrom.h (working copy) @@ -0,0 +1,97 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2012 secunet Security Networks AG + * (Written by Nico Huber nico.huber@secunet.com for secunet) + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef __LIBFLASHROM_H__ +#define __LIBFLASHROM_H__ 1 + +#include <stdarg.h> + +int fl_init(int perform_selfcheck); +int fl_shutdown(void); +/** @ingroup fl-general */ +typedef enum { /* This has to match enum msglevel. */ + FL_MSG_ERROR = 0, + FL_MSG_INFO = 1, + FL_MSG_DEBUG = 2, + FL_MSG_DEBUG2 = 3, + FL_MSG_SPEW = 4, +} fl_log_level_t; +/** @ingroup fl-general */ +typedef int(fl_log_callback_t)(fl_log_level_t, const char *format, va_list); +void fl_set_log_callback(fl_log_callback_t *); + +typedef enum { + FL_TESTED_OK = 0, + FL_TESTED_NT = 1, + FL_TESTED_BAD = 2, + FL_TESTED_DEP = 3, + FL_TESTED_NA = 4, +} fl_test_state; + +typedef struct { + const char *vendor; + const char *name; + unsigned int total_size; + + struct fl_tested { + fl_test_state probe; + fl_test_state read; + fl_test_state erase; + fl_test_state write; + } tested; +} fl_flashchip_info_t; + +typedef struct { + const char *vendor; + const char *name; + fl_test_state working; +} fl_board_info_t; + +typedef struct { + const char *vendor; + const char *chipset; + fl_test_state status; +} fl_chipset_info_t; + +int fl_supported_programmers(const char **supported_programmers); +int fl_supported_flash_chips(fl_flashchip_info_t *fchips); +int fl_supported_boards(fl_board_info_t *boards); +int fl_supported_chipsets(fl_chipset_info_t *chipsets); +int fl_supported_programmers_number(); +int fl_supported_flash_chips_number(); +int fl_supported_boards_number(); +int fl_supported_chipsets_number(); + +int fl_programmer_init(const char *prog_name, const char *prog_params); +int fl_programmer_shutdown(void); + +struct flashctx; +typedef struct flashctx fl_flashctx_t; +int fl_flash_probe(fl_flashctx_t **, const char *chip_name); +size_t fl_flash_getsize(const fl_flashctx_t *); +int fl_flash_erase(fl_flashctx_t *); +void fl_flash_release(fl_flashctx_t *); + +int fl_image_read(fl_flashctx_t *, void *buffer, size_t buffer_len); +int fl_image_write(fl_flashctx_t *, void *buffer, size_t buffer_len); +int fl_image_verify(fl_flashctx_t *, void *buffer, size_t buffer_len); + +#endif /* !__LIBFLASHROM_H__ */
-
Stefan Tauner -
Urja Rannikko -
Łukasz Dmitrowski