Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.

View Change

2 comments:

• File buspirate_spi.c:

  • Patch Set #2, Line 167: struct buspirate_spi_data *buspirate_data = data;

    I can probably create bp_commbuf as a local variable, alloc at the beginning and free at the end.

    Absolutely, yes. As DEFAULT_BUFSIZE is small, it could also live on the
    stack in most cases.

    bp_commbufsize is different, I can see why it needs to be in a spi data in flashctx.

    AFAICT, it's only there to track the size of `bp_commbuf`. So they should
    share a common fate.

    I guess Angel is right, somebody tried to optimize things. Not in any way
    that would gain us something, though. malloc() and free() are not expensive,
    realloc() may be for bigger sizes. Compared to a single byte transferred
    over a serial line, it's still nothing.

    There's a third alternative. The size is eventually limited by what the
    v2 command supports. Somewhere below max_data_read/write is given. We
    could just allocate the maximum during init (256 + 5 + 2048 + 1). Define
    this as BUFFER_SIZE and let the command functions check that their
    parameters fit. Or a combination: use the stack during init, shutdown,
    command_v1 and only allocate the buffer if we use command_v2.

  • Patch Set #2, Line 325: int bp_commbufsize = 0;

    We could declare it as follows

        /* 16 bytes data, 3 bytes control. */
        unsigned char commbuf[16 + 3];

    Then use `sizeof(commbuf)` instead of DEFAULT_BUFSIZE below.

    Something similar could be done to buspirate_shutdown(). All other
    users of the `bp_commbuf` use buspirate_commbuf_grow() and can use
    malloc()/free() instead. Hmm, maybe the v1 function should use the
    stack as well?

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