Re: [PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Jarkko Sakkinen]

On Fri, Jul 29, 2016 at 10:30:22AM -0700, Dmitry Torokhov wrote:
>    On Fri, Jul 29, 2016 at 10:27 AM, Jason Gunthorpe
>    <jguntho...@obsidianresearch.com> wrote:
> 
>      On Thu, Jul 28, 2016 at 07:59:13PM -0700, Andrey Pronin wrote:
>      > Annotate buffers used in spi transactions as ____cacheline_aligned
>      > to use in DMA transfers.
>      >
>      > Signed-off-by: Andrey Pronin <apro...@chromium.org>
>      >  drivers/char/tpm/st33zp24/spi.c | 4 ++--
>      >  drivers/char/tpm/tpm_tis_spi.c  | 4 ++--
>      >  2 files changed, 4 insertions(+), 4 deletions(-)
>      >
>      > diff --git a/drivers/char/tpm/st33zp24/spi.c
>      b/drivers/char/tpm/st33zp24/spi.c
>      > index 9f5a011..0e9aad9 100644
>      > +++ b/drivers/char/tpm/st33zp24/spi.c
>      > @@ -70,8 +70,8 @@
>      >  struct st33zp24_spi_phy {
>      >       struct spi_device *spi_device;
>      >
>      > -     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
>      > -     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
>      > +     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
>      > +     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
>      >
>      >       int io_lpcpd;
>      >       int latency;
> 
>      Hurm, this still looks wrong to me. Aligning the start of buffers is
>      not enough, the DMA'able space must also end on a cache line as well.
> 
>      So, the buffers must also always be placed at the end of the struct.
> 
>      IMHO It would be cleaner and safer to always kmalloc the DMA buffer
>      alone than to try and optimize like this.
> 
>    In this case moving them to the end of the structure and commenting why
>    they have to be at the end might be less invasive change. More
>    performance-efficient and resilient in low memory situations too.

kmallocs would be done in the driver initialization:

* you rarely are in low memory situation
* performance gain/loss is insignificant

I really don't see your point.

>    Thanks,
>    Dmitry

/Jarkko