From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756610Ab0EKGMI (ORCPT ); Tue, 11 May 2010 02:12:08 -0400 Received: from mail-gy0-f174.google.com ([209.85.160.174]:52468 "EHLO mail-gy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751467Ab0EKGMC convert rfc822-to-8bit (ORCPT ); Tue, 11 May 2010 02:12:02 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=k+g1XKd7BEbR0ofCvQCKtlOASdiCXy3O+kjtv2HWJdTniXuerAmzCOGo1Lf6rV0RVD tRw6c42TDhDj46SqDY734uYImeRLXkiJD8sUDF8ExFeAGIKdEeohDV4Xg2rkhqbrTqyA GhSa8KyVwx49tlJIeD5fX5qw8oJTS5UbeA330= MIME-Version: 1.0 In-Reply-To: <20100511060547.GA9644@core.coreip.homeip.net> References: <1273487642-2169-1-git-send-email-os@emlix.com> <1273488154-2993-1-git-send-email-os@emlix.com> <20100510142225.4bf215ef.akpm@linux-foundation.org> <20100511060547.GA9644@core.coreip.homeip.net> From: Mike Frysinger Date: Tue, 11 May 2010 02:11:41 -0400 Message-ID: Subject: Re: [PATCH v3] ad7877: keep dma rx buffers in seperate cache lines To: Dmitry Torokhov Cc: Andrew Morton , Oskar Schirmer , Michael Hennerich , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, =?UTF-8?Q?Daniel_Gl=C3=B6ckner?= , Oliver Schneidewind , Johannes Weiner Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > 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 > Signed-off-by: Daniel Glöckner > Signed-off-by: Oliver Schneidewind > Signed-off-by: Johannes Weiner > Acked-by: Mike Frysinger > [dtor@mail.ru - changed to use ___cacheline_aligned at suggestion >  of akpm] > Signed-off-by: Dmitry Torokhov > --- > >  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