Modify

Opened 7 years ago

Last modified 5 years ago

#104 assigned enhancement

flashrom: Partial operations; change flash drivers to not erase in the write function

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

Description

The user should be responsible for ensuring that the flash chip has been erased where new data should be written.

At the moment, flashrom will erase at least one full page and then rewrite it, when the user asks to only write a few bytes using -l/-i/-s/-e.

Attachments (0)

Change History (17)

comment:1 Changed 7 years ago by hailfinger

This needs a big fat warning issued to the user if he attempts to write to a non-erased area. Maybe even an abort.

comment:2 Changed 7 years ago by anonymous

If at all, NOT erasing should be an option. Not the other way round.

a) this would break all scripts out there b) it's just bad design from a user perspective. c) it's technical infatuation rather than looking at the big picture.

I veto any patch that changes the "automatic" behavior per default in advance.

comment:3 Changed 7 years ago by hailfinger

The problem is that SPI chips do not erase by default, so we have inconsistent behaviour. We need to agree on one behaviour and "fix" the non-conforming drivers.

comment:4 Changed 7 years ago by stepan

If this should become agreed upon, which I don't hope, I suggest that we at least check the area before writing, if the writing can theoretically succeed at all. ie. if all bytes are 0xff. And print a warning if they are not all ff. Plus print an error if those bits that we try to set to 1 are 0.

comment:5 Changed 6 years ago by stuge

Maybe the original comment was overzealous, and the decision to erase should not be moved all the way out to the user - but I think it should at least be moved out of the write functions in drivers, and instead be handled by flashrom common code.

comment:6 Changed 6 years ago by hailfinger

Stefan's comments about printing a warning if to-be-written areas are not 0xff is valid. Same about erroring out if any bit being 0 would be set to 1 by the write (which is impossible).

comment:7 Changed 6 years ago by stuge

  • Dependencies #103 deleted
  • Milestone changed from flashrom v1.0 to flashrom v1.1
  • Owner changed from somebody to stuge

comment:8 Changed 6 years ago by stuge

  • Keywords partial added
  • Summary changed from flashrom: Change flash drivers to never erase data before writing to flashrom: Change flash drivers to not erase in the write function
  • Type changed from defect to enhancement

--8<-- email from Yu Ning FENG

We are talking about choosing some options from the full list:

* chip read
* chip erase
  chip write (0xff to other)
* chip erase-and-write (option 'write')
   
  partial read
  partial erase
1 partial write
2 partial erase-and-write
  
* = Options we have
1 = Carl-Daniel suggests
2 = Other people suggest

Every option in the list is useful in some case. My opinion on -

the choice  - Support the full list;
1           - Warning and ask for continuation. No checking for 0xff;
2           - We need this convenience for partial modification.

My two cents.

yu ning

-->8--

comment:9 Changed 6 years ago by stuge

  • Summary changed from flashrom: Change flash drivers to not erase in the write function to flashrom: Partial operations; change flash drivers to not erase in the write function

comment:10 Changed 6 years ago by hailfinger

  • Milestone changed from flashrom v1.1 to flashrom v1.0

flashrom has been changed to consistent erase-before-write operation.

Support for partial erase has been added, so now is the time to consider splitting write and erase and have some walker function which walks the blocks and erases one, then writes it etc.

comment:11 Changed 5 years ago by hailfinger

  • Owner changed from stuge to hailfinger

The "do we need an erase" detection is implemented by this patch: http://patchwork.coreboot.org/patch/582/

An Acked-by would be appreciated.

comment:12 Changed 5 years ago by stuge

I could ack the patch since it just adds a function, but the function doesn't fix this ticket.

This ticket is about changing write functions in drivers to never erase. (Ie. move erasing out of the driver write functions.)

Once that's done (if already, then this could be closed as fixed) the next step is how to decide when to erase. It used to be always. Patch 582 suggest a heuristic, but I don't think this is wise, since code can not know what has happened to the flash chip before. I suggest that this decision must be made by the user.

comment:13 Changed 5 years ago by hailfinger

Yes, patch 582 is just a part of the solution, but I think it is a prerequisite to solve this completely. The idea behind patch 582 is to annotate struct flashchip with erase granularity (separate patch!) and use that info and patch 582 to determine whether we need an erase or not.

The removal of unconditional erase is halfway done. A few chips remain, but Sean Nelson is tackling them right now.

comment:14 Changed 5 years ago by stuge

Yes, granularity was coded into write functions and should absolutely go into structured data instead. That patch I'll gladly ack if needed.

But I still think erase-or-not must be a user decision rather than a heuristic.

comment:15 Changed 5 years ago by hailfinger

Could you please elaborate on what you mean with "heuristic"? It seems we are talking about different things and I want to understand your point.

AFAICT flashrom should arrive at the desired image (i.e. what the user wants) with the minimal amount (may be zero) of erases. For that, we need write granularity info about the chip. Asking the user to look that up in the datasheet is doable, but not optimal. Flashrom should know that information (not by guessing or heuristic, but by hard info from the datasheet). For convenience, there could be a flashrom option "do not erase even if the chip datasheet explicitly mentions that this write will not work without preceding erase".

comment:16 Changed 5 years ago by hailfinger

  • Status changed from new to assigned

comment:17 Changed 5 years ago by stuge

"If current flash bits look like we might be able to write without erase" is a heuristic and since it's impossible to know how many times the flash chip has already been written to in order to get the current contents a heuristic isn't useful.

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.