[flashrom] [PATCH] Real partial writes

[Carl-Daniel Hailfinger]

On 29.10.2010 01:02, Idwer Vollering wrote:
> 2010/10/29 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>
>   
>> On 26.10.2010 19:22, Carl-Daniel Hailfinger wrote:
>>     
>>> On 26.10.2010 16:27, Idwer Vollering wrote:
>>>
>>>       
>>>> 2010/10/22 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>>>
>>>>
>>>>
>>>>         
>>>>> Given that the version which addresses the review comments apparently
>>>>>           
>> is
>>     
>>>>> buggy, I hereby propose to merge the earlier version which worked. It
>>>>>           
>> is
>>     
>>>>> here again for your reference with some slight cosmetic fixes which
>>>>> should not impact functionality.
>>>>> Error recovery still needs work (see the FIXME comment). It will work
>>>>>           
>> in
>>     
>>>>> most cases, but if a given erase command for a block only erased parts
>>>>> of the block (partial write lock), the code will make incorrect
>>>>> assumptions.
>>>>>
>>>>> If anyone feels adventurous, I would love to see logs on Intel/VIA
>>>>> chipsets with SPI (preferably locked down chipsets or with r1193
>>>>>           
>> reverted).
>>     
>>>>> If you're going to review this, make sure you keep a stack of bananas
>>>>> (quickly mobilized carbohydrates for your brain), a bucket of ice (to
>>>>> prevent brain overheating) and a bottle of aspirin handy. If any code
>>>>>           
>> is
>>     
>>>>> unclear, please tell me and I'll try to add comments to improve
>>>>> readability.
>>>>>
>>>>> This code has been tested. Testing erase (and checking with a separate
>>>>> readback that erase actually worked) and write (same test with separate
>>>>> readback) would be highly appreciated. Verbose logs are even more
>>>>> appreciated.
>>>>>
>>>>> I think the code is ready for merge if you trust write/erase to never
>>>>> fail. The error cases still need to be tested. Should we reread the
>>>>> whole chip if write/erase failed to make sure our current view of the
>>>>> chip contents is not stale?
>>>>>
>>>>> This patch makes flashrom use real partial writes. If you write an
>>>>>           
>> image
>>     
>>>>> full of 0xff, flashrom will erase and detect that no write is needed.
>>>>>           
>> If
>>     
>>>>> you write an image which differs only in some parts from the current
>>>>> flash contents, flashrom will detect that and not touch unchanged
>>>>>           
>> areas.
>>     
>>>>> Fix a long-standing bug in need_erase() for 256 byte granularity as
>>>>>           
>> well.
>>     
>>>>> Nice side benefit: Detailed progress printing.
>>>>> S means skipped
>>>>> E means erased
>>>>> W means written
>>>>>
>>>>> Thanks to Andrew Morgan for testing countless iterations of this patch.
>>>>> Thanks to Richard A. Smith for testing on Dediprog.
>>>>> Thanks to David Hendricks for the review (will be addressed later).
>>>>>
>>>>> Signed-off-by: Carl-Daniel Hailfinger <
>>>>>           
>> c-d.hailfinger.devel.2006 at gmx.net>
>>     
>>>>>
>>>>>           
>>>> With your latest patch (http://patchwork.coreboot.org/patch/2160/)
>>>>         
>> applied:
>>     
>>>> $ sudo ./flashrom -p nicintel_spi -V -w nicintel_spi.rom -c M25P10.RES
>>>> flashrom v0.9.3-r1215 on FreeBSD 8.1-RELEASE (i386), built with libpci
>>>> 3.1.7, GCC 4.2.1 20070719  [FreeBSD], little endian
>>>> Calibrating delay loop... OK.
>>>> Initializing nicintel_spi programmer
>>>> Found "Intel 82541PI Gigabit Ethernet Controller" (8086:107c, BDF
>>>>         
>> 01:03.0).
>>     
>>>> Probing for ST M25P10.RES, 128 KB: probe_spi_res1: id 0x10
>>>> Chip status register is 00
>>>> Found chip "ST M25P10.RES" (128 KB, SPI) at physical address 0xfffe0000.
>>>> Reading old flash chip contents...
>>>> Erasing and writing flash chip... Looking at blockwise erase function
>>>>         
>> 0...
>>     
>>>> trying... 0x000000-0x007fff:W, 0x008000-0x00ffff:W, 0x010000-0x017fff:W,
>>>> 0x018000-0x01ffff:S
>>>> Done.
>>>> Verifying flash... VERIFY FAILED at 0x00008000! Expected=0xc6,
>>>>         
>> Read=0x15,
>>     
>>>> failed byte count from 0x00000000-0x0001ffff: 0x8d75
>>>>
>>>>
>>>>         
>>> Crap. Could you please retest http://patchwork.coreboot.org/patch/2155/?
>>>
>>> If that one works, I am tempted to commit it, and send all other changes
>>> as separate patches on top.
>>>
>>>       
>> Idwer tested this patch together with patch 2155
>>     
>
>
> No. That was 2160, like you asked me to test on IRC.
>   

Indeed, you tested patch 2160 (partial write) together with patch 2188
(fix offset calculation).


>> from patchwork and it
>> worked.
>> http://paste.flashrom.org/view.php?id=138
>>     

Sorry for the confusion.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/