[Patch] Add coarse-grained IPC locking mechanism to Flashrom

[David Hendricks]

Hi,
I have attached a patch which adds a locking mechanism to Flashrom to
prevent multiple instances from running simultaneously. It may need some
modification to fit different Flashrom use cases better, but I think it's a
good start.

The size of the patch is somewhat misleading -- The only real changes to
Flashrom's current code are some of the exit points in cli_classic() and
some added stuff in the Makefile. Everything else is contained in new files.
The new files are:
csem.{c,h}: Low-level code that interfaces with semctl(), semget(), etc.
ipc_lock.{c,h}: Wrapper for csem stuff.
locks.h: A listing of locks that Flashrom cares about.
big_lock.{c,h}: An even higher-level wrapper around ipc_lock stuff, useful
for simple, coarse-grained locking.
util/use_big_lock.sh: Used by the Makefile to test for POSIX.1-2001
compliance.

The advantages of this approach are:
- The semaphore can be shared easily with other processes.
- The semaphore is cleaned automagically in case of abnormal program
termination.
- Semaphore info can be viewed and managed easily using standard utilities
like ipcs.
- On Linux at least, a second instance of Flashrom will automatically begin
immediately after the first instance is done. No busy waiting, no excessive
delays.
- We avoid errata with other locking mechanisms (there are known issues with
flock(), for example).

Disadvantages:
- The current patch is very coarse-grained, which could be problematic for
people who wish to flash multiple chips simultaneously.
- Requires >= POSIX.1-2001 compliance.

Please note that some files in this patch are BSD-licensed. Much of it is
based off code which was open-sourced by Sun Microsystems, after acquiring
it from Cobalt Networks.

Signed-off-by: David Hendricks

[Patch] Add coarse-grained IPC locking mechanism to Flashrom

[Mathias Krause]

On 01.02.2011 22:37, David Hendricks wrote:
> Hi,
> I have attached a patch which adds a locking mechanism to Flashrom to
> prevent multiple instances from running simultaneously. It may need some
> modification to fit different Flashrom use cases better, but I think
> it's a good start.

Nice.
> The size of the patch is somewhat misleading -- The only real changes to
> Flashrom's current code are some of the exit points in cli_classic() and
> some added stuff in the Makefile. Everything else is contained in new
> files. The new files are:
> csem.{c,h}: Low-level code that interfaces with semctl(), semget(), etc.
> ipc_lock.{c,h}: Wrapper for csem stuff.
> locks.h: A listing of locks that Flashrom cares about.
> big_lock.{c,h}: An even higher-level wrapper around ipc_lock stuff,
> useful for simple, coarse-grained locking.

Well, quite some wrapping wrappers :/
> util/use_big_lock.sh: Used by the Makefile to test for POSIX.1-2001
> compliance.

That test should be integrated into the Makefile, no?
> The advantages of this approach are:
> The semaphore can be shared easily with other processes.
> The semaphore is cleaned automagically in case of abnormal program
>
> termination.
> Semaphore info can be viewed and managed easily using standard
>
> utilities like ipcs.
> On Linux at least, a second instance of Flashrom will automatically
>
> begin immediately after the first instance is done. No busy waiting, no
> excessive delays.
> We avoid errata with other locking mechanisms (there are known issues
>
> with flock(), for example).
>
> Disadvantages:
> The current patch is very coarse-grained, which could be problematic
>
> for people who wish to flash multiple chips simultaneously.
> Requires >= POSIX.1-2001 compliance.
>
> Please note that some files in this patch are BSD-licensed. Much of it
> is based off code which was open-sourced by Sun Microsystems, after
> acquiring it from Cobalt Networks.
>
> Signed-off-by: David Hendricks <[e-mail]
> >
>
> flashrom mailing list
> [e-mail]
> http://www.flashrom.org/mailman/listinfo/flashrom
> Index: big_lock.c
>
> --- big_lock.c	(revision 0)
> +++ big_lock.c	(revision 0)
> @@ -0,0 +1,34 @@
> +/*
> Copyright (C) 2010 Google Inc.
> 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 Street, Fifth Floor, Boston, MA 02110-1301, USA
> */
>
> +#include "big_lock.h"
> +#include "locks.h"
>
> +#include "ipc_lock.h"
>
> +static struct ipc_lock big_lock = IPC_LOCK_INIT(BIGLOCK);
>
> +int acquire_big_lock(int timeout_secs)
> +{
> return acquire_lock(&big_lock, timeout_secs * 1000);
>
> +}
>
> +int release_big_lock(void)
> +{
> return release_lock(&big_lock);
>
> +}
> Index: big_lock.h
> ===================================================================
> --- big_lock.h	(revision 0)
> +++ big_lock.h	(revision 0)
> @@ -0,0 +1,39 @@
> +/*
> Copyright (C) 2010 Google Inc.
> 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 Street, Fifth Floor, Boston, MA 02110-1301, USA
> */
>
> +#ifndef BIG_LOCK_H__
> +#define BIG_LOCK_H__
>
> +/*
> acquire_big_lock  -  acquire global lock
> returns 0 to indicate lock acquired
> returns >0 to indicate lock was already held
> returns <0 to indicate failed to acquire lock
> */
>
> +extern int acquire_big_lock(int timeout_secs);
>
> +/*
> release_big_lock  -  release global lock
> returns 0 if lock was released successfully
> returns -1 if lock had not been held before the call
> */
>
> +extern int release_big_lock(void);
>
> +#endif /* BIG_LOCK_H__ */
> Index: Makefile
> ===================================================================
> --- Makefile	(revision 1257)
> +++ Makefile	(working copy)
> @@ -88,6 +88,12 @@
>
> LIB_OBJS = layout.o
>
> +LOCK_OBJS = csem.o ipc_lock.o big_lock.o
> +ifeq ($(shell ./util/use_big_lock.sh), 0)
> +LIB_OBJS += $(LOCK_OBJS)
> +FEATURE_CFLAGS += -D'USE_BIG_LOCK=1'
> +endif
>
> CLI_OBJS = flashrom.o cli_classic.o cli_output.o print.o
>
> PROGRAMMER_OBJS = udelay.o programmer.o
> Index: cli_classic.c
> ===================================================================
> --- cli_classic.c	(revision 1257)
> +++ cli_classic.c	(working copy)
> @@ -27,10 +27,13 @@
>  #include <string.h>
>  #include <stdlib.h>
>  #include <getopt.h>
> +#include "big_lock.h"
>  #include "flash.h"
>  #include "flashchips.h"
>  #include "programmer.h"
>
> +#define LOCK_TIMEOUT_SECS       30
>
> static void cli_classic_usage(const char *name)
>  {
>    printf("Usage: flashrom [-n] [-V] [-f] [-h|-R|-L|"
> @@ -112,7 +115,7 @@
>    int list_supported_wiki = 0;
>  #endif
>    int operation_specified = 0;
> int i;
>
> int i, rc = 0;
>
> static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
> static const struct option long_options[] = {
>
> @@ -351,12 +354,24 @@
>    	flash = NULL;
>    }
>
> +#if USE_BIG_LOCK == 1
> /* get lock before doing any work that touches hardware */
> msg_gdbg("Acquiring lock (timeout=%d sec)...\n", LOCK_TIMEOUT_SECS);
> if (acquire_big_lock(LOCK_TIMEOUT_SECS) < 0) {
> msg_gerr("Could not acquire lock.\n");
> rc = 1;
> goto cli_exit;

How about changing that goto into an exit(1) and using an atexit(2)
handler for cleanup? That would make all the gotos below unnecessary.
E.g.:
if (acquire_big_lock(LOCK_TIMEOUT_SECS) < 0) {
    msg_gerr(...);
    exit(1);
}
atexit(release_big_lock);
> }
> msg_gdbg("Lock acquired.\n");
>
> +#endif
>
> /* FIXME: Delay calibration should happen in programmer code. */
>  myusec_calibrate_delay();
>
> if (programmer_init(pparam)) {
>   fprintf(stderr, "Error: Programmer initialization failed.\n");
> exit(1);
> rc = 1;
> goto cli_exit;

here
> }
>
> /* FIXME: Delay calibration should happen in programmer code. */
> @@ -374,7 +389,8 @@
>    		printf(" %s", flashes[i]->name);
>    	printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
>    	programmer_shutdown();
> exit(1);
> rc = 1;
> goto cli_exit;

here
> } else if (!flashes[0]) {
>    	printf("No EEPROM/flash device found.\n");
>    	if (!force || !chip_to_probe) {
> @@ -393,7 +409,8 @@
>    	}
>    	// FIXME: flash writes stay enabled!
>    	programmer_shutdown();
> exit(1);
> rc = 1;
> goto cli_exit;

here
> }
>
> flash = flashes[0];
> @@ -406,21 +423,24 @@
>    	fprintf(stderr, "Chip is too big for this programmer "
>    		"(-V gives details). Use --force to override.\n");
>    	programmer_shutdown();
> return 1;
> rc = 1;
> goto cli_exit;

here
> }
>
> if (!(read_it | write_it | verify_it | erase_it)) {
>    	printf("No operations were specified.\n");
>    	// FIXME: flash writes stay enabled!
>    	programmer_shutdown();
> exit(0);
> rc = 0;
> goto cli_exit;

here
> }
>
> if (!filename && !erase_it) {
>    	printf("Error: No filename specified.\n");
>    	// FIXME: flash writes stay enabled!
>    	programmer_shutdown();
> exit(1);
> rc = 1;
> goto cli_exit;

here
> }
>
> /* Always verify write operations unless -n is used. */
> @@ -432,5 +452,12 @@
>     * Give the chip time to settle.
>     */
>    programmer_delay(100000);
> return doit(flash, force, filename, read_it, write_it, erase_it, verify_it);
> rc = doit(flash, force, filename, read_it, write_it, erase_it, verify_it);
>
> +cli_exit:
> +#if USE_BIG_LOCK == 1
> release_big_lock();
>
> +#endif
> return rc;

The atexit() handler will handle this job so this can be skipped, too.
But we might want to keep this so we release the lock early. This is
safe because release_big_lock() can be called multiple times without harm.
> }
> Index: locks.h
> ===================================================================
> --- locks.h	(revision 0)
> +++ locks.h	(revision 0)
> @@ -0,0 +1,34 @@
> +/*
> Copyright (C) 2010 Google Inc.
> 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 Street, Fifth Floor, Boston, MA 02110-1301, USA
> locks.h: locks are used to preserve atomicity of operations.
> */
>
> +#ifndef LOCKS_H__
> +#define LOCKS_H__
>
> +/* this is the base key, since we have to pick something global */
> +#define IPC_LOCK_KEY	(0x67736c00 & 0xfffffc00) /* 22 bits "gsl" */
>
> +/* The ordering of the following keys matters a lot. We don't want to reorder
> keys and have a new binary dependent on deployed/used because it will break
> atomicity of existing users and binaries. In other words, DO NOT REORDER. */
>
> +/* this is the "big lock". */
> +#define BIGLOCK		(IPC_LOCK_KEY + 0)
>
> +#endif /* LOCKS_H__ */
> Index: util/use_big_lock.sh
> ===================================================================
> --- util/use_big_lock.sh	(revision 0)
> +++ util/use_big_lock.sh	(revision 0)
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2010 Google Inc.
> +# Written by David Hendricks for Google Inc.
> +#
> +# 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
> +#
> +# This script tests the availability of semctl() and similar functions by
> +# checking that _POSIX_C_SOURCE indicates POSIX.1-2001 (or greaater) compliance.
>
> +if [ -z $CC ]; then

Quote $CC and maybe use test for portability?
> CC=${CROSS_COMPILE}gcc

That should be cc instead of gcc, again for portability.
> +fi

If this test would be in the Makefile itself $(CC) would be valid
without further checks. :/
> +echo "
> +#include <stdlib.h>
> +int main()\
> +{
> +#ifdef _POSIX_C_SOURCE
> if ((long)_POSIX_C_SOURCE >= 200112)
>        exit(EXIT_SUCCESS);
>
> +#endif
>    exit (EXIT_FAILURE);
>
> +}
> +" > .test.c
>
> +${CC} -o .test .test.c && ./.test || exit 1
> +rc=$?
> +echo $rc
>
> +rm -f .test
> +exit $rc
>
> Property changes on: util/use_big_lock.sh
> ___________________________________________________________________
> Added: svn:executable
> Index: ipc_lock.c
>
> --- ipc_lock.c	(revision 0)
> +++ ipc_lock.c	(revision 0)
> @@ -0,0 +1,94 @@
> +/*
> Copyright (C) 2010 Google Inc.
> 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 Street, Fifth Floor, Boston, MA 02110-1301, USA
> */
>
> +#include <inttypes.h>
> +#include <time.h>
>
> +#include "csem.h"
> +#include "flash.h"
> +#include "ipc_lock.h"
>
> +static int lock_init(struct ipc_lock *lock)
> +{
> if (lock->sem < 0) {
> /* get or create the semaphore, init to 1 if needed */
> int sem = csem_get_or_create(lock->key, 1);
> if (sem < 0) {
> 	return -1;
> }
> lock->sem = sem;
> }
> return 0;
>
> +}
>
> +static void msecs_to_timespec(int msecs, struct timespec *tmspec)
> +{
> tmspec->tv_sec = msecs / 1000;
> tmspec->tv_nsec = (msecs % 1000) * 1000 * 1000;
>
> +}
>
> +int acquire_lock(struct ipc_lock *lock, int timeout_msecs)
> +{
> int ret;
> struct timespec timeout;
> struct timespec *timeout_ptr;
> /* initialize the lock */
> if (lock_init(lock) < 0) {
> msg_gdbg("%s(): failed to init lock 0x%08x\n",
>          __func__, (uint32_t)lock->key);
> return -1;
> }
> /* check if it is already held */
> if (lock->is_held) {
> return 1;
> }
> /* calculate the timeout */
> if (timeout_msecs >= 0) {
> timeout_ptr = &timeout;
> msecs_to_timespec(timeout_msecs, timeout_ptr);
> } else {
> timeout_ptr = NULL;
> }
> /* try to get the lock */
> ret = csem_down_timeout_undo(lock->sem, timeout_ptr);
> if (ret < 0) {
> msg_gdbg("%s(): failed to acquire lock 0x%08x",
>          __func__, (uint32_t)lock->key);
> return -1;
> }
> /* success */
> lock->is_held = 1;
> return 0;
>
> +}
>
> +int release_lock(struct ipc_lock *lock)
> +{
> if (lock->is_held) {
> lock->is_held = 0;
> csem_up_undo(lock->sem);
> /* NOTE: do not destroy the semaphore, we want it to persist */
> return 0;
> }
>    /* did not hold the lock */
>    return -1;
>
> +}
> Index: csem.c
> ===================================================================
> --- csem.c	(revision 0)
> +++ csem.c	(revision 0)
> @@ -0,0 +1,279 @@
> +/*
> Copyright 2003 Sun Microsystems, Inc.
> Copyright 2010 Google, Inc.
> 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.
> Developer's note: This was open sourced by Sun Microsystems, which got it
> via Cobalt Networks.  It has been fairly extensively modified since then.
> */
>
> +#ifndef _GNU_SOURCE
> +#  define _GNU_SOURCE 1
> +#endif
> +#include <sys/types.h>
> +#include <sys/ipc.h>
> +#include <sys/sem.h>
> +#include <sys/stat.h>
> +#include <time.h>
> +#include <errno.h>
> +#include <sched.h>
>
> +#include "csem.h"
>
> +#if defined(__GNU_LIBRARY__) && !defined(_SEM_SEMUN_UNDEFINED)
> +/* union semun is defined by including <sys/sem.h> */
> +#else
> +/* according to X/OPEN we have to define it ourselves */
> +union semun {
> int val;                    /* value for SETVAL */
> struct semid_ds *buf;       /* buffer for IPC_STAT, IPC_SET */
> unsigned short int *array;  /* array for GETALL, SETALL */
> struct seminfo *__buf;      /* buffer for IPC_INFO */
>
> +};
> +#endif
>
> +/*
> On some platforms semctl(SETVAL) sets sem_otime, on other platforms it
> does not.  Figure out what this platform does.
> Returns 0 if semctl(SETVAL) does not set sem_otime
> Returns 1 if semctl(SETVAL) does set sem_otime
> Returns -1 on error
> */
>
> +static int does_semctl_set_otime(void)
> +{
> int sem_id;
> int ret;
> /* create a test semaphore */
> sem_id = semget(IPC_PRIVATE, 1, S_IRUSR|S_IWUSR);
> if (sem_id < 0) {
> return -1;
> }
> /* set the value */
> if (csem_setval(sem_id, 1) < 0) {
> csem_destroy(sem_id);
> return -1;
> }
> /* read sem_otime */
> ret = (csem_get_otime(sem_id) > 0) ? 1 : 0;
> /* clean up */
> csem_destroy(sem_id);
> return ret;
>
> +}
>
> +int csem_create(key_t key, unsigned val)
> +{
> static int need_otime_hack = -1;
> int sem_id;
> /* see if we need to trigger a semop to set sem_otime */
> if (need_otime_hack < 0) {
> int ret = does_semctl_set_otime();
> if (ret < 0) {
> 	return -1;
> }
> need_otime_hack = !ret;
> }
> /* create it or fail */
> sem_id = semget(key, 1, IPC_CREAT|IPC_EXCL | S_IRUSR|S_IWUSR);
> if (sem_id < 0) {
> return -1;
> }
> /* initalize the value */
> if (need_otime_hack) {
> val++;
> }
> if (csem_setval(sem_id, val) < 0) {
> csem_destroy(sem_id);
> return -1;
> }
> if (need_otime_hack) {
> /* force sem_otime to change */
> csem_down(sem_id);
> }
> return sem_id;
>
> +}
>
> +/* how many times to loop, waiting for sem_otime */
> +#define MAX_OTIME_LOOPS 1000
>
> +int csem_get(key_t key)
> +{
> int sem_id;
> int i;
> /* CSEM_PRIVATE needs to go through csem_create() to get an
> * initial value */
> if (key == CSEM_PRIVATE) {
> errno = EINVAL;
> return -1;
> }
> /* get the (assumed existing) semaphore */
> sem_id = semget(key, 1, S_IRUSR|S_IWUSR);
> if (sem_id < 0) {
> return -1;
> }
> /* loop until sem_otime != 0, which means it has been initialized */
> for (i = 0; i < MAX_OTIME_LOOPS; i++) {
> time_t otime = csem_get_otime(sem_id);
> if (otime < 0) {
> 	/* error */
> 	return -1;
> }
> if (otime > 0) {
> 	/* success */
> 	return sem_id;
> }
> /* retry */
> sched_yield();
> }
> /* fell through - error */
> return -1;
>
> +}
>
> +int csem_get_or_create(key_t key, unsigned val)
> +{
> int sem_id;
> /* try to create the semaphore */
> sem_id = csem_create(key, val);
> if (sem_id >= 0 || errno != EEXIST) {
> /* it either succeeded or got an error */
> return sem_id;
> }
> /* it must exist already - get it */
> sem_id = csem_get(key);
> if (sem_id < 0) {
> return -1;
> }
> return sem_id;
>
> +}
>
> +int csem_destroy(int sem_id)
> +{
> return semctl(sem_id, 0, IPC_RMID);
>
> +}
>
> +int csem_getval(int sem_id)
> +{
> return semctl(sem_id, 0, GETVAL);
>
> +}
>
> +int csem_setval(int sem_id, unsigned val)
> +{
> union semun arg;
> arg.val = val;
> if (semctl(sem_id, 0, SETVAL, arg) < 0) {
> return -1;
> }
> return 0;
>
> +}
>
> +static int csem_up_undoflag(int sem_id, int undoflag)
> +{
> struct sembuf sops;
> sops.sem_num = 0;
> sops.sem_op = 1;
> sops.sem_flg = undoflag;
> return semop(sem_id, &sops, 1);
>
> +}
>
> +int csem_up(int sem_id)
> +{
> return csem_up_undoflag(sem_id, 0);
>
> +}
>
> +int csem_up_undo(int sem_id)
> +{
> return csem_up_undoflag(sem_id, SEM_UNDO);
>
> +}
>
> +static int csem_down_undoflag(int sem_id, int undoflag)
> +{
> struct sembuf sops;
> sops.sem_num = 0;
> sops.sem_op = -1;
> sops.sem_flg = undoflag;
> return semop(sem_id, &sops, 1);
>
> +}
>
> +int csem_down(int sem_id)
> +{
> return csem_down_undoflag(sem_id, 0);
>
> +}
>
> +int csem_down_undo(int sem_id)
> +{
> return csem_down_undoflag(sem_id, SEM_UNDO);
>
> +}
>
> +static int csem_down_timeout_undoflag(int sem_id,
>                                  struct timespec *timeout,
>                                  int undoflag)
>
> +{
> struct sembuf sops;
> sops.sem_num = 0;
> sops.sem_op = -1;
> sops.sem_flg = undoflag;
> return semtimedop(sem_id, &sops, 1, timeout);
>
> +}
>
> +int csem_down_timeout(int sem_id, struct timespec *timeout)
> +{
> return csem_down_timeout_undoflag(sem_id, timeout, 0);
>
> +}
>
> +int csem_down_timeout_undo(int sem_id, struct timespec *timeout)
> +{
> return csem_down_timeout_undoflag(sem_id, timeout, SEM_UNDO);
>
> +}
>
> +time_t csem_get_otime(int sem_id)
> +{
> union semun arg;
> struct semid_ds ds;
> arg.buf = &ds;
> if (semctl(sem_id, 0, IPC_STAT, arg) < 0) {
> return -1;
> }
> return ds.sem_otime;
>
> +}
> Index: ipc_lock.h
> ===================================================================
> --- ipc_lock.h	(revision 0)
> +++ ipc_lock.h	(revision 0)
> @@ -0,0 +1,59 @@
> +/*
> Copyright (C) 2010 Google Inc.
> 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 Street, Fifth Floor, Boston, MA 02110-1301, USA
> */
>
> +#ifndef IPC_LOCK_H__
> +#define IPC_LOCK_H__
>
> +#include <sys/ipc.h>
>
> +struct ipc_lock {
> key_t key;         /* provided by the developer */
> int sem;           /* internal */
> int is_held;       /* internal */
>
> +};
>
> +/* don't use C99 initializers here, so this can be used in C++ code */
> +#define IPC_LOCK_INIT(key) \
> { \
> key,       /* name */ \
> -1,        /* sem */ \
> 0,         /* is_held */ \
> }
>
> +/*
> acquire_lock: acquire a lock
> timeout <0 = no timeout (try forever)
> timeout 0  = do not wait (return immediately)
> timeout >0 = wait up to $timeout milliseconds (subject to kernel scheduling)
> return 0   = lock acquired
> return >0  = lock was already held
> return <0  = failed to acquire lock
> */
>
> +extern int acquire_lock(struct ipc_lock *lock, int timeout_msecs);
>
> +/*
> release_lock: release a lock
> returns 0 if lock was released successfully
> returns -1 if lock had not been held before the call
> */
>
> +extern int release_lock(struct ipc_lock *lock);
>
> +#endif /* IPC_LOCK_H__ */
> Index: csem.h
> ===================================================================
> --- csem.h	(revision 0)
> +++ csem.h	(revision 0)
> @@ -0,0 +1,154 @@
> +/*
> Copyright 2003 Sun Microsystems, Inc.
> Copyright 2010 Google, Inc.
> 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.
> Developer's note: This was open sourced by Sun Microsystems, which got it
> via Cobalt Networks.  It has been fairly extensively modified since then.
> */
>
> +#ifndef CSEM_H__
> +#define CSEM_H__
>
> +#include <sys/ipc.h>
> +#include <time.h>
>
> +/* create a private key */
> +#define CSEM_PRIVATE IPC_PRIVATE
>
> +/*
> Create a new semaphore with the specified key, initialized to the
> specified value.  If the key is CSEM_PRIVATE, a new private semaphore
> is allocated.
> Returns the sempahore ID (>= 0) on success.
> Returns < 0 on error, or if the key already exists.
> */
>
> +extern int csem_create(key_t key, unsigned val);
>
> +/*
> Fetch an existing semaphore with the specified key.
> Returns the sempahore ID (>= 0) on success.
> Returns < 0 on error, or if the key does not exist.
> */
>
> +extern int csem_get(key_t key);
>
> +/*
> Fetch or create a semaphore with the specified key.  If the semaphore
> did not exist, it will be created with the specified value.
> Returns the sempahore ID (>= 0) on success.
> Returns < 0 on error.
> */
>
> +extern int csem_get_or_create(key_t key, unsigned val);
>
> +/*
> Destroy the semaphore.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_destroy(int sem_id);
>
> +/*
> Get the value of the semaphore.
> Returns the value (>= 0) on success.
> Returns < 0 on error.
> */
>
> +extern int csem_getval(int sem_id);
>
> +/*
> Set the value of the semaphore.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_setval(int sem_id, unsigned val);
>
> +/*
> Increment the semaphore.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_up(int sem_id);
>
> +/*
> Increment the semaphore.  This operation will be undone when the
> process terminates.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_up_undo(int sem_id);
>
> +/*
> Decrement the semaphore, or block if sem == 0.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_down(int sem_id);
>
> +/*
> Decrement the semaphore, or block if sem == 0.  This operation will be
> undone when the process terminates.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_down_undo(int sem_id);
>
> +/*
> Decrement the semaphore, or block with a timeout if sem == 0.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_down_timeout(int sem_id, struct timespec *timeout);
>
> +/*
> Decrement the semaphore, or block with a timeout if sem == 0.  This
> operation will be undone when the process terminates.
> Returns 0 on success.
> Returns < 0 on error.
> */
>
> +extern int csem_down_timeout_undo(int sem_id, struct timespec *timeout);
>
> +/*
> Get the timestamp of the last csem_up()/csem_down() call.
> Returns sem_otime on success.
> Returns < 0 on error
> */
>
> +extern time_t csem_get_otime(int sem_id);
>
> +#endif /* CSEM_H__ */

Otherwise this code looks good to me, albeit a little bit bloated for
just such little functionality. Anyway, Carl-Daniel should comment on this.

Mathias

[David Hendricks]

On Wed, Feb 2, 2011 at 12:58 AM, Mathias Krause
wrote:
> On 01.02.2011 22:37, David Hendricks wrote:
> > Hi,
> > I have attached a patch which adds a locking mechanism to Flashrom to
> > prevent multiple instances from running simultaneously. It may need some
> > modification to fit different Flashrom use cases better, but I think
> > it's a good start.
>
> Nice

Thanks!

On Wed, Feb 2, 2011 at 12:58 AM, Mathias Krause 
 wrote:
> .> The size of the patch is somewhat misleading -- The only real changes to
> > Flashrom's current code are some of the exit points in cli_classic() and
> > some added stuff in the Makefile. Everything else is contained in new
> > files. The new files are:
> > csem.{c,h}: Low-level code that interfaces with semctl(), semget(), etc.
> > ipc_lock.{c,h}: Wrapper for csem stuff.
> > locks.h: A listing of locks that Flashrom cares about.
> > big_lock.{c,h}: An even higher-level wrapper around ipc_lock stuff,
> > useful for simple, coarse-grained locking.
>
> Well, quite some wrapping wrappers :/

Agreed, to a point. The ipc_lock stuff is a useful abstraction. The big_lock
stuff is just there to emphasize the notion of a single lock used for
multiple programs -- It's just a trivial wrapper for handling a single lock.
> From Flashrom's perspective this probably isn't very useful. Perhaps that's

reason enough to omit it. It's mostly just there to help coordinate between
different programs (ie for distro maintainers).

On Wed, Feb 2, 2011 at 12:58 AM, Mathias Krause 
 wrote:
> > util/use_big_lock.sh: Used by the Makefile to test for POSIX.1-2001
> > compliance.
>
> That test should be integrated into the Makefile, no?

Maybe. I spent some time trying to do that but my result came out fugly.

IIRC the hard part was trying to define a macro based on the result of the
command. The other tests do the opposite -- They use an existing macro to
determine whether to run a test. Someone with stronger Make-fu is welcome to
show me how this is done :-)

On Wed, Feb 2, 2011 at 12:58 AM, Mathias Krause 
 wrote:
> Otherwise this code looks good to me, albeit a little bit bloated for
> just such little functionality. Anyway, Carl-Daniel should comment on this.

Fair enough. I feel it's useful and wanted to get this upstreamed to reduce
diffs w/ Chromium OS. But it is pretty heavy-weight for what might not be
important to the upstream flashrom community at this time.

To illustrate our usage case, on Chrome OS devices* we have multiple
programs that access the ROM. Our auto-updater uses these programs in the
background. We've already been bitten by developers attempting to manually
update/reflash their BIOS and/or EC firmware while the auto-updater is
attempting to do stuff. ECs can be especially troublesome since they often
access their ROM constantly and will crash if you corrupt the ROM, unlike
most x86 BIOSes where you at least stand a chance to recover.

*Chrome OS is tailored for specific devices. Chromium OS is the more generic
version that can be put on a broad range of hardware.

[Patch] Add coarse-grained IPC locking mechanism to Flashrom

[Carl-Daniel Hailfinger]

Hi David,

thanks for your patch!

Auf 01.02.2011 22:37, David Hendricks schrieb:
> I have attached a patch which adds a locking mechanism to Flashrom to
> prevent multiple instances from running simultaneously. It may need some
> modification to fit different Flashrom use cases better, but I think it's a
> good start.
> [...]
> Disadvantages:
> The current patch is very coarse-grained, which could be problematic for
>
> people who wish to flash multiple chips simultaneously.

That's the problem which might be the biggest hurdle. Users with
Dediprog won't be happy about that.
> From my perspective, we first have to make a design decision:

- Global lock
- Programmer type lock
- Programmer instance lock

If we decide to use the programmer instance lock, the internal
programmer must use the programmer instance lock like a programmer type
lock due to bus contention etc. A programmer instance lock would be
extremely useful for Dediprog, FT2232, anything Serprog and EZo/Willem
devices.
For PCI-based programmers the usefulness is debatable because those
usually use a busy loop for timing, and having more than one such
busy-loop run at the same time will screw up timing. In fact, even a
programmer type lock would not be sufficient here because nic3com,
internal, satasii and others all use the same busy loop for timing.

Side note: If you have any active (i.e. consuming CPU) processes in the
background or if you have any CPU frequency scaling daemon active in the
background, timing is guaranteed to be shot. SPI flash does not really
care about timing, so you probably won't notice it on the Cr-48 laptop.
It may cause random corruption on devices with parallel or LPC/FWH
flash, though. NOTE: flashrom qualifies as CPU-consuming process. A way
to prevent timing mishaps is to dedicate one CPU core to each flashrom
instance.

In short: Either you use timing-insensitive flash (usually SPI) or you
use n CPU cores for n flashrom instances. Anything else will result in
timing-related corruption which may or may not be below the threshold
which triggers flashrom to abort.
> Requires >= POSIX.1-2001 compliance.

That will be difficult to get on Windows and DOS, and I'd like to see
some tests on OSX/*BSD/OpenSolaris as well. We'll need a test for
working locking, though. I fear there might be systems out there which
advertise POSIX.1-2001 compliance to get stuff to compile, but which are
missing real locking.
Then there's the problem of running flashrom on top of libpayload. Do we
need locking for that? Probably not until libpayload grows into a
lightweight OS.
> Signed-off-by: David Hendricks

No code review yet, because the design has to be agreed upon first.

Regards,
Carl-Daniel