* [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE @ 2009-09-27 5:26 Ben Nizette 2009-09-28 7:12 ` Haavard Skinnemoen 0 siblings, 1 reply; 4+ messages in thread From: Ben Nizette @ 2009-09-27 5:26 UTC (permalink / raw) To: hskinnemoen; +Cc: spi-devel-general, Linux Kernel list, kernel, dbrownell If len > BUFFER_LEN and !xfer->rx_buf we end up calculating the tx buffer address as *tx_dma = xfer->tx_dma + xfer->len - BUFFER_SIZE; which is constant; i.e. we just send the last BUFFER_SIZE data over again until we've reached the right number of bytes. This patch gets around this by using the /requested/ length when calculating addresses. Note there's no way len != *plen when we calculate the rx buffer address but conceptually we should be using *plen and I don't want someone to come through later, see the calculations for rx and tx are different and "clean up" back to what we had. Signed-off-by: Ben Nizette <bn@niasdigital.com> --- drivers/spi/atmel_spi.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c index f5b3fdb..8ce70cb 100644 --- a/drivers/spi/atmel_spi.c +++ b/drivers/spi/atmel_spi.c @@ -189,14 +189,14 @@ static void atmel_spi_next_xfer_data(struct spi_master *master, /* use scratch buffer only when rx or tx data is unspecified */ if (xfer->rx_buf) - *rx_dma = xfer->rx_dma + xfer->len - len; + *rx_dma = xfer->rx_dma + xfer->len - *plen; else { *rx_dma = as->buffer_dma; if (len > BUFFER_SIZE) len = BUFFER_SIZE; } if (xfer->tx_buf) - *tx_dma = xfer->tx_dma + xfer->len - len; + *tx_dma = xfer->tx_dma + xfer->len - *plen; else { *tx_dma = as->buffer_dma; if (len > BUFFER_SIZE) -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE 2009-09-27 5:26 [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE Ben Nizette @ 2009-09-28 7:12 ` Haavard Skinnemoen 2009-09-29 0:55 ` Ben Nizette 0 siblings, 1 reply; 4+ messages in thread From: Haavard Skinnemoen @ 2009-09-28 7:12 UTC (permalink / raw) To: Ben Nizette Cc: hskinnemoen, spi-devel-general, Linux Kernel list, kernel, dbrownell Ben Nizette <bn@niasdigital.com> wrote: > If len > BUFFER_LEN and !xfer->rx_buf we end up calculating the tx > buffer address as > > *tx_dma = xfer->tx_dma + xfer->len - BUFFER_SIZE; > > which is constant; i.e. we just send the last BUFFER_SIZE data over > again until we've reached the right number of bytes. > > This patch gets around this by using the /requested/ length when > calculating addresses. > > Note there's no way len != *plen when we calculate the rx buffer address > but conceptually we should be using *plen and I don't want someone to > come through later, see the calculations for rx and tx are different and > "clean up" back to what we had. > > Signed-off-by: Ben Nizette <bn@niasdigital.com> Wow, that is subtle. I had to stare at it for a long while before I understood what's going on, but I believe you're right. Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com> While you're at it, could you send another patch renaming 'len' to 'next_len'? I think that would make it a bit more obvious why your patch is correct and prevent similar mistakes in the future. Haavard ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE 2009-09-28 7:12 ` Haavard Skinnemoen @ 2009-09-29 0:55 ` Ben Nizette 2009-09-29 6:57 ` Haavard Skinnemoen 0 siblings, 1 reply; 4+ messages in thread From: Ben Nizette @ 2009-09-29 0:55 UTC (permalink / raw) To: Haavard Skinnemoen Cc: hskinnemoen, spi-devel-general, Linux Kernel list, kernel, dbrownell On Mon, 2009-09-28 at 09:12 +0200, Haavard Skinnemoen wrote: > Wow, that is subtle. I had to stare at it for a long while before I > understood what's going on, but I believe you're right. Prolly just means my changelog was crappy ;-) > > Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com> > > While you're at it, could you send another patch renaming 'len' to > 'next_len'? I think that would make it a bit more obvious why your > patch is correct and prevent similar mistakes in the future. Good plan, below. Thx, --Ben. > > Haavard ---8<--- From: Ben Nizette <bn@niasdigital.com> Subject: [PATCH] atmel_spi: make "len" variable name less ambiguous in dma addr calculation "[PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE" fixed a bug where the "len" variable in atmel_spi_next_xfer_data() was taken to be the total number of bytes remaining in the transfer but it actually represents the number of bytes which will be sent this dma chunk. While the 2 will be the same if there is less than 1 chunk to go (or if we aren't using a scratch buffer and therefore aren't breaking the transfers in to chunks), they won't be the same in general. s/len/next_len to reflect the true nature and usage of this variable. Signed-off-by: Ben Nizette <bn@niasdigital.com> --- drivers/spi/atmel_spi.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c index 8ce70cb..5d94fca 100644 --- a/drivers/spi/atmel_spi.c +++ b/drivers/spi/atmel_spi.c @@ -185,28 +185,28 @@ static void atmel_spi_next_xfer_data(struct spi_master *master, u32 *plen) { struct atmel_spi *as = spi_master_get_devdata(master); - u32 len = *plen; + u32 next_len = *plen; /* use scratch buffer only when rx or tx data is unspecified */ if (xfer->rx_buf) *rx_dma = xfer->rx_dma + xfer->len - *plen; else { *rx_dma = as->buffer_dma; - if (len > BUFFER_SIZE) - len = BUFFER_SIZE; + if (next_len > BUFFER_SIZE) + next_len = BUFFER_SIZE; } if (xfer->tx_buf) *tx_dma = xfer->tx_dma + xfer->len - *plen; else { *tx_dma = as->buffer_dma; - if (len > BUFFER_SIZE) - len = BUFFER_SIZE; - memset(as->buffer, 0, len); + if (next_len > BUFFER_SIZE) + next_len = BUFFER_SIZE; + memset(as->buffer, 0, next_len); dma_sync_single_for_device(&as->pdev->dev, - as->buffer_dma, len, DMA_TO_DEVICE); + as->buffer_dma, next_len, DMA_TO_DEVICE); } - *plen = len; + *plen = next_len; } /* -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE 2009-09-29 0:55 ` Ben Nizette @ 2009-09-29 6:57 ` Haavard Skinnemoen 0 siblings, 0 replies; 4+ messages in thread From: Haavard Skinnemoen @ 2009-09-29 6:57 UTC (permalink / raw) To: Ben Nizette Cc: dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f, hskinnemoen-AIFe0yeh4nAAvxtiuMwx3w, kernel, Kernel list, Linux, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f Ben Nizette <bn-pV1zxKMKwgg3AZtZ2NlBNQ@public.gmane.org> wrote: > From: Ben Nizette <bn-pV1zxKMKwgg3AZtZ2NlBNQ@public.gmane.org> > Subject: [PATCH] atmel_spi: make "len" variable name less ambiguous in dma addr calculation > > "[PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE" > fixed a bug where the "len" variable in atmel_spi_next_xfer_data() was > taken to be the total number of bytes remaining in the transfer but it > actually represents the number of bytes which will be sent this dma > chunk. While the 2 will be the same if there is less than 1 chunk to go > (or if we aren't using a scratch buffer and therefore aren't breaking > the transfers in to chunks), they won't be the same in general. > > s/len/next_len to reflect the true nature and usage of this variable. > > Signed-off-by: Ben Nizette <bn-pV1zxKMKwgg3AZtZ2NlBNQ@public.gmane.org> Excellent, thanks. Acked-by: Haavard Skinnemoen <haavard.skinnemoen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> ------------------------------------------------------------------------------ Come build with us! The BlackBerry® Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9-12, 2009. Register now! http://p.sf.net/sfu/devconf ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-09-29 6:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-09-27 5:26 [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE Ben Nizette 2009-09-28 7:12 ` Haavard Skinnemoen 2009-09-29 0:55 ` Ben Nizette 2009-09-29 6:57 ` Haavard Skinnemoen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).