Modify

Opened 7 years ago

Last modified 5 years ago

#103 assigned enhancement

flashrom: Add -T to test all operations in one invocation

Reported by: stuge Owned by: hailfinger
Priority: major Milestone: flashrom v1.0
Component: flashrom (please use trac on flashrom.org) Keywords: testing
Cc: Dependencies: #117
Patch Status: there is no patch

Description


Attachments (3)

fr.103.erasenoexit.patch (2.0 KB) - added by stuge 7 years ago.
fr.103.erasenoexit.r3793.patch (2.0 KB) - added by stuge 7 years ago.
fr.103.erasenoexit.r3797.patch (968 bytes) - added by stuge 7 years ago.
Don't exit after successful erase. Maybe reading should be moved before erasing?

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by stuge

  • Owner changed from somebody to stuge

comment:2 Changed 7 years ago by stuge

  • Status changed from new to assigned

Changed 7 years ago by stuge

comment:3 Changed 7 years ago by stuge

  • Patch Status changed from there is no patch to patch needs review
  • Summary changed from flashrom: Don't exit() after successful erase operation to flashrom: Don't exit() after successful erase

comment:4 Changed 7 years ago 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.

Changed 7 years ago by stuge

comment:5 Changed 7 years ago by stuge

  • Dependencies set to #117
  • Keywords testing added
  • Summary changed from flashrom: Don't exit() after successful erase to flashrom: Don't exit() after successful erase; enable testing all operations in one invocation
  • Type changed from defect to enhancement

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.

Changed 7 years ago by stuge

Don't exit after successful erase. Maybe reading should be moved before erasing?

comment:6 Changed 7 years ago by stepan

  • Priority changed from major to minor

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.

comment:7 Changed 7 years ago by stuge

  • Priority changed from minor to major

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.)

comment:8 Changed 7 years ago by hailfinger

Moving read before erase as a single simple patch is

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@…>

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.

comment:9 Changed 7 years ago by stepan

  • Patch Status changed from patch needs review to patch needs work

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.

comment:10 Changed 6 years ago by stepan

ping

comment:11 Changed 6 years ago 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.

comment:12 Changed 6 years ago by hailfinger

  • Keywords erase exit removed
  • Owner changed from stuge to hailfinger
  • Patch Status changed from patch needs work to there is no patch
  • Status changed from assigned to new
  • Summary changed from flashrom: Don't exit() after successful erase; enable testing all operations in one invocation to flashrom: Add -T to test all operations in one invocation

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.

comment:13 Changed 5 years ago by hailfinger

  • Status changed from new to assigned

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

Add Comment

Modify Ticket

Action
as assigned The owner will remain hailfinger.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.