On Thu, Feb 13, 2014 at 12:30:18AM +0800, Barry Song wrote:
> From: Qipan Li <Qipan.Li@xxxxxxx>
>
> there are many SPI clients which use the following protocal:
> step 1: send command bytes to clients(rx buffer is empty)
> step 2: send data bytes to clients or receive data bytes from
> clients.
The code here is a bit impenetrable, it could probably benefit from some
comments or possibly refactoring to split out the command based
transfers into a separate function so you end up with this:
> + if (t && t->tx_buf && !t->rx_buf && (t->len <= SIRFSOC_MAX_CMD_BYTES)) {
do_cmd_xfer();
else
do_normal_xfer();
> + const u32 *cmd_ptr;
> + u32 cmd;
> + sspi->tx_by_cmd = true;
> + cmd_ptr = sspi->tx;
> + cmd = *cmd_ptr;
> + if (sspi->word_width == 1 && !(spi->mode & SPI_LSB_FIRST))
> + cmd = cpu_to_be32(cmd) >>
> + ((SIRFSOC_MAX_CMD_BYTES - t->len) * 8);
> + if (sspi->word_width == 2 && t->len == 4 &&
> + (!(spi->mode & SPI_LSB_FIRST)))
> + cmd = ((cmd & 0xffff) << 16) | (cmd >> 16);
The above dererferences cmd_ptr no matter what the actual length is. If
it's 4 bytes that's fine but if it's less this means the code is reading
beyond the end of the buffer. Most likely this is going to be safe but
it's out of spec and could result in doing something that messes with
DMA mappings. A memcpy() would be safer.
This is also a bit unclear since the code is so densely written, more
whitespace would help, as would avoiding splitting the set of
assignments you use to avoid casts (but like I say they seem unsafe
anyway).
> + return t->len;
> + } else
> + sspi->tx_by_cmd = false;
You should have { } on either both sides or only one side of the if.
> + if (t && t->tx_buf && !t->rx_buf && (t->len <= SIRFSOC_MAX_CMD_BYTES))
> + regval |= (SIRFSOC_SPI_CMD_BYTE_NUM((t->len - 1)) |
> + SIRFSOC_SPI_CMD_MODE);
> + else
> + regval &= ~SIRFSOC_SPI_CMD_MODE;
This check is being repeated in two places which doesn't feel 100% safe,
why not use tx_by_cmd?
What the code is doing is basically fine but it's quite hard to read.
Attachment:
signature.asc
Description: Digital signature
