[flashrom] Libflashrom draft patch - initial review
Антон Кочков
anton.kochkov at gmail.com
Fri Jun 19 14:41:39 CEST 2015
So, my patches are here
https://github.com/XVilka/flashrom/commits/libflashrom_layout - feel
free to use them as you wish, if you want so.
Best regards,
Anton Kochkov.
On Fri, Jun 19, 2015 at 2:54 PM, Антон Кочков <anton.kochkov at gmail.com> wrote:
> 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