lkml.org 
[lkml]   [2010]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3] ad7877: keep dma rx buffers in seperate cache lines
    On Tue, May 11, 2010 at 02:23:34AM -0400, Mike Frysinger wrote:
    > 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@emlix.com>
    > >> >
    > >> > 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@emlix.com>
    > >> > Signed-off-by: Daniel Glöckner <dg@emlix.com>
    > >> > Signed-off-by: Oliver Schneidewind <osw@emlix.com>
    > >> > Signed-off-by: Johannes Weiner <jw@emlix.com>
    > >> > Acked-by: Mike Frysinger <vapier@gentoo.org>
    > >> > [dtor@mail.ru - changed to use ___cacheline_aligned at suggestion
    > >> >  of akpm]
    > >> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
    > >> > ---
    > >> >
    > >> >  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 ?

    The data is kmalloced, kmalloc aligns on cacheline boundary AFAIK which
    means that next kmalloc data chunk will not share "our" cacheline.

    --
    Dmitry
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-05-12 11:27    [W:0.039 / U:120.976 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site