pxa2xx_spi: fix memory corruption

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/spi/pxa2xx_spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..ef38fbf 100644

drivers/spi/pxa2xx_spi.c +0 -0

 	drv_data->ioaddr = ssp->mmio_base;

Quoted 17 lines · Vasily Khoruzhick

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/spi/pxa2xx_spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..ef38fbf 100644
@@ -1569,7 +1569,7 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) master->transfer = transfer;
 

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) · line 1573

Quoted 4 lines

drivers/spi/pxa2xx_spi.c +1 -1

 	drv_data->ssp_type = ssp->type;
-	drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
+	drv_data->null_dma_buf = (u32 *)ALIGN(((u32)drv_data +
 						sizeof(struct driver_data)), 8);

This thing looks a bit disturbing in itself. Like, where the heck is that thing pointing in the end ? Since some data are written to address in "null_dma_buf" ... isn't this just changing the corruption impact ?

Quoted 21 lines · Marek Vasut

Quoted 20 lines · Vasily Khoruzhick

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---

 drivers/spi/pxa2xx_spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..ef38fbf 100644
@@ -1569,7 +1569,7 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) master->transfer = transfer;
 
 	drv_data->ssp_type = ssp->type;
 

Quoted 8 lines

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev)

Quoted 4 lines

drivers/spi/pxa2xx_spi.c +1 -1

-	drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
+	drv_data->null_dma_buf = (u32 *)ALIGN(((u32)drv_data +
 
 						sizeof(struct driver_data)), 8);

This thing looks a bit disturbing in itself. Like, where the heck is that thing pointing in the end ? Since some data are written to address in "null_dma_buf" ... isn't this just changing the corruption impact ?

        /* Allocate master with space for drv_data and null dma buffer */
        master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);

So there's 16 bytes at the end of driver_data.

However:

(u32)(drv_data + sizeof(struct driver_data))

is pointer arithmetic. drv_data points at an object of sizeof(struct driver_data). Adding one to this increments the pointer by sizeof(struct driver_data) bytes. So the above expression increments the pointer by sizeof(struct driver_data)*sizeof(struct driver_data) bytes, which is obviously complete rubbish.

((u32)drv_data + sizeof(struct driver_data))

casts drv_data to a u32 first, then adds the sizeof(struct driver_data) which moves us into the 16 bytes allocated off the end of the struct.

Quoted 46 lines · Russell King - ARM Linux

Quoted 24 lines · Marek Vasut

Quoted 23 lines · Vasily Khoruzhick

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---

 drivers/spi/pxa2xx_spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/spi/pxa2xx_spi.c +1 -1

index dc25bee..ef38fbf 100644
@@ -1569,7 +1569,7 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) master->transfer = transfer;
 
 	drv_data->ssp_type = ssp->type;
 
-	drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
+	drv_data->null_dma_buf = (u32 *)ALIGN(((u32)drv_data +
 

Quoted 7 lines

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) · line 1573

Quoted 1 line

drivers/spi/pxa2xx_spi.c +0 -0

 						sizeof(struct driver_data)), 8);

This thing looks a bit disturbing in itself. Like, where the heck is that thing pointing in the end ? Since some data are written to address in "null_dma_buf" ... isn't this just changing the corruption impact ?

        /* Allocate master with space for drv_data and null dma buffer */
        master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);

So there's 16 bytes at the end of driver_data.

However:
(u32)(drv_data + sizeof(struct driver_data))

is pointer arithmetic. drv_data points at an object of sizeof(struct driver_data). Adding one to this increments the pointer by sizeof(struct driver_data) bytes. So the above expression increments the pointer by sizeof(struct driver_data)*sizeof(struct driver_data) bytes, which is obviously complete rubbish.

Quoted 4 lines

((u32)drv_data + sizeof(struct driver_data))

casts drv_data to a u32 first, then adds the sizeof(struct driver_data) which moves us into the 16 bytes allocated off the end of the struct.

The ptr arritmetic is clear, it was the 16 bytes after the structure I was missing ... if it's allocated there, it's fine. Thanks for clearing this.

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure
and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..25358cd 100644

drivers/spi/pxa2xx_spi.c +2 -2

 	/* SSP register addresses */
 	void __iomem *ioaddr;

@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) return -ENODEV;
 	}
 
-	/* Allocate master with space for drv_data and null dma buffer */
-	master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+	/* Allocate master with space for drv_data */
+	master = spi_alloc_master(dev, sizeof(struct driver_data));
 	if (!master) {
 		dev_err(&pdev->dev, "cannot alloc spi_master\n");
 		pxa_ssp_free(ssp);

drivers/spi/pxa2xx_spi.c +0 -0

 	drv_data->ioaddr = ssp->mmio_base;
 	drv_data->ssdr_physical = ssp->phys_base + SSDR;

Quoted 21 lines · Vasily Khoruzhick

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure

and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..25358cd 100644
@@ -106,6 +106,7 @@ struct driver_data {
 

drivers/spi/pxa2xx_spi.c struct driver_data {

Quoted 5 lines

drivers/spi/pxa2xx_spi.c +1 -0

 	int rx_channel;
 	int tx_channel;
 	u32 *null_dma_buf;
 
+	u8 __null_dma_buf[16];

Ah, please don't name it starting with two underscores.

Quoted 21 lines

drivers/spi/pxa2xx_spi.c +0 -0

 	/* SSP register addresses */

drivers/spi/pxa2xx_spi.c +2 -2

 	void __iomem *ioaddr;

@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) return -ENODEV;
 
 	}
 
-	/* Allocate master with space for drv_data and null dma buffer */
-	master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+	/* Allocate master with space for drv_data */
+	master = spi_alloc_master(dev, sizeof(struct driver_data));
 
 	if (!master) {

drivers/spi/pxa2xx_spi.c +0 -0

 		dev_err(&pdev->dev, "cannot alloc spi_master\n");
 		pxa_ssp_free(ssp);

@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) master->transfer = transfer;
 

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) · line 1574

Quoted 6 lines

drivers/spi/pxa2xx_spi.c +2 -2

 	drv_data->ssp_type = ssp->type;
 
-	drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
-						sizeof(struct driver_data)), 8);
+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN((u8 *)drv_data->__null_dma_buf, 8);

Do you need that (u8 *) cast there ?

#define PTR_ALIGN(p, a) ((typeof(p))ALIGN((unsigned long)(p), (a)))

from linux/kernel.h line 42

Quoted 2 lines

drivers/spi/pxa2xx_spi.c +0 -0

 	drv_data->ioaddr = ssp->mmio_base;
 	drv_data->ssdr_physical = ssp->phys_base + SSDR;

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure
and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN
v3: - drop (u8 *) cast, use & operator instead, change array name

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +5 -4

index dc25bee..b25fe27 100644
@@ -106,6 +106,7 @@ struct driver_data {
 	int rx_channel;
 	int tx_channel;
 	u32 *null_dma_buf;
+	u8 null_dma_buf_unaligned[16];
 
 	/* SSP register addresses */
 	void __iomem *ioaddr;

@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) return -ENODEV;
 	}
 
-	/* Allocate master with space for drv_data and null dma buffer */
-	master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+	/* Allocate master with space for drv_data */
+	master = spi_alloc_master(dev, sizeof(struct driver_data));
 	if (!master) {
 		dev_err(&pdev->dev, "cannot alloc spi_master\n");
 		pxa_ssp_free(ssp);

@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) master->transfer = transfer;
 
 	drv_data->ssp_type = ssp->type;
-	drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
-						sizeof(struct driver_data)), 8);
+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);
 
 	drv_data->ioaddr = ssp->mmio_base;
 	drv_data->ssdr_physical = ssp->phys_base + SSDR;

Quoted 4 lines

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Ping?

Quoted 44 lines

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN
v3: - drop (u8 *) cast, use & operator instead, change array name drivers/spi/pxa2xx_spi.c |? ? ? 9 +++++----

1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +1 -0

index dc25bee..b25fe27 100644
@@ -106,6 +106,7 @@ struct driver_data {
 ??? int rx_channel;
 ??? int tx_channel;
 ??? u32 *null_dma_buf;
+??? u8 null_dma_buf_unaligned[16];
 

??? /* SSP register addresses */
??? void __iomem *ioaddr;

drivers/spi/pxa2xx_spi.c +4 -4

@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) return -ENODEV;
 ??? }
 
-??? /* Allocate master with space for drv_data and null dma buffer */
-??? master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+??? /* Allocate master with space for drv_data */
+??? master = spi_alloc_master(dev, sizeof(struct driver_data));
 ??? if (!master) {
 ??? ??? dev_err(&pdev->dev, "cannot alloc spi_master\n");
 ??? ??? pxa_ssp_free(ssp);

@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct
 platform_device *pdev) master->transfer = transfer;
 
 ??? drv_data->ssp_type = ssp->type;
-??? drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
-??? ??? ??? ??? ??? ??? sizeof(struct driver_data)), 8);
+??? drv_data->null_dma_buf =
+??? ??? (u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);
 
 ??? drv_data->ioaddr = ssp->mmio_base;
 ??? drv_data->ssdr_physical = ssp->phys_base + SSDR;

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure
and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN
v3: - drop (u8 *) cast, use & operator instead, change array name

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..b25fe27 100644

drivers/spi/pxa2xx_spi.c +4 -4

 	/* SSP register addresses */
 	void __iomem *ioaddr;

@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	/* Allocate master with space for drv_data and null dma buffer */
-	master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+	/* Allocate master with space for drv_data */
+	master = spi_alloc_master(dev, sizeof(struct driver_data));
 	if (!master) {
 		dev_err(&pdev->dev, "cannot alloc spi_master\n");
 		pxa_ssp_free(ssp);

@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct platform_device *pdev)
 	master->transfer = transfer;
 
 	drv_data->ssp_type = ssp->type;
-	drv_data->null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
-						sizeof(struct driver_data)), 8);
+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);
 
 	drv_data->ioaddr = ssp->mmio_base;
 	drv_data->ssdr_physical = ssp->phys_base + SSDR;

Signature · 1 line

1.7.5.rc3

drivers/spi/pxa2xx_spi.c struct driver_data { · line 109

Quoted 1 line

drivers/spi/pxa2xx_spi.c +1 -0

+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned? How large is the actual transfer? Using the __aligned() or __cacheline_aligned attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

Quoted 2 lines

drivers/spi/pxa2xx_spi.c +2 -0

+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);

Quoted 7 lines · Grant Likely

drivers/spi/pxa2xx_spi.c struct driver_data { · line 109

Quoted 1 line

drivers/spi/pxa2xx_spi.c +1 -0

+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned? How large is the actual transfer? Using the __aligned() or __cacheline_aligned attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

Err, did you not read the whole patch?

Quoted 2 lines

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) · line 1574

Quoted 2 lines

drivers/spi/pxa2xx_spi.c +2 -0

+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);

Quoted 11 lines · Russell King - ARM Linux

Quoted 7 lines · Grant Likely

drivers/spi/pxa2xx_spi.c struct driver_data { · line 109

Quoted 1 line

drivers/spi/pxa2xx_spi.c +1 -0

+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned? How large is the actual transfer? Using the __aligned() or __cacheline_aligned attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

Err, did you not read the whole patch?

Quoted 2 lines

Quoted 2 lines

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) · line 1574

Quoted 2 lines

drivers/spi/pxa2xx_spi.c +2 -0

+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);

I read a lot of patches yesterday. I may very well have missed something. I still don't see what you're referring to though. If the __aligned() was used inside the structure definition, then there would be no need to have both the null_dma_buf pointer and the null_dma_buf_unaligned buffer. It would just be a correctly aligned null_dma_buf.

Plus, I was asking about whether it was valid to use the structure as allocated in DMA operations since it may very well end up in the same cache line as the allocated structure. Firstly, that could mean DMA and the cache referencing the same memory which could cause corruption, and secondly on ARM isn't it a problem to have DMA buffers in memory that is also cache mapped?

Quoted 12 lines · Grant Likely

Quoted 11 lines · Russell King - ARM Linux

Quoted 7 lines · Grant Likely

drivers/spi/pxa2xx_spi.c struct driver_data { · line 109

Quoted 1 line

drivers/spi/pxa2xx_spi.c +1 -0

+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned? How large is the actual transfer? Using the __aligned() or __cacheline_aligned attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

Err, did you not read the whole patch?

Quoted 9 lines

Quoted 2 lines

Quoted 2 lines

drivers/spi/pxa2xx_spi.c static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) · line 1574

Quoted 2 lines

drivers/spi/pxa2xx_spi.c +2 -0

+	drv_data->null_dma_buf =
+		(u32 *)PTR_ALIGN(&drv_data->null_dma_buf_unaligned, 8);

I read a lot of patches yesterday. I may very well have missed something. I still don't see what you're referring to though. If the __aligned() was used inside the structure definition, then there would be no need to have both the null_dma_buf pointer and the null_dma_buf_unaligned buffer. It would just be a correctly aligned null_dma_buf.

That depends on the alignment guarantees from kmalloc, which may not be 8 bytes - we have this:

#if defined(CONFIG_AEABI) && (__LINUX_ARM_ARCH__ >= 5) #define ARCH_SLAB_MINALIGN 8
#endif

so presumably on !AEABI or arches < ARMv5, kmalloc _can_ return less than 8 byte alignments. Which makes using __aligned() in the definition useless.

Quoted 6 lines

Plus, I was asking about whether it was valid to use the structure as allocated in DMA operations since it may very well end up in the same cache line as the allocated structure. Firstly, that could mean DMA and the cache referencing the same memory which could cause corruption, and secondly on ARM isn't it a problem to have DMA buffers in memory that is also cache mapped?

For the second point, that depends on whether you're talking about the coherent stuff or the streaming stuff.

The coherent DMA API has entirely different semantics to streaming DMA API. The coherent DMA API allows for simultaneous access to the buffer by both the DMA device and the host CPU.

The streaming DMA API only allows exclusive access by either the DMA device or the host CPU.

Therefore, with the streaming DMA API, the only thing that's required is to ensure that data is visible in some manner to the DMA device. If the DMA device can read from the CPU cache, then probably nothing's required. If not, then the data must be evicted from as many levels of cache that are necessary to make it visible. Conversely, for DMA writes, what matters is the visibility of the data to the host CPU.

... plus care must be taken not to accidentally reload the line into cache after it has been pushed out for DMA, which is a risk on structures with embedded DMA buffers if other non-DMA elements end up in the same cache line. This is the situation I was wondering about.

Quoted 23 lines · Vasily Khoruzhick

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure

and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN

v3: - drop (u8 *) cast, use & operator instead, change array name

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..b25fe27 100644
@@ -106,6 +106,7 @@ struct driver_data {
 

drivers/spi/pxa2xx_spi.c struct driver_data {

Quoted 5 lines

drivers/spi/pxa2xx_spi.c +1 -0

 	int rx_channel;
 	int tx_channel;
 	u32 *null_dma_buf;
 
+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned?

How large is the actual transfer?

Using the __aligned() or __cacheline_aligned
attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

g.

Quoted 24 lines · On Friday 15 July 2011 05:53:31 Grant Likely wrote:

Quoted 23 lines · Vasily Khoruzhick

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure

and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN

v3: - drop (u8 *) cast, use & operator instead, change array name

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..b25fe27 100644
@@ -106,6 +106,7 @@ struct driver_data {
 

Quoted 7 lines

drivers/spi/pxa2xx_spi.c struct driver_data {

Quoted 5 lines

drivers/spi/pxa2xx_spi.c +1 -0

 	int rx_channel;
 	int tx_channel;
 	u32 *null_dma_buf;
 
+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned?

No, on PXA2xx they need to be 8-bytes aligned (according to PXA27x developer's manual)

Quoted 1 line

How large is the actual transfer?

Looks like 8 bytes, but I'm not sure, I'm not author of driver and did not dig deeply into its code. Just attempting to fix memory corruption.

Quoted 4 lines

Using the __aligned() or __cacheline_aligned
attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

Errr, it can't be applied to struct field, right? But driver needs per-device null_dma_buf (there's 3 SPI controllers on PXA2xx)

Quoted 1 line

g.

Regards
Vasily

Quoted 51 lines · On Friday 15 July 2011 05:53:31 Grant Likely wrote:

Quoted 24 lines · Vasily Khoruzhick

Quoted 23 lines

pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure

and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN

v3: - drop (u8 *) cast, use & operator instead, change array name

 drivers/spi/pxa2xx_spi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0

index dc25bee..b25fe27 100644
@@ -106,6 +106,7 @@ struct driver_data {
 

Quoted 7 lines

drivers/spi/pxa2xx_spi.c struct driver_data {

Quoted 5 lines

drivers/spi/pxa2xx_spi.c +1 -0

 	int rx_channel;
 	int tx_channel;
 	u32 *null_dma_buf;
 
+	u8 null_dma_buf_unaligned[16];

Don't dma buffers need to be cache-line aligned?

No, on PXA2xx they need to be 8-bytes aligned (according to PXA27x developer's manual)

Quoted 1 line

How large is the actual transfer?

Looks like 8 bytes, but I'm not sure, I'm not author of driver and did not dig deeply into its code. Just attempting to fix memory corruption.

Quoted 4 lines

Using the __aligned() or __cacheline_aligned
attribute is the correct way to make sure you've got a data buffer that can be used for DMA mixed with other stuff. Then you don't need to fool around with PTR_ALIGN or anything.

Errr, it can't be applied to struct field, right? But driver needs per-device null_dma_buf (there's 3 SPI controllers on PXA2xx)

Quoted 1 line

g.

Quoted 2 lines

Regards
Vasily

So, any chance to see this patch merged?

Regards
Vasily

So, any chance to see this patch merged?