[PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors

[PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors

[Vishnu Suresh]

The async_tx descriptors contains dangling pointers.
Hence, re-initialize them to NULL before use.

Signed-off-by: Vishnu Suresh <Vishnu@freescale.com>
---
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 */
-- 
1.6.4.2

Re: [PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors

[Dan Williams]

On Mon, Dec 14, 2009 at 6:33 AM, Vishnu Suresh <Vishnu@freescale.com> wrote:
> The async_tx descriptors contains dangling pointers.
> Hence, re-initialize them to NULL before use.
>
> Signed-off-by: Vishnu Suresh <Vishnu@freescale.com>
> ---
> 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.  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.
Also that dma_async_tx_descriptor_init() is unexpected in the hot
path, it's only needed at initial descriptor allocation.  End result I
think this driver needs some more time to brew.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors

[Suresh Vishnu-B05022]

[-- Attachment #1.1: Type: text/plain, Size: 2185 bytes --]

> On Mon, Dec 14, 2009 at 6:33 AM, Vishnu Suresh <Vishnu@freescale.com> wrote:
> > The async_tx descriptors contains dangling pointers.
> > Hence, re-initialize them to NULL before use.
> >
> > Signed-off-by: Vishnu Suresh <Vishnu@freescale.com>
> > ---
> > 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?

> > Also that dma_async_tx_descriptor_init() is unexpected in the hot
> > path, it's only needed at initial descriptor allocation.  End result I
> > think this driver needs some more time to brew.



> --
> Dan




[-- Attachment #1.2: Type: text/html, Size: 3265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v0] Crypto: Talitos: re-initialize async_tx descriptors

[Dan Williams]

Suresh Vishnu-B05022 wrote:
>  > On Mon, Dec 14, 2009 at 6:33 AM, Vishnu Suresh <Vishnu@freescale.com> 
> wrote:
>  > > The async_tx descriptors contains dangling pointers.
>  > > Hence, re-initialize them to NULL before use.
>  > >
>  > > Signed-off-by: Vishnu Suresh <Vishnu@freescale.com>
>  > > ---
>  > > 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