Attention is currently required from: Nico Huber, Subrata Banik, Edward O'Callaghan. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62213 )
Change subject: flashrom.c: Implement file-based locking semantics ......................................................................
Patch Set 1:
(4 comments)
File big_lock.c:
https://review.coreboot.org/c/flashrom/+/62213/comment/a75f89f5_7dccf3a7 PS1, Line 39: return acquire_lock(&big_lock, timeout_secs * 1000); should we check #if BIG_LOCK and decide if lock is required?
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/62213/comment/1513440f_8be62fe3 PS1, Line 51: #ifndef USE_BIG_LOCK : #define USE_BIG_LOCK 0 : #endif Not really used?
https://review.coreboot.org/c/flashrom/+/62213/comment/05e159b1_68263457 PS1, Line 151: /* Get big 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"); : return -1; : } : big_lock_acquired = true; : msg_gdbg("Lock acquired.\n"); : If we're going to have a big_lock.c, I think the implementation here should only contain the timeout, not the status and debug messages; e.g.,
if (acquire_big_lock(LOCK_TIMEOUT_SECS) < 0) return -1;
And move all other logic into acquire_big_lock itself. WDYT?
https://review.coreboot.org/c/flashrom/+/62213/comment/c9b83ce3_65a8c0ce PS1, Line 217: if (big_lock_acquired) { : release_big_lock(); : big_lock_acquired = false; : } we should just call release_big_lock() and let it check/handle big_lock_acquired internally