March 2018 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add support for Atmel / Adesto AT25SF041 SPI flash chip
by Nico Huber (Code Review) 09 Mar '18

09 Mar '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/25047 ) Change subject: Add support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Patch Set 4: (3 comments) Before I continue the review: Is this really the version you tested? You said something about a copy of AT25DF041A on IRC, but this looks like a copy of AT26DF041 which doesn't seem to make much sense. https://review.coreboot.org/#/c/25047/4//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/25047/4//COMMIT_MSG@9 PS4, Line 9: Reasonably certain this is working due to erase/read/write hardware-tests in a setup in which the flash content contained a running FPGA program. Meaningful content was stored on the chip beforehand. I read a.rom, erased, observed the FPGA dead, read b.rom, wrote back a.rom, observed the FPGA alive again. I verified b.rom in a hex editor contained 0xff only. : The FPGA data used only a small portion of the address space, so I also wrote out a file with specific 3 bytes in the first and last memory page. All flashrom verify cycles passed. Line length limit is 72 chars. I also don't see a reason for the whole story. Just stating that probe/read/erase/write was tested should be enough. https://review.coreboot.org/#/c/25047/4/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/25047/4/flashchips.c@2194 PS4, Line 2194: .block_erase = spi_block_erase_81, Can't find anything about it in the datasheet. https://review.coreboot.org/#/c/25047/4/flashchips.c@2197 PS4, Line 2197: .block_erase = spi_block_erase_50, 0x50 is EWSR (enable write status register) -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 4 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Fri, 09 Mar 2018 13:42:40 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add support for Atmel / Adesto AT25SF041 SPI flash chip
by build bot (Jenkins) (Code Review) 09 Mar '18

09 Mar '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25047 ) Change subject: Add support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Patch Set 4: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1179/ : SUCCESS -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 4 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 09 Mar 2018 11:31:29 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: Add support for Atmel / Adesto AT25SF041 SPI flash chip
by Julian von Mendel (Code Review) 09 Mar '18

09 Mar '18

Hello Paul Menzel, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/25047 to look at the new patch set (#4). Change subject: Add support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Add support for Atmel / Adesto AT25SF041 SPI flash chip Reasonably certain this is working due to erase/read/write hardware-tests in a setup in which the flash content contained a running FPGA program. Meaningful content was stored on the chip beforehand. I read a.rom, erased, observed the FPGA dead, read b.rom, wrote back a.rom, observed the FPGA alive again. I verified b.rom in a hex editor contained 0xff only. The FPGA data used only a small portion of the address space, so I also wrote out a file with specific 3 bytes in the first and last memory page. All flashrom verify cycles passed. Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Signed-off-by: Julian von Mendel <git(a)jinvent.de> Signed-off-by: jvm <git(a)jinvent.de> --- M flashchips.c M flashchips.h 2 files changed, 33 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/25047/4 -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 4 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: Support for Atmel / Adesto AT25SF041 SPI flash chip
by Julian von Mendel (Code Review) 09 Mar '18

09 Mar '18

Julian von Mendel has removed a vote on this change. Change subject: Support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Removed Code-Review+1 by Julian von Mendel <git(a)jinvent.de> -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 3 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: Support for Atmel / Adesto AT25SF041 SPI flash chip
by Paul Menzel (Code Review) 09 Mar '18

09 Mar '18

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/25047 ) Change subject: Support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Patch Set 3: I think, the commit authors normally doesn’t score their own changes. -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 3 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 09 Mar 2018 08:44:16 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add "gingerly" flashing mode for the unreliable ISP environments
by Nico Huber (Code Review) 08 Mar '18

08 Mar '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23840 ) Change subject: Add "gingerly" flashing mode for the unreliable ISP environments ...................................................................... Patch Set 2: > However, even with such a dangerous boards in the existence, ISP is > still much safer than desoldering a flash chip without an experienced > steady hand - that's why I believe the advanced modes for ISP flashing, > be it that "gingerly" mode or any other future inventions, have a right > to exist No need to argue about that. It just has to be done right. And in a way that doesn't encourage users to do stupid things. >>>> And that is where the problems start... if it can fail, >>>> we have to handle the failure correctly, otherwise >>>> flashrom (in its current implementation) would probably >>>> fall back to erase the whole chip and make things worse. >>> >>> If there is a timeout: we could just print some error message and >>> shutdown the flashrom, then it shouldn't do anything else... also, >>> why would it try to erase the whole chip if we didn't specify such >>> an operation? >> >> Flashrom defaults to try another erase function if one fails, >> finally using one that erases the whole chip. Can be handled >> more gracefully ofc, just something that we have to keep in >> mind. >> > > Perhaps there should be a way to just exit a flashrom with some message > without trying any other erase functions. Maybe by adding a new return > value to the erase functions ( perhaps "2" or "-2" if there is already > "1" or "-1" code in the failure case ) and adjusting the flashrom > algorithm to act accordingly if such a return value has been accepted > from erase function Yes, there is already a distinction in return values. One just has to choose the right one. Maybe it is already safe, I just wanted to make sure we don't forget about it ;) >>> Also, if we'd introduce a timeout - it shouldn't be hardcoded. In >>> example: if I'd hardcode it to 5 seconds - it will block a way for >>> the boards whose chip is available for 1 second per each 10 seconds; >>> hardcode to 15 seconds - blocks those which are 1sec/20sec available, >>> and so on >>> >>> Most likely we'll have to introduce a new flag "-t | --timeout >>> <seconds>" ("t" is also not occupied) but lets' have more discussions >>> to clarify our further steps >> >> I would prefer a static timeout, doesn't have to be a small one. >> For instance 60s, then bail out. It's not that bad to wait 1min >> but if single hunks take more than 60s, nobody would have the >> patience to wait for the full run anyway. >> > > Is it possible that there are some boards with a lengthy > available/unavailable durations, like " 70 seconds unavailable / 30 > seconds available " ? If yes, the full run would still could take a > reasonably small time despite the long wait between each two groups of > many successful attempts Anything is possible with a broken hardware setup... how about we start with a static timeout and discuss an option for it in a follow- up commit? -- To view, visit https://review.coreboot.org/23840 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3f18276d9fb7233d082720cb29d154f31c77100 Gerrit-Change-Number: 23840 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 23:06:33 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Support for Atmel / Adesto AT25SF041 SPI flash chip
by Julian von Mendel (Code Review) 08 Mar '18

08 Mar '18

Julian von Mendel has posted comments on this change. ( https://review.coreboot.org/25047 ) Change subject: Support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Patch Set 3: Code-Review+1 I did function tests with read/write/erase, no systematic flash performance tests or similar. I had a Pi connected to the SPI flash chip, that contained FPGA configuration data (that were already on the chip). I read the data, erased, read that as well, confirmed that both images made sense in a hex editor and that the FPGA did no longer configure. I wrote back the original image and the FPGA worked as it did before. No errors during flashrom verify. (Possible flaw in that test: Only a small portion of the address space was filled with non-zero data.) Sorry about the comments in the .c file, I am not sure about these (they were copied from AT26DF041). -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 3 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 21:54:35 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: Support for Atmel / Adesto AT25SF041 SPI flash chip
by Paul Menzel (Code Review) 08 Mar '18

08 Mar '18

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/25047 ) Change subject: Support for Atmel / Adesto AT25SF041 SPI flash chip ...................................................................... Patch Set 3: (2 comments) https://review.coreboot.org/#/c/25047/3//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/25047/3//COMMIT_MSG@7 PS3, Line 7: Support for Atmel / Adesto AT25SF041 SPI flash chip Maybe: > Add support for … https://review.coreboot.org/#/c/25047/3//COMMIT_MSG@8 PS3, Line 8: Tested how? -- To view, visit https://review.coreboot.org/25047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0be930ff2258300508398e12fbe5abe10400fea2 Gerrit-Change-Number: 25047 Gerrit-PatchSet: 3 Gerrit-Owner: Julian von Mendel <git(a)jinvent.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 19:38:20 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add "gingerly" flashing mode for the unreliable ISP environments
by Mike Banon (Code Review) 08 Mar '18

08 Mar '18

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23840 ) Change subject: Add "gingerly" flashing mode for the unreliable ISP environments ...................................................................... Patch Set 2: > >> ... though, as you already work with two masters on the > >> SPI bus, did you try to boot the SoC up and halt the OS? > > > > Maybe it is possible to put this SoC to reset from the firmware, > > but we need a universal method to read/flash the firmware without > > relying on what is already flashed at the router's flash chip: in > > example, what if my router is bricked and its' firmware cannot be > > loaded to halt a router? > > Universal method: isolate VCC (e.g. desolder flash chip, or put a > diode on the VCC). > But that wouldn't be ISP method ;) And many people are afraid of soldering their densely packed boards and would like to avoid this at all costs - which is reasonable! My friend, who is quite experienced at soldering, accidentally broke a copper track while desoldering a SOIC8 chip from his coreboot board - and still couldn't repair :( > > > > > I can't see how this new mode could be more dangerous than a > normal > > operation; also, dangerous to what - software or hardware? > > Hardware. It encourages users to operate flashrom with multiple > masters on the SPI bus. Which is generally not supported by hard- > ware (it depends on how the masters drive their outputs, e.g. > open-drain vs. push-pull). If you do it nevertheless and flashrom > only runs for 1s and bails out, that is much safer than shorting > the master's outputs for a longer period. > > That it works for you and you haven't broken anything yet tells us > nothing about other possible hardware setups. > Sometimes the whole ISP thing can be very dangerous, even without any extra flashrom modes! This week I connected CH341A USB programmer through a test clip to SOIC8 flash chip at HD capture board - of course without any power adapters connected to a board or a clip. Then I plugged CH341A to the laptop's USB port and 1 second later pressed the "Enter" key for " sudo ./flashrom -p ch341a_spi -V " command - without any gingerly mode as you see, just a quick probe! But the flashrom did not detect a programmer, about 5 seconds later I sensed a horrible burn smell and instantly disconnected everything, almost shitting my pants in the process ;) This smell definitely came from CH341A, because the HD capture board still works 100% OK and at CH341A's chip label almost all letters color got dark from the heat: only "WC" letters are of the original light-grey color, the rest "H CH341A" letters got very dark. The most wonderful thing is that CH341A turned out to be very resilient and is still working perfectly! :) It seems this hungry board tried to eat too much current through CH341A while trying to power itself! Could have killed a Bus Pirate or another more-advanced-but-less-resilient programmer without any protection against such things. (maybe would repost this experience to the mailing lists) However, even with such a dangerous boards in the existence, ISP is still much safer than desoldering a flash chip without an experienced steady hand - that's why I believe the advanced modes for ISP flashing, be it that "gingerly" mode or any other future inventions, have a right to exist > > > >> And... this might be the biggest issue: the possible > >> endless loop in spi_rw_gingerly(). For a mergeable solution, > >> you'd have to put some kind of timeout there > > > > Yes, it will hang a flashrom if this chip never becomes available > > even for a split second; but I don't see it as a problem: the > user > > can terminate flashrom / restart his hardware programmer and > > understand that perhaps even with this mode its' impossible to > read > > a firmware of his board and the desoldering of a chip can't be > > avoided with ISP mode > > I would generally agree, but all that without a progress indi- > cation? How do you tell after 30min if it's nearly done or didn't > get anywhere so far? > Currently, if we'd run it with -V, if there are any successful "probing for" messages - we could continue waiting and the whole process will be successful. I agree with you that we need to come up with a better indication of how much percentage of a chip has been done, although perhaps this feature could be added independently for the whole flashrom modes - will be more informative than e.g. a current simple "Reading flash" message, which in case of a turtle speed chips like KB9012 (about 15 minutes to read) without "-V" flag and any percentage printed - almost gives an impression that a flashrom is stuck :P > > > >> And that is where the problems start... if it can fail, > >> we have to handle the failure correctly, otherwise > >> flashrom (in its current implementation) would probably > >> fall back to erase the whole chip and make things worse. > > > > If there is a timeout: we could just print some error message and > > shutdown the flashrom, then it shouldn't do anything else... > also, > > why would it try to erase the whole chip if we didn't specify > such > > an operation? > > Flashrom defaults to try another erase function if one fails, > finally using one that erases the whole chip. Can be handled > more gracefully ofc, just something that we have to keep in > mind. > Perhaps there should be a way to just exit a flashrom with some message without trying any other erase functions. Maybe by adding a new return value to the erase functions ( perhaps "2" or "-2" if there is already "1" or "-1" code in the failure case ) and adjusting the flashrom algorithm to act accordingly if such a return value has been accepted from erase function > > > > Also, if we'd introduce a timeout - it shouldn't be hardcoded. In > > example: if I'd hardcode it to 5 seconds - it will block a way > for > > the boards whose chip is available for 1 second per each 10 > seconds; > > hardcode to 15 seconds - blocks those which are 1sec/20sec > available, > > and so on > > > > Most likely we'll have to introduce a new flag "-t | --timeout > > <seconds>" ("t" is also not occupied) but lets' have more > discussions > > to clarify our further steps > > I would prefer a static timeout, doesn't have to be a small one. > For instance 60s, then bail out. It's not that bad to wait 1min > but if single hunks take more than 60s, nobody would have the > patience to wait for the full run anyway. > Is it possible that there are some boards with a lengthy available/unavailable durations, like " 70 seconds unavailable / 30 seconds available " ? If yes, the full run would still could take a reasonably small time despite the long wait between each two groups of many successful attempts > > > > >> Last but not least, why do it at the SPI level? Retries > >> due to unreliable connections should be handled at a higher > >> level, IMO. > > > > Sorry, I could not see a way to do this low level verification of > > each chunk at the higher level than the same level where these > > individual chunks are being read/written > > spi_write_chunked() and spi_read_chunked() can be called with > huge ranges, yes. But they don't have to be. You could just > generate smaller hunks at a higher level. For instance, you > could replace the flash chip's read() and write() pointers > with implementations that produce smaller hunks and "gingerly" > run the original read/write functions on them. > This is a good idea. Perhaps I would implement it after we'd discuss other issues with this "gingerly" mode and its' fate would become more clear -- To view, visit https://review.coreboot.org/23840 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3f18276d9fb7233d082720cb29d154f31c77100 Gerrit-Change-Number: 23840 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 19:02:24 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add "gingerly" flashing mode for the unreliable ISP environments
by Nico Huber (Code Review) 08 Mar '18

08 Mar '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23840 ) Change subject: Add "gingerly" flashing mode for the unreliable ISP environments ...................................................................... Patch Set 2: >> ... though, as you already work with two masters on the >> SPI bus, did you try to boot the SoC up and halt the OS? > > Maybe it is possible to put this SoC to reset from the firmware, > but we need a universal method to read/flash the firmware without > relying on what is already flashed at the router's flash chip: in > example, what if my router is bricked and its' firmware cannot be > loaded to halt a router? Universal method: isolate VCC (e.g. desolder flash chip, or put a diode on the VCC). > >> About this implementation, when I read "gingerly" I >> expected something more sophisticated (e.g. some sort of >> collision avoidance, listening on the bus instead of >> jamming it). Though, I have nothing against the way >> you do it, I would call it brute-force. > > Could be renamed of course! "gingerly" was a temporary name I've > chosen partially because it starts with unoccupied 'g' letter while > giving an impression of what we're trying to do. If you'd like I > could rename this mode to "-b | --brute-force" ("b" isn't occupied > too) while freeing a "gingerly" name for the something more > significant Rarely used options shouldn't get a single letter anyway. And in this case, I would even go for something like --dangerous-brute-force > >> Also, if I call `flashrom --gingerly` I would expect >> something that is safer to use, but it kind of is >> the opposite, isn't it? > > I can't see how this new mode could be more dangerous than a normal > operation; also, dangerous to what - software or hardware? Hardware. It encourages users to operate flashrom with multiple masters on the SPI bus. Which is generally not supported by hard- ware (it depends on how the masters drive their outputs, e.g. open-drain vs. push-pull). If you do it nevertheless and flashrom only runs for 1s and bails out, that is much safer than shorting the master's outputs for a longer period. That it works for you and you haven't broken anything yet tells us nothing about other possible hardware setups. OTOH, looking beyond your multiple master use case, the same ap- proach can be used when you just have an unreliable connection. > >> >> I don't like to clutter up spi_read_chunked() and >> spi_write_chunked(), most changes to them seem to be there >> only to avoid malloc() in your spi_rw_gingerly()? >> I would rather move it there. >> > > I agree with you, but can't see a good way of how this could be > avoided. spi_rw_gingerly has to be called for each chunk, and right > now there are just two malloc's for two buffers which are > constantly being reused by spi_rw_gingerly. If I'd move the > malloc's inside the "spi_rw_gingerly" - it will significantly raise > the frequency of malloc/free calls, reducing the efficiency of > flashrom. E.g. if we'd try to read a 4MB chip by those 256 byte > chunks, there would be at least 32768 extra malloc's and free's OMG, that would slow the whole process down by what? 10ms? maybe 100ms on an RPi gen1? sorry ;) Anyway, let's decide on the correct place for the overall imple- mentation first, see bottom. > >> And... this might be the biggest issue: the possible >> endless loop in spi_rw_gingerly(). For a mergeable solution, >> you'd have to put some kind of timeout there > > Yes, it will hang a flashrom if this chip never becomes available > even for a split second; but I don't see it as a problem: the user > can terminate flashrom / restart his hardware programmer and > understand that perhaps even with this mode its' impossible to read > a firmware of his board and the desoldering of a chip can't be > avoided with ISP mode I would generally agree, but all that without a progress indi- cation? How do you tell after 30min if it's nearly done or didn't get anywhere so far? > >> And that is where the problems start... if it can fail, >> we have to handle the failure correctly, otherwise >> flashrom (in its current implementation) would probably >> fall back to erase the whole chip and make things worse. > > If there is a timeout: we could just print some error message and > shutdown the flashrom, then it shouldn't do anything else... also, > why would it try to erase the whole chip if we didn't specify such > an operation? Flashrom defaults to try another erase function if one fails, finally using one that erases the whole chip. Can be handled more gracefully ofc, just something that we have to keep in mind. > > Also, if we'd introduce a timeout - it shouldn't be hardcoded. In > example: if I'd hardcode it to 5 seconds - it will block a way for > the boards whose chip is available for 1 second per each 10 seconds; > hardcode to 15 seconds - blocks those which are 1sec/20sec available, > and so on > > Most likely we'll have to introduce a new flag "-t | --timeout > <seconds>" ("t" is also not occupied) but lets' have more discussions > to clarify our further steps I would prefer a static timeout, doesn't have to be a small one. For instance 60s, then bail out. It's not that bad to wait 1min but if single hunks take more than 60s, nobody would have the patience to wait for the full run anyway. > >> Last but not least, why do it at the SPI level? Retries >> due to unreliable connections should be handled at a higher >> level, IMO. > > Sorry, I could not see a way to do this low level verification of > each chunk at the higher level than the same level where these > individual chunks are being read/written spi_write_chunked() and spi_read_chunked() can be called with huge ranges, yes. But they don't have to be. You could just generate smaller hunks at a higher level. For instance, you could replace the flash chip's read() and write() pointers with implementations that produce smaller hunks and "gingerly" run the original read/write functions on them. -- To view, visit https://review.coreboot.org/23840 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3f18276d9fb7233d082720cb29d154f31c77100 Gerrit-Change-Number: 23840 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 17:19:57 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2018