Re: [PATCH v3] ad7877: keep dma rx buffers in seperate cache lines

[Mike Frysinger]

On Tue, May 11, 2010 at 02:05, Dmitry Torokhov wrote:
> On Mon, May 10, 2010 at 02:22:25PM -0700, Andrew Morton wrote:
>> On Mon, 10 May 2010 12:42:34 +0200 Oskar Schirmer wrote:
>> > With dma based spi transmission, data corruption
>> > is observed occasionally. With dma buffers located
>> > right next to msg and xfer fields, cache lines
>> > correctly flushed in preparation for dma usage
>> > may be polluted again when writing to fields
>> > in the same cache line.
>> >
>> > Make sure cache fields used with dma do not
>> > share cache lines with fields changed during
>> > dma handling. As both fields are part of a
>> > struct that is allocated via kzalloc, thus
>> > cache aligned, moving the fields to the 1st
>> > position and insert padding for alignment
>> > does the job.
>>
>> This sounds odd. ÂDoesn't it imply that some code somewhere is missing
>> some DMA synchronisation actions?
>>
>> >
>> > v2: add a comment to explain why alignment is needed
>> >
>> > v3: fix the typo in comment and layout (- to end of line)
>> >
>> > diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
>> > index 885354c..9ebb1b4 100644
>> > --- a/drivers/input/touchscreen/ad7877.c
>> > +++ b/drivers/input/touchscreen/ad7877.c
>> > @@ -153,15 +153,29 @@ enum {
>> > Â */
>> >
>> > Âstruct ser_req {
>> > + Â u16 Â Â Â Â Â Â Â Â Â Â sample;
>> > + Â /*
>> > + Â Â* DMA (thus cache coherency maintenance) requires the
>> > + Â Â* transfer buffers to live in their own cache lines.
>> > + Â Â*/
>> > +  char          Â__padalign[L1_CACHE_BYTES - sizeof(u16)];
>>
>> It would be better to use __cacheline_aligned, rather than open-coding
>> things in this manner.
>>
>
> OK, then I have the following which I will apply unless someone shouts.
>
> --
> Dmitry
>
> Input: ad7877 - keep dma rx buffers in seperate cache lines
>
> From: Oskar Schirmer <os@xxxxxxxxx>
>
> With dma based spi transmission, data corruption is observed
> occasionally. With dma buffers located right next to msg and
> xfer fields, cache lines correctly flushed in preparation for
> dma usage may be polluted again when writing to fields in the
> same cache line.
>
> Make sure cache fields used with dma do not share cache lines
> with fields changed during dma handling. As both fields are part
> of a struct that is allocated via kzalloc, thus cache aligned,
> moving the fields to the 1st position and insert padding for
> alignment does the job.
>
> Signed-off-by: Oskar Schirmer <os@xxxxxxxxx>
> Signed-off-by: Daniel GlÃckner <dg@xxxxxxxxx>
> Signed-off-by: Oliver Schneidewind <osw@xxxxxxxxx>
> Signed-off-by: Johannes Weiner <jw@xxxxxxxxx>
> Acked-by: Mike Frysinger <vapier@xxxxxxxxxx>
> [dtor@xxxxxxx - changed to use ___cacheline_aligned at suggestion
> Âof akpm]
> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
> ---
>
> Âdrivers/input/touchscreen/ad7877.c | Â 15 ++++++++++++---
> Â1 files changed, 12 insertions(+), 3 deletions(-)
>
>
> diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
> index e019d53..0d2d7e5 100644
> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -156,9 +156,14 @@ struct ser_req {
> Â Â Â Âu16 Â Â Â Â Â Â Â Â Â Â reset;
> Â Â Â Âu16 Â Â Â Â Â Â Â Â Â Â ref_on;
> Â Â Â Âu16 Â Â Â Â Â Â Â Â Â Â command;
> - Â Â Â u16 Â Â Â Â Â Â Â Â Â Â sample;
>    Âstruct spi_message   Âmsg;
>    Âstruct spi_transfer   xfer[6];
> +
> + Â Â Â /*
> + Â Â Â Â* DMA (thus cache coherency maintenance) requires the
> + Â Â Â Â* transfer buffers to live in their own cache lines.
> + Â Â Â Â*/
> + Â Â Â u16 sample ____cacheline_aligned;
> Â};
>
> Âstruct ad7877 {
> @@ -182,8 +187,6 @@ struct ad7877 {
> Â Â Â Âu8 Â Â Â Â Â Â Â Â Â Â Âaveraging;
> Â Â Â Âu8 Â Â Â Â Â Â Â Â Â Â Âpen_down_acc_interval;
>
> - Â Â Â u16 Â Â Â Â Â Â Â Â Â Â conversion_data[AD7877_NR_SENSE];
> -
>    Âstruct spi_transfer   xfer[AD7877_NR_SENSE + 2];
>    Âstruct spi_message   Âmsg;
>
> @@ -195,6 +198,12 @@ struct ad7877 {
>    Âspinlock_t       Âlock;
>    Âstruct timer_list    timer;     Â/* P: lock */
>    Âunsigned        Âpending:1;   Â/* P: lock */
> +
> + Â Â Â /*
> + Â Â Â Â* DMA (thus cache coherency maintenance) requires the
> + Â Â Â Â* transfer buffers to live in their own cache lines.
> + Â Â Â Â*/
> + Â Â Â u16 conversion_data[AD7877_NR_SENSE] ____cacheline_aligned;
> Â};

i'm not sure this is correct.  the cached_aligned attribute makes sure
it starts on a cache boundary, but it doesnt make sure it pads out to
one.  so it might work more of the time, but i dont think it's
guaranteed.
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/