From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Williams Subject: Re: [PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors Date: Tue, 15 Dec 2009 11:23:40 -0700 Message-ID: <4B27D42C.8070309@intel.com> References: <1260797602-7476-1-git-send-email-Vishnu@freescale.com> <76952DF81420A349876E087D089DF034562C75@zin33exm21.fsl.freescale.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "linuxppc-dev@ozlabs.org" , "linux-kernel@vger.kernel.org" , "linux-raid@vger.kernel.org" , "linux-crypto@vger.kernel.org" , "herbert@gondor.apana.org.au" , Tabi Timur-B04825 , Li Yang-R58472 , Ira Snyder To: Suresh Vishnu-B05022 Return-path: Received: from mga09.intel.com ([134.134.136.24]:25022 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760781AbZLOSXn (ORCPT ); Tue, 15 Dec 2009 13:23:43 -0500 In-Reply-To: <76952DF81420A349876E087D089DF034562C75@zin33exm21.fsl.freescale.net> Sender: linux-crypto-owner@vger.kernel.org List-ID: Suresh Vishnu-B05022 wrote: > > On Mon, Dec 14, 2009 at 6:33 AM, Vishnu Suresh > wrote: > > > The async_tx descriptors contains dangling pointers. > > > Hence, re-initialize them to NULL before use. > > > > > > Signed-off-by: Vishnu Suresh > > > --- > > > o. Rebased to linux-next as of 20091214 > > > > > > drivers/crypto/talitos.c | 3 +++ > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c > > > index 87f06be..9e261c6 100644 > > > --- a/drivers/crypto/talitos.c > > > +++ b/drivers/crypto/talitos.c > > > @@ -952,6 +952,9 @@ static struct dma_async_tx_descriptor * > talitos_prep_dma_xor( > > > return NULL; > > > } > > > dma_async_tx_descriptor_init(&new->async_tx, &xor_chan->common); > > > + new->async_tx.parent = NULL; > > > + new->async_tx.next = NULL; > > > + > > > > > > desc = &new->hwdesc; > > > /* Set destination: Last pointer pair */ > > > These two values are owned by the async_tx api, drivers are not > > supposed to touch them. > I have sent this patch and the similar one for fsldma seperately, > so that if the changes are needed and can be done in > dma_async_tx_descriptor_init(), > these patches can be ignored. > > Both iop_adma and the new ppx4xx driver > > (which use the async_tx channel switching capability) get away without > > touching these fields which makes me suspect there is a > > misunderstanding/bug somewhere else in the talitos implementation. > > This bug does not occur on all the platforms. The occurrance is random. > This occurs when a channel switch between two different devices are > present. > This same initialization is required in case of fsldma as well. > In case of fsldma/talitosXOR, there are two DMA channels (on same > device) and single XOR channel (on another device). > > When used without fsldma, this driver works fine. > Does iop_adma and ppx4xx work in similar enviroment? Yes, iop_adma when running on iop3xx hardware has one xor channel and two memcpy channels. The bug may very well be in the fsldma driver, but the workarounds are just band aids. I would be more comfortable with dropping the three workaround patches and simply adding "depends on !FSL_DMA" to the CRYPTO_DEV_TALITOS option until the true fix can be developed. -- Dan From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by ozlabs.org (Postfix) with ESMTP id 456CCB6F19 for ; Wed, 16 Dec 2009 05:23:42 +1100 (EST) Message-ID: <4B27D42C.8070309@intel.com> Date: Tue, 15 Dec 2009 11:23:40 -0700 From: Dan Williams MIME-Version: 1.0 To: Suresh Vishnu-B05022 Subject: Re: [PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors References: <1260797602-7476-1-git-send-email-Vishnu@freescale.com> <76952DF81420A349876E087D089DF034562C75@zin33exm21.fsl.freescale.net> In-Reply-To: <76952DF81420A349876E087D089DF034562C75@zin33exm21.fsl.freescale.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Cc: "herbert@gondor.apana.org.au" , Ira Snyder , Tabi Timur-B04825 , "linux-kernel@vger.kernel.org" , "linux-raid@vger.kernel.org" , "linuxppc-dev@ozlabs.org" , "linux-crypto@vger.kernel.org" , Li Yang-R58472 List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Suresh Vishnu-B05022 wrote: > > On Mon, Dec 14, 2009 at 6:33 AM, Vishnu Suresh > wrote: > > > The async_tx descriptors contains dangling pointers. > > > Hence, re-initialize them to NULL before use. > > > > > > Signed-off-by: Vishnu Suresh > > > --- > > > o. Rebased to linux-next as of 20091214 > > > > > > drivers/crypto/talitos.c | 3 +++ > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c > > > index 87f06be..9e261c6 100644 > > > --- a/drivers/crypto/talitos.c > > > +++ b/drivers/crypto/talitos.c > > > @@ -952,6 +952,9 @@ static struct dma_async_tx_descriptor * > talitos_prep_dma_xor( > > > return NULL; > > > } > > > dma_async_tx_descriptor_init(&new->async_tx, &xor_chan->common); > > > + new->async_tx.parent = NULL; > > > + new->async_tx.next = NULL; > > > + > > > > > > desc = &new->hwdesc; > > > /* Set destination: Last pointer pair */ > > > These two values are owned by the async_tx api, drivers are not > > supposed to touch them. > I have sent this patch and the similar one for fsldma seperately, > so that if the changes are needed and can be done in > dma_async_tx_descriptor_init(), > these patches can be ignored. > > Both iop_adma and the new ppx4xx driver > > (which use the async_tx channel switching capability) get away without > > touching these fields which makes me suspect there is a > > misunderstanding/bug somewhere else in the talitos implementation. > > This bug does not occur on all the platforms. The occurrance is random. > This occurs when a channel switch between two different devices are > present. > This same initialization is required in case of fsldma as well. > In case of fsldma/talitosXOR, there are two DMA channels (on same > device) and single XOR channel (on another device). > > When used without fsldma, this driver works fine. > Does iop_adma and ppx4xx work in similar enviroment? Yes, iop_adma when running on iop3xx hardware has one xor channel and two memcpy channels. The bug may very well be in the fsldma driver, but the workarounds are just band aids. I would be more comfortable with dropping the three workaround patches and simply adding "depends on !FSL_DMA" to the CRYPTO_DEV_TALITOS option until the true fix can be developed. -- Dan