| Message ID | 1260797602-7476-1-git-send-email-Vishnu@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
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
> 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
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
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 */
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(-)