On Tue, May 11, 2010 at 02:21, Dmitry Torokhov wrote:
> On Tue, May 11, 2010 at 02:11:41AM -0400, Mike Frysinger wrote:
>> 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.
>
> The buffers are moved to the end of the structure - there is nothing
> else there.
what guarantee exactly do you have for that statement ?
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
