[03/10] spi: Guarantee cacheline alignment of driver-private data

Commit Message

Lukas Wunner Aug. 3, 2019, 10:10 a.m. UTC

__spi_alloc_controller() uses a single allocation to accommodate struct
spi_controller and the driver-private data, but places the latter behind
the former.  This order does not guarantee cacheline alignment of the
driver-private data.  It does guarantee cacheline alignment of struct
spi_controller but the structure doesn't make any use of that property.

Reverse the order.  A forthcoming commit leverages this to grant DMA
access to driver-private data of the BCM2835 SPI master.

An alternative approach would be to round up struct spi_controller to
cacheline size, at the expense of some wasted memory.

Tested-by: Nuno Sá <[e-mail]>
Signed-off-by: Lukas Wunner <[e-mail]>
---
 drivers/spi/spi.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Mark Brown Sept. 10, 2019, 11:29 a.m. UTC | #1

On Sat, Aug 03, 2019 at 12:10:00PM +0200, Lukas Wunner wrote:
> __spi_alloc_controller() uses a single allocation to accommodate struct
> 
> Reverse the order.  A forthcoming commit leverages this to grant DMA
> access to driver-private data of the BCM2835 SPI master.

That's just shuffling the problem around, the same issues will
then apply to the controller struct and you'll take a performance
penalty on architectures that don't like unaligned accesses.

> An alternative approach would be to round up struct spi_controller to
> cacheline size, at the expense of some wasted memory.

That would seem more logical, or just do two alloacations.  It's
not like we allocate huge numbers of SPI controllers.

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8e83c9567353..8f692f657690 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2071,9 +2071,11 @@  static inline void acpi_register_spi_devices(struct spi_controller *ctlr) {}
 static void spi_controller_release(struct device *dev)
 {
 	struct spi_controller *ctlr;
+	void *devdata;
 
 	ctlr = container_of(dev, struct spi_controller, dev);
-	kfree(ctlr);
+	devdata = spi_controller_get_devdata(ctlr);
+	kfree(devdata);
 }
 
 static struct class spi_master_class = {
@@ -2187,8 +2189,10 @@  extern struct class spi_slave_class;	/* dummy */
  * __spi_alloc_controller - allocate an SPI master or slave controller
  * @dev: the controller, possibly using the platform_bus
  * @size: how much zeroed driver-private data to allocate; the pointer to this
- *	memory is in the driver_data field of the returned device,
- *	accessible with spi_controller_get_devdata().
+ *	memory is in the driver_data field of the returned device, accessible
+ *	with spi_controller_get_devdata(); the memory is cacheline aligned;
+ *	drivers granting DMA access to portions of their private data need to
+ *	round up @size using ALIGN(size, dma_get_cache_alignment()).
  * @slave: flag indicating whether to allocate an SPI master (false) or SPI
  *	slave (true) controller
  * Context: can sleep
@@ -2210,14 +2214,16 @@  struct spi_controller *__spi_alloc_controller(struct device *dev,
 					      unsigned int size, bool slave)
 {
 	struct spi_controller	*ctlr;
+	void *devdata;
 
 	if (!dev)
 		return NULL;
 
-	ctlr = kzalloc(size + sizeof(*ctlr), GFP_KERNEL);
-	if (!ctlr)
+	devdata = kzalloc(size + sizeof(*ctlr), GFP_KERNEL);
+	if (!devdata)
 		return NULL;
 
+	ctlr = devdata + size;
 	device_initialize(&ctlr->dev);
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = 1;
@@ -2228,7 +2234,7 @@  struct spi_controller *__spi_alloc_controller(struct device *dev,
 		ctlr->dev.class = &spi_master_class;
 	ctlr->dev.parent = dev;
 	pm_suspend_ignore_children(&ctlr->dev, true);
-	spi_controller_set_devdata(ctlr, &ctlr[1]);
+	spi_controller_set_devdata(ctlr, devdata);
 
 	return ctlr;
 }