RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload

On Fri, 31 Aug 2012 06:08:05 +0300, Liu Qiang-B32616 [off-list ref]
wrote:

quoted

-----Original Message-----
From: Geanta Neag Horia Ioan-B05471
Sent: Thursday, August 30, 2012 10:23 PM
To: Liu Qiang-B32616; linux-crypto@vger.kernel.org;
dan.j.williams@gmail.com; herbert@gondor.hengli.com.au;
davem@davemloft.net; linux-kernel@vger.kernel.org; linuxppc-
dev@lists.ozlabs.org
Cc: Li Yang-R58472; Phillips Kim-R1AAHA; vinod.koul@intel.com;
dan.j.williams@intel.com; arnd@arndb.de; gregkh@linuxfoundation.org; Liu
Qiang-B32616
Subject: RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload

On Thu, 9 Aug 2012 11:20:48 +0300, qiang.liu@freescale.com wrote:

quoted

From: Qiang Liu <redacted>

Expose Talitos's XOR functionality to be used for RAID parity
calculation via the Async_tx layer.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Dipen Dudhat <redacted>
Signed-off-by: Maneesh Gupta <redacted>
Signed-off-by: Kim Phillips <redacted>
Signed-off-by: Vishnu Suresh <redacted>
Signed-off-by: Qiang Liu <redacted>
---
 drivers/crypto/Kconfig   |    9 +
 drivers/crypto/talitos.c |  413

++++++++++++++++++++++++++++++++++++++++++++++

quoted

 drivers/crypto/talitos.h |   53 ++++++
 3 files changed, 475 insertions(+), 0 deletions(-)

quoted

quoted

+static int talitos_alloc_chan_resources(struct dma_chan *chan)
+{
+	struct talitos_xor_chan *xor_chan;
+	struct talitos_xor_desc *desc;
+	LIST_HEAD(tmp_list);
+	int i;
+
+	xor_chan = container_of(chan, struct talitos_xor_chan, common);
+
+	if (!list_empty(&xor_chan->free_desc))
+		return xor_chan->total_desc;
+
+	for (i = 0; i < TALITOS_MAX_DESCRIPTOR_NR; i++) {
+		desc = talitos_xor_alloc_descriptor(xor_chan,
+				GFP_KERNEL | GFP_DMA);

talitos_xor_alloc_descriptor() is called here without holding
the xor_chan->desc_lock and it increments xor_chan->total_desc.
Isn't this an issue ?

No, please refer to the code as below,
+	list_add_tail(&desc->node, &tmp_list);

The list is temporary list, it will be merged to xor_chan->free_desc in next step, here is protected
by lock,
+	spin_lock_bh(&xor_chan->desc_lock);
+	list_splice_init(&tmp_list, &xor_chan->free_desc);
+	spin_unlock_bh(&xor_chan->desc_lock);

I was not referring to the list, but to xor_chan->total_desc variable.
The following access:
talitos_alloc_chan_resources()->talitos_xor_alloc_descriptor()->total_desc++
is not protected by the xor_chan->desc_lock.