Nico Huber posted comments on this change.

View Change

Patch set 5:

I ended up re-writing a significant portion of this patch. Assuming
the changes look good, what's the "right" thing to do to get things
merged? Merge everything in this patch, or split my updates into a
follow-up patch?

I think the changes should be done right away in this patch. In this
case it wouldn't break anything in between two patches, but merging
things that you know need an update feels generally wrong (bisect
argument). The actual question we should ask is, who should do the
changes? In this case, I would have asked Keno in advance if he wants
to amend it (don't know if you did).

Back to the patch: You can ignore all the new comments and nits,
looks quite good actually :) But... I really care about the change to
the spi_read_chunked() call.

(5 comments)

• File linux_spi.c:

  • Patch Set #3, Line 199: The implementation currently does not support requests that

    Re "why": As to not rely on the driver to split large transactions? I'm not

    No, I meant in the kernel. And the command is never written into
    the rx buffer anyway. Actually it seems that there is never a buffer
    that holds both tx and rx data throughout the whole stack starting
    with flashrom.

• File linux_spi.c:

  • Patch Set #5, Line 136: msg_pwarn("%s: Cannot open %s: %s.\n", __func__, BUF_SIZE_FROM_SYSFS, strerror(errno));

    I was about to say, "function names in warnings?". But hey, we do it
    all over the place...

  • Patch Set #5, Line 143: msg_pwarn("%s: Cannot read %s: %s.\n", __func__, BUF_SIZE_FROM_SYSFS, strerror(errno));

    Nit, in the unlikely event that we are presented an empty file,
    we'd print:

        Cannot read /sys/module/spidev/parameters/bufsiz: Success.

    i.e. EOF is not an error :-/

  • Patch Set #5, Line 154: max_kernel_buf_size = (size_t)tmp;

    Nit, possible overflow. Very theoretic, don't know any environment
    where sizeof(size_t) < sizeof(long).

  • Patch Set #5, Line 163: max_kernel_buf_size = (size_t)getpagesize();

    getpagesize() is kind of deprecated, but we've used it before anyway.

To view, visit change 22344. To unsubscribe, visit settings.