| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1411464267.18580.46.camel@iivanov-dev>
Date: Tue, 23 Sep 2014 12:24:27 +0300
From: "Ivan T. Ivanov" <iivanov@...sol.com>
To: Andy Gross <agross@...eaurora.org>
Cc: Mark Brown <broonie@...nel.org>,
Bjorn Andersson <bjorn.andersson@...ymobile.com>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-spi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] spi: qup: Fix incorrect block transfers
Hi Andy,
On Sun, 2014-09-21 at 23:27 -0500, Andy Gross wrote:
> This patch fixes a number of errors with the QUP block transfer mode. Errors
> manifested themselves as input underruns, output overruns, and timed out
> transactions.
At what speeds are you seeing those errors?
>
> The block mode does not require the priming that occurs in FIFO mode. At the
> moment that the QUP is placed into the RUN state, the QUP may immediately raise
> an interrupt if the request is a write. Therefore, there is no need to prime
> the pump.
>
> In addition, the block transfers require that whole blocks of data are
> read/written at a time. The last block of data that completes a transaction may
> contain less than a full blocks worth of data.
Does this mean that block transfer will start only if the required
bytes from block is written into buffer?
>
> Each block of data results in an input/output service interrupt accompanied with
> a input/output block flag set. Additional block reads/writes require clearing
> of the service flag. It is ok to check for additional blocks of data in the
> ISR, but you have to ack every block you transfer. Imbalanced acks result in
> early return from complete transactions with pending interrupts that still have
> to be ack'd. The next transaction can be affected by these interrupts.
>
> Signed-off-by: Andy Gross <agross@...eaurora.org>
> ---
> drivers/spi/spi-qup.c | 194 +++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 141 insertions(+), 53 deletions(-)
>
<snip>
> +static void qup_fill_read_buffer(struct spi_qup *controller,
> + struct spi_transfer *xfer, u32 data)
Please, could prefix this whit spi_ to be consistent with the
rest of the code.
> {
> u8 *rx_buf = xfer->rx_buf;
> - u32 word, state;
> - int idx, shift, w_size;
> -
> - w_size = controller->w_size;
> -
> - while (controller->rx_bytes < xfer->len) {
> -
> - state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> - if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> - break;
> + int idx, shift;
> + int read_len = min_t(int, xfer->len - controller->rx_bytes,
> + controller->w_size);
You should not need this check here. xfer->len is multiple of controller->w_size
and you always read one word at time.
>
> - word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
> -
> - if (!rx_buf) {
> - controller->rx_bytes += w_size;
> - continue;
> - }
> -
> - for (idx = 0; idx < w_size; idx++, controller->rx_bytes++) {
> + if (rx_buf)
> + for (idx = 0; idx < read_len; idx++) {
> /*
> * The data format depends on bytes per SPI word:
> * 4 bytes: 0x12345678
> @@ -229,40 +218,129 @@ static void spi_qup_fifo_read(struct spi_qup *controller,
> * 1 byte : 0x00000012
> */
> shift = BITS_PER_BYTE;
> - shift *= (w_size - idx - 1);
> - rx_buf[controller->rx_bytes] = word >> shift;
> + shift *= (controller->w_size - idx - 1);
> + rx_buf[controller->rx_bytes + idx] = data >> shift;
> }
> - }
> +
> + controller->rx_bytes += read_len;
> }
>
> -static void spi_qup_fifo_write(struct spi_qup *controller,
> - struct spi_transfer *xfer)
> +static void qup_prepare_write_data(struct spi_qup *controller,
> + struct spi_transfer *xfer, u32 *data)
> {
> const u8 *tx_buf = xfer->tx_buf;
> - u32 word, state, data;
> - int idx, w_size;
> + u32 val;
> + int idx;
> + int write_len = min_t(int, xfer->len - controller->tx_bytes,
> + controller->w_size);
>
Same here.
> - w_size = controller->w_size;
> + *data = 0;
>
> - while (controller->tx_bytes < xfer->len) {
> + if (tx_buf)
> + for (idx = 0; idx < write_len; idx++) {
> + val = tx_buf[controller->tx_bytes + idx];
> + *data |= val << (BITS_PER_BYTE * (3 - idx));
> + }
>
> - state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> - if (state & QUP_OP_OUT_FIFO_FULL)
> - break;
> + controller->tx_bytes += write_len;
> +}
>
> - word = 0;
> - for (idx = 0; idx < w_size; idx++, controller->tx_bytes++) {
> +static void spi_qup_service_block(struct spi_qup *controller,
> + struct spi_transfer *xfer, bool is_read)
> +{
Please, could you split this function to read and write, so we can use:
spi_qup_fifo_read() and spi_qup_fifo_write() in FIFO modes and
spi_qup_block_read() and spi_qup_block_write() for BLOCK mode.
> + u32 data, words_per_blk, num_words, ack_flag, op_flag;
> + int i;
> +
> + if (is_read) {
> + op_flag = QUP_OP_IN_BLOCK_READ_REQ;
> + ack_flag = QUP_OP_IN_SERVICE_FLAG;
> + num_words = DIV_ROUND_UP(xfer->len - controller->rx_bytes,
> + controller->w_size);
Same here and below.
> + words_per_blk = controller->in_blk_sz >> 2;
> + } else {
> + op_flag = QUP_OP_OUT_BLOCK_WRITE_REQ;
> + ack_flag = QUP_OP_OUT_SERVICE_FLAG;
> + num_words = DIV_ROUND_UP(xfer->len - controller->tx_bytes,
> + controller->w_size);
> + words_per_blk = controller->out_blk_sz >> 2;
> + }
Thank you.
Ivan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists