[flashrom] Libflashrom draft patch - initial review
Антон Кочков
anton.kochkov at gmail.com
Fri Jun 19 13:54:39 CEST 2015
Sorry again, that was old tree, I will rearrange patches and will push
updated version in a hour or so.
Best regards,
Anton Kochkov.
On Fri, Jun 19, 2015 at 2:50 PM, Антон Кочков <anton.kochkov at gmail.com> wrote:
> Uh, forgot to add, that loading layout will be quite useful too, in the API and
> adding romentry_t* argument for all fl_image_* functions.
> Best regards,
> Anton Kochkov.
>
>
> On Fri, Jun 19, 2015 at 2:43 PM, Антон Кочков <anton.kochkov at gmail.com> wrote:
>> Hi!
>> I've done also some rebasing of the libflashrom patches before (on top
>> of the layout branch from the stefanct)
>> https://github.com/XVilka/flashrom/commits/libflashrom
>>
>> May be you'll find something useful here.
>> Best regards,
>> Anton Kochkov.
>>
>>
>> On Thu, Jun 18, 2015 at 7:45 AM, Urja Rannikko <urjaman at gmail.com> wrote:
>>> Hi,
>>>
>>>
>>> On Fri, Jun 12, 2015 at 12:26 AM, Łukasz Dmitrowski
>>> <lukasz.dmitrowski at gmail.com> wrote:
>>>> 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 :)
>>>>
>>> I'm here essentially trying out reviewing stuff, so expect more from
>>> somebody a bit more experienced, but I heard my comments would be
>>> useful, so moving on :)
>>>
>>>> Patch summary:
>>>>
>>> First thing i have to say about the patch is that it has been
>>> malformed, most likely line wrapped by your email client. If you cant
>>> properly include the patch inline, attaching a .patch would be okay.
>>> (Also turn off that HTML output if you can... although it wasnt used
>>> for anything, but still.). So I didnt do any compile testing on this
>>> stuff because of this.
>>>
>>>> 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
>>> I might comment on any part of the patch because I havent reviewed the
>>> original and it's only about the code.
>>>
>>>>
>>>> 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.
>>> My opinion is that the _number functions make the API unnecessarily
>>> complex to use.
>>> I'm suggesting like this:
>>> fl_flashchip_info_t * fl_supported_flash_chips(void);
>>> Have the call allocate the table using a not specified allocator
>>> (really just malloc for now, but we're not telling), and freed with a
>>> function like
>>> int fl_support_info_free(void*p);
>>> This would allow implementation using const tables or allocation,
>>> whatever we like.
>>> (if (p == const_table) return 0; in our _free() if mixed.)
>>>
>>>>
>>>> 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 at 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 at 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? */
>>> I'm pretty sure there is, but need to investigate more.
>>>
>>>> +}
>>>> +
>>>> +/* 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;
>>> These names are confusing (just FYI), i first thought we're counting
>>> chipsets here..
>>>> + }
>>>> + } 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.
>>>> + */
>>> Need a way to output multiple flash chips from here, it shouldnt be an
>>> error, just a
>>> "pick from these" dialog and continue with "OK", or something like that.
>>> Maybe add int *chip_count as a parameter and the set pointer will
>>> point to an array of flashctx with the int set to the count of found
>>> chips.
>>> (Hopefully I'm not saying totally obvious stuff here...)
>>>
>>>> +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
>>> Maybe have some other method to indicate that the UI should display an
>>> emergency help message, maybe ret = 2. Same comment for the _write
>>> version, but that already has ret values for these.
>>>
>>>> + */
>>>> +/**
>>>> + * @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
>>> For any of these "this is static"... then make it not static or figure
>>> out a cleaner way of doing it, but yes i get this is an early patch.
>>>
>>>> + */
>>>> +/**
>>>> + * @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 at 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. */
>>> Maybe you could just bring enum msglevel here from flash.h...
>>> or have some other header for this printing stuff thats
>>> used by both libflashrom.h and other parts of flashrom.
>>>
>>>> + 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__ */
>>>>
>>>> _______________________________________________
>>>> flashrom mailing list
>>>> flashrom at flashrom.org
>>>> http://www.flashrom.org/mailman/listinfo/flashrom
>>>
>>> --
>>> Urja Rannikko
>>>
>>> _______________________________________________
>>> flashrom mailing list
>>> flashrom at flashrom.org
>>> http://www.flashrom.org/mailman/listinfo/flashrom
More information about the flashrom
mailing list