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

[Dmitry Torokhov]

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.

-- 
Dmitry
--
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/