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

View Change

To view, visit change 23840. To unsubscribe, or for help writing mail filters, visit settings.