Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(3 comments)
I like the USB code and its potential to unify programmer parameters. It belongs into a separate patch, though, to ease review.
Ah I squashed it in so this is a functional bit of code upon it's own merits. Maybe we can separate out in the final round?
Final round of what? This review here? Reviewing two patches of 500 lines each usually goes twice as fast as reviewing a big 1k line one... Gerrit is a great tool to assist with reviewing, but that doesn't help if patches aren't organized for review.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@154 PS3, Line 154: * maximum USB packet size for the device.
Do you have a suggested fix, Nico?
Well, the basic issue seems to be that programmers don't always have max read/write sizes, but a max transfer size instead. I would simply add a field for that. There are only five lines consuming `max_data_(read|write)` in the whole flashrom code, that would need to be adapted.
Maybe make them use helper functions like:
size_t spi_max_read(spi, header_size);
that would interpret the fields in `spi` and subtract `header_size` in case. The tricky part might be to figure out the exact `header_size`, but if we are allowed to make educated guesses here in programmer code, we could as well there.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@157 PS3, Line 157: command header maximum size
Sorry I wasn't clear on the expectation on what you wish me to change here if anything?
The paragraph is technically wrong. I would drop it or tell the truth. Or get rid of the entire problem (see above).
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I brought up the same point in the cros/flashrom tree. I very much agree. Can it be it's own patch though or do I need to fix this first in our tree then rebase this patch from our tree again? It's hard to juggle two trees when my goal is to make one go away in the end.
Edward, if you see issues in the cros tree, please always fix them ahead. It's a huge waste of time for the flashrom community to review bad code, especially when the submitter already knows that it's in bad shape.
About patch juggling. I would simply write new commits for upstreaming, with all known issues already addressed. Once it's merged, push a commit with the resulting diff downstream. If the patches are not in good shape for upstream, it just wastes resources on both sides (mostly of unpaid individuals, but I don't see how Google could gain anything from ugly reviews either).
Edward is doing flashrom community a great service by upstreaming this work.
Damien, can you please explain this claim. Who is this flashrom community? FWIW, it's the exact opposite with the chromium-fork upstreaming: Nobody helps the community with review resources, no help to get better automated build tests running. Instead, most remaining resources are drawn towards the upstreaming, which stalls more urgent flashrom work and delays new releases.
Can we please accommodate that the first check-in will not be perfect? We need to realise that his aim is to remove a whole fork so others can work upstream.
Again, please explain what this opinion is based on. Who are these "others"? Who said that they want to work upstream, which would include helping with the project and not bluntly congesting it. I've seen no sign so far, that things could remotely get better.
As stated, he can submit further patches to fix this small problem, but please don't block new functionality from entering the tree over some minor detail.
Sorry, I've moved past that. Promises of future work have turned into the opposite so far. Plus, Edward has just shown that he won't even fix serious regressions.
And, wtf, potential build breakage is a minor detail now?
NB. I wanted to continue review on this today. Now you are drawing resources too.