#104: flashrom: Change flash drivers to never erase data before writing
#104: flashrom: Change flash drivers to never erase data before writing ---------------------------------+------------------------------------------ Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase write | Dependencies: #103 Patchstatus: there is no patch | ---------------------------------+------------------------------------------ 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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to never erase data before writing -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase write Dependencies: #103 | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:1> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to never erase data before writing -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase write Dependencies: #103 | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:2> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to never erase data before writing -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase write Dependencies: #103 | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:3> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to never erase data before writing -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: erase write Dependencies: #103 | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:4> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to never erase data before writing ---------------------------------+------------------------------------------ Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase write | Dependencies: #103 Patchstatus: there is no patch | ---------------------------------+------------------------------------------ Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:5> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to never erase data before writing ---------------------------------+------------------------------------------ Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: erase write | Dependencies: #103 Patchstatus: there is no patch | ---------------------------------+------------------------------------------ Comment(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). -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:6> coreboot <http://www.coreboot.org/>
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
#104: flashrom: Change flash drivers to never erase data before writing ---------------------------------+------------------------------------------ Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.1 Component: flashrom | Version: Keywords: erase write | Dependencies: Patchstatus: there is no patch | ---------------------------------+------------------------------------------ Changes (by stuge): * owner: somebody => stuge * dependencies: #103 => * milestone: flashrom v1.0 => flashrom v1.1 -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:7> coreboot <http://www.coreboot.org/>
#104: flashrom: Change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.1 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Changes (by stuge): * keywords: erase write => partial erase write * type: defect => enhancement Comment: --8<-- [http://www.coreboot.org/pipermail/coreboot/2008-December/042781.html 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-- -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:8> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.1 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:9> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: stuge Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Changes (by hailfinger): * milestone: flashrom v1.1 => flashrom v1.0 Comment: 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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:10> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Changes (by hailfinger): * owner: stuge => hailfinger Comment: The "do we need an erase" detection is implemented by this patch: http://patchwork.coreboot.org/patch/582/ An Acked-by would be appreciated. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:11> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:12> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:13> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:14> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Comment(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". -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:15> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Changes (by hailfinger): * status: new => assigned -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:16> coreboot <http://www.coreboot.org/>
#104: flashrom: Partial operations; change flash drivers to not erase in the write function -----------------------------------+---------------------------------------- Reporter: stuge | Owner: hailfinger Type: enhancement | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: partial erase write | Dependencies: Patchstatus: there is no patch | -----------------------------------+---------------------------------------- Comment(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. -- Ticket URL: <http://tracker.coreboot.org/trac/coreboot/ticket/104#comment:17> coreboot <http://www.coreboot.org/>
participants (2)
-
coreboot -
FENG Yu Ning