diff mbox

[5/5] spi: sirf: fix spi full-duplex DMA transferring issue

Message ID 1397457001-5266-6-git-send-email-21cnbao@gmail.com (mailing list archive)
State Accepted
Commit bf83fd6402a856eeb9a22c364c50ccf9bbdf9b17
Headers show

Commit Message

Barry Song April 14, 2014, 6:30 a.m. UTC
From: Qipan Li <Qipan.Li@csr.com>

sometimes t->tx can be equal with t->rx. for example, spidev will make
tx and rx point to spidev->buffer at the same time. currently, for this
case, we map the buffer BIDIRECTION to fix the cache consistency.

Signed-off-by: Qipan Li <Qipan.Li@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 drivers/spi/spi-sirf.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

Mark Brown April 14, 2014, 8:06 p.m. UTC | #1
On Mon, Apr 14, 2014 at 02:30:01PM +0800, Barry Song wrote:
> From: Qipan Li <Qipan.Li@csr.com>
> 
> sometimes t->tx can be equal with t->rx. for example, spidev will make
> tx and rx point to spidev->buffer at the same time. currently, for this
> case, we map the buffer BIDIRECTION to fix the cache consistency.

I've applied this but such usage is out of spec - do we have any drivers
doing this in mainline?
Barry Song April 15, 2014, 1:43 a.m. UTC | #2
2014-04-15 4:06 GMT+08:00 Mark Brown <broonie@kernel.org>:
> On Mon, Apr 14, 2014 at 02:30:01PM +0800, Barry Song wrote:
>> From: Qipan Li <Qipan.Li@csr.com>
>>
>> sometimes t->tx can be equal with t->rx. for example, spidev will make
>> tx and rx point to spidev->buffer at the same time. currently, for this
>> case, we map the buffer BIDIRECTION to fix the cache consistency.
>
> I've applied this but such usage is out of spec - do we have any drivers
> doing this in mainline?

i felt strange too at the first look from internal gerrit. qipan told
me there is one:
drivers/spi/spidev.c

static int spidev_message(struct spidev_data *spidev,
                struct spi_ioc_transfer *u_xfers, unsigned n_xfers)
{
buf = spidev->buffer;
..
k_tmp->rx_buf = buf;
...
k_tmp->tx_buf = buf;
...
spi_message_add_tail(k_tmp, &msg);

spidev_sync(spidev, &msg);
}

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 15, 2014, 9:26 a.m. UTC | #3
On Tue, Apr 15, 2014 at 09:43:24AM +0800, Barry Song wrote:

> i felt strange too at the first look from internal gerrit. qipan told
> me there is one:
> drivers/spi/spidev.c

OK, that needs fixing.  That code predates anyone doing DMA.
Barry Song April 15, 2014, 9:57 a.m. UTC | #4
2014-04-15 17:26 GMT+08:00 Mark Brown <broonie@kernel.org>:
> On Tue, Apr 15, 2014 at 09:43:24AM +0800, Barry Song wrote:
>
>> i felt strange too at the first look from internal gerrit. qipan told
>> me there is one:
>> drivers/spi/spidev.c
>
> OK, that needs fixing.  That code predates anyone doing DMA.

if it is out of specification, it seems fixing spidev.c is not enough
as spi transfer actually needs an precondition of tx!=rx now.

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 15, 2014, 10:12 a.m. UTC | #5
On Tue, Apr 15, 2014 at 05:57:39PM +0800, Barry Song wrote:
> 2014-04-15 17:26 GMT+08:00 Mark Brown <broonie@kernel.org>:

> > OK, that needs fixing.  That code predates anyone doing DMA.

> if it is out of specification, it seems fixing spidev.c is not enough
> as spi transfer actually needs an precondition of tx!=rx now.

Well, it's enough to avoid bugs.  Adding the validation to make sure
nobody introduces the problem again is nice too but isn't as urgent.
diff mbox

Patch

diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c
index 3c12f39..0c039d4 100644
--- a/drivers/spi/spi-sirf.c
+++ b/drivers/spi/spi-sirf.c
@@ -383,7 +383,8 @@  static int spi_sirfsoc_transfer(struct spi_device *spi, struct spi_transfer *t)
 		struct dma_async_tx_descriptor *rx_desc, *tx_desc;
 
 		sspi->dst_start = dma_map_single(&spi->dev,
-				sspi->rx, t->len, DMA_FROM_DEVICE);
+				sspi->rx, t->len, (t->tx_buf != t->rx_buf) ?
+				DMA_FROM_DEVICE : DMA_BIDIRECTIONAL);
 		rx_desc = dmaengine_prep_slave_single(sspi->rx_chan,
 			sspi->dst_start, t->len, DMA_DEV_TO_MEM,
 			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -391,7 +392,9 @@  static int spi_sirfsoc_transfer(struct spi_device *spi, struct spi_transfer *t)
 		rx_desc->callback_param = &sspi->rx_done;
 
 		sspi->src_start = dma_map_single(&spi->dev,
-				(void *)sspi->tx, t->len, DMA_TO_DEVICE);
+				(void *)sspi->tx, t->len,
+				(t->tx_buf != t->rx_buf) ?
+				DMA_TO_DEVICE : DMA_BIDIRECTIONAL);
 		tx_desc = dmaengine_prep_slave_single(sspi->tx_chan,
 			sspi->src_start, t->len, DMA_MEM_TO_DEV,
 			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);