#103: flashrom: Don't exit() after successful erase operation

List overview All Threads
Download

newer

older

[commit] r5412 - in trunk/src:...

#119: Winbond W39V040FBPZ is not...

coreboot

2 Jul 2008 2 Jul '08

6:42 p.m.

#103: flashrom: Don't exit() after successful erase operation ---------------------------------+------------------------------------------ Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase exit | Dependencies: Patchstatus: there is no patch | ---------------------------------+------------------------------------------

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103 coreboot http://www.coreboot.org/

Show replies by thread

coreboot

3 Jul 3 Jul

9:35 p.m.

#103: flashrom: Don't exit() after successful erase operation -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase exit Dependencies: | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Changes (by stuge):

* owner: somebody => stuge

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:1 coreboot http://www.coreboot.org/

coreboot

9:35 p.m.

#103: flashrom: Don't exit() after successful erase operation -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase exit Dependencies: | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Changes (by stuge):

* status: new => assigned

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:2 coreboot http://www.coreboot.org/

coreboot

5 Jul 5 Jul

9:42 a.m.

New subject: #103: flashrom: Don't exit() after successful erase (was: flashrom: Don't exit() after successful erase operation)

#103: flashrom: Don't exit() after successful erase -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase exit Dependencies: | Patchstatus: patch needs review -------------------------+-------------------------------------------------- Changes (by stuge):

* patchstatus: there is no patch => patch needs review

-- Ticket URL: http://tracker.linuxbios.org/trac/coreboot/ticket/103#comment:3 coreboot http://www.coreboot.org/

coreboot

12:11 p.m.

New subject: #103: flashrom: Don't exit() after successful erase

Comment(by hailfinger):

While I do like the patch, I fear the user interface of flashrom is less than optimal in this case.

AFAICS flashrom -Er and flashrom -rE are equivalent, but user expectation is completely different. Can we honor the order of commands or at least place reading before erasing and writing?

The best solution would be making -r and -E mutually exclusive like -r -and -w are.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:4 coreboot http://www.coreboot.org/

coreboot

5 Dec 5 Dec

3:58 a.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation (was: flashrom: Don't exit() after successful erase)

#103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation ----------------------------------+----------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase exit testing | Dependencies: #117 Patchstatus: patch needs review | ----------------------------------+----------------------------------------- Changes (by stuge):

* keywords: erase exit => erase exit testing * dependencies: => #117 * type: defect => enhancement

Comment:

This allows -Er -Ew and -Ev but they are pretty useless. The point is that this allows -Ewv which will test erase, write and read (both during erase and verify) operations on the probed flash chip, so it's an easy way to exercise flash chip drivers.

Currently the order of commands (-E -r -w -v) on the command line is not significant, if multiple commands are specified flashrom will always execute them in order ERASE READ WRITE VERIFY.

This can be bad for users running -rE expecting to get a backup before the erase is done. A simple solution would be to change the order in main() into READ ERASE WRITE VERIFY, in which case the invocation:

flashrom -rEwv org.bin

will save the original contents, erase the chip and write back the original, verifying each step.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:5 coreboot http://www.coreboot.org/

coreboot

6 Dec 6 Dec

11:21 p.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation

#103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation ----------------------------------+----------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: assigned Priority: minor | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase exit testing | Dependencies: #117 Patchstatus: patch needs review | ----------------------------------+----------------------------------------- Changes (by stepan):

* priority: major => minor

Comment:

As you said, the new "features" introduced by the patch are rather useless.

If this is about making testing easier, what about explicitly creating a " --test" option that does things in a defined order. Then we don't have to worry about doing implicit things.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:6 coreboot http://www.coreboot.org/

coreboot

11:48 p.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation

* priority: minor => major

Comment:

I suggested adding -T in #106 (dup of this) and I closed it because the patch suggested here achieves the same thing but with the user interface -Ewv (already familiar options) instead of -T (which would be new).

But if everyone is really serious about restricting what flashrom lets the user do - unless it is invoked a special way - I guess that's what we have to do, but I think it's silly. Let user shoot foot if they try - that's why I fell in love with UNIX anyway.

(And I don't think simplifying the test procedure from many flashrom+hexdump commands into a single flashrom command is minor, so changing prio back.)

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:7 coreboot http://www.coreboot.org/

coreboot

8 Dec 8 Dec

12:41 a.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation

Comment(by hailfinger):

Moving read before erase as a single simple patch is

Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net

That leaves us with another question: Should -Erwv or any other read/write combinations be allowed? I think the semantics of read+write are totally unclear. If you perform read, erase, write, verify (in that order) it means that you just rewrite ROM contents. That may or may not be desired, but it sure would work for testing read/write/erase operations.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:8 coreboot http://www.coreboot.org/

coreboot

2:31 p.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation

#103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation ----------------------------------+----------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase exit testing | Dependencies: #117 Patchstatus: patch needs work | ----------------------------------+----------------------------------------- Changes (by stepan):

* patchstatus: patch needs review => patch needs work

Comment:

It also does not necessarily do the same thing with the hardware as calling the single functions. Strictly, a reboot between any of the tests would be the most reliable way of testing flashrom.

Answering Peter's concerns, if we're going to add a method of "simplifying flashrom tests" that is a new feature, and it should get a new option. Using -Erwv for that ignores that fact by adding a lot of implicit assumptions. Implicit assumptions are fatal for any user interface.

Strictly speaking, -wv should be disallowed, too. Instead, write should guarantee that the write succeeded, or print an error otherwise. As should an erase make sure that the chip is erased, if the program exists without error.

Other than that, we should stick with the small utility approach, and have the tool do one thing at a time.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:9 coreboot http://www.coreboot.org/

coreboot

13 Mar 13 Mar

6:20 p.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation

#103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation ----------------------------------+----------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase exit testing | Dependencies: #117 Patchstatus: patch needs work | ----------------------------------+-----------------------------------------

Comment(by stepan):

ping

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:10 coreboot http://www.coreboot.org/

coreboot

5 Oct 5 Oct

2:48 a.m.

New subject: #103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation

#103: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation ----------------------------------+----------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase exit testing | Dependencies: #117 Patchstatus: patch needs work | ----------------------------------+-----------------------------------------

Comment(by hailfinger):

We now have auto-erase for write and auto-verify for erase and write.

Combining -Erwv does not make sense anymore, but a separate -T parameter for a full test would indeed be helpful. Not sure if I should close this ticket.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:11 coreboot http://www.coreboot.org/

coreboot

16 Jan 16 Jan

12:23 a.m.

New subject: #103: flashrom: Add -T to test all operations in one invocation (was: flashrom: Don't exit() after successful erase; enable testing all operations in one invocation)

#103: flashrom: Add -T to test all operations in one invocation ---------------------------------+------------------------------------------ Reporter: stuge | Owner: hailfinger Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: testing | Dependencies: #117 Patchstatus: there is no patch | ---------------------------------+------------------------------------------ Changes (by hailfinger):

* keywords: erase exit testing => testing * owner: stuge => hailfinger * patchstatus: patch needs work => there is no patch * status: assigned => new

Comment:

Given that chips now support multiple erase functions, testing is way more complicated. We need a separate -T, everything else will only give us incomplete test reports.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/103#comment:12 coreboot http://www.coreboot.org/

coreboot

12 Apr 12 Apr

11:26 p.m.

New subject: #103: flashrom: Add -T to test all operations in one invocation

#103: flashrom: Add -T to test all operations in one invocation -----------------------------------------------------------+---------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom (please use trac on flashrom.org) | Keywords: testing Dependencies: #117 | Patchstatus: there is no patch -----------------------------------------------------------+---------------- Changes (by hailfinger):

* status: new => assigned

Comment:

I have a patch for this, but it needs partial write support, and that will be added post 0.9.2.

-- Ticket URL: https://tracker.coreboot.org/trac/coreboot/ticket/103#comment:13 coreboot http://www.coreboot.org/

5128

days inactive

5777

days old

coreboot@coreboot.org

13 comments

1 participants

tags (0)

participants (1)

coreboot