RE: [PATCH V4 1/3] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled

["Sricharan" <sricharan@codeaurora.org>]

+Wolfram Sang

>-----Original Message-----
>From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org] On Behalf Of Sricharan R
>Sent: Friday, June 10, 2016 11:38 PM
>To: linux-arm-msm@vger.kernel.org; ntelkar@codeaurora.org; galak@codeaurora.org; linux-kernel@vger.kernel.org;
>andy.gross@linaro.org; linux-i2c@vger.kernel.org; agross@codeaurora.org; linux-arm-kernel@lists.infradead.org;
>nkaje@codeaurora.org; absahu@codeaurora.org
>Cc: sricharan@codeaurora.org
>Subject: [PATCH V4 1/3] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled
>
>With CONFIG_DEBUG_SG is enabled and when dma mode is used, below dump is seen,
>
>------------[ cut here ]------------
>kernel BUG at include/linux/scatterlist.h:140!
>Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>Modules linked in:
>CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-00459-g9f087b9-dirty #7
>Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>task: ffffffc036868000 ti: ffffffc036870000 task.ti: ffffffc036870000
>PC is at qup_sg_set_buf.isra.13+0x138/0x154
>LR is at qup_sg_set_buf.isra.13+0x50/0x154
>pc : [<ffffffc0005a0ed8>] lr : [<ffffffc0005a0df0>] pstate: 60000145
>sp : ffffffc0368735c0
>x29: ffffffc0368735c0 x28: ffffffc036873752
>x27: ffffffc035233018 x26: ffffffc000c4e000
>x25: 0000000000000000 x24: 0000000000000004
>x23: 0000000000000000 x22: ffffffc035233668
>x21: ffffff80004e3000 x20: ffffffc0352e0018
>x19: 0000004000000000 x18: 0000000000000028
>x17: 0000000000000004 x16: ffffffc0017a39c8
>x15: 0000000000001cdf x14: ffffffc0019929d8
>x13: ffffffc0352e0018 x12: 0000000000000000
>x11: 0000000000000001 x10: 0000000000000001
>x9 : ffffffc0012b2d70 x8 : ffffff80004e3000
>x7 : 0000000000000018 x6 : 0000000030000000
>x5 : ffffffc00199f018 x4 : ffffffc035233018
>x3 : 0000000000000004 x2 : 00000000c0000000
>x1 : 0000000000000003 x0 : 0000000000000000
>
>Process swapper/0 (pid: 1, stack limit = 0xffffffc036870020)
>Stack: (0xffffffc0368735c0 to 0xffffffc036874000)
>
>sg_set_bug expects that the buf parameter passed in should be from
>lowmem and a valid pageframe. This is not true for pages from
>dma_alloc_coherent which can be carveouts, hence the check fails.
>Change allocation of sg buffers from dma_coherent memory to kzalloc
>to fix the issue. Note that now dma_map/unmap is used to make the
>kzalloc'ed buffers coherent before passing it to the dmaengine.
>
>Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>Reviewed-by: Andy Gross <andy.gross@linaro.org>
>---
> drivers/i2c/busses/i2c-qup.c | 51 +++++++++++++-------------------------------
> 1 file changed, 15 insertions(+), 36 deletions(-)
>
>diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>index cc6439ab..43adc2d 100644
>--- a/drivers/i2c/busses/i2c-qup.c
>+++ b/drivers/i2c/busses/i2c-qup.c
>@@ -585,8 +585,8 @@ static void qup_i2c_bam_cb(void *data)
> }
>
> static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
>-			  struct qup_i2c_tag *tg, unsigned int buflen,
>-			  struct qup_i2c_dev *qup, int map, int dir)
>+			  unsigned int buflen, struct qup_i2c_dev *qup,
>+			  int dir)
> {
> 	int ret;
>
>@@ -595,9 +595,6 @@ static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
> 	if (!ret)
> 		return -EINVAL;
>
>-	if (!map)
>-		sg_dma_address(sg) = tg->addr + ((u8 *)buf - tg->start);
>-
> 	return 0;
> }
>
>@@ -670,16 +667,15 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
> 				/* scratch buf to read the start and len tags */
> 				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
> 						     &qup->brx.tag.start[0],
>-						     &qup->brx.tag,
>-						     2, qup, 0, 0);
>+						     2, qup, DMA_FROM_DEVICE);
>
> 				if (ret)
> 					return ret;
>
> 				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
> 						     &msg->buf[limit * i],
>-						     NULL, tlen, qup,
>-						     1, DMA_FROM_DEVICE);
>+						     tlen, qup,
>+						     DMA_FROM_DEVICE);
> 				if (ret)
> 					return ret;
>
>@@ -688,7 +684,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
> 			}
> 			ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
> 					     &qup->start_tag.start[off],
>-					     &qup->start_tag, len, qup, 0, 0);
>+					     len, qup, DMA_TO_DEVICE);
> 			if (ret)
> 				return ret;
>
>@@ -696,8 +692,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
> 			/* scratch buf to read the BAM EOT and FLUSH tags */
> 			ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
> 					     &qup->brx.tag.start[0],
>-					     &qup->brx.tag, 2,
>-					     qup, 0, 0);
>+					     2, qup, DMA_FROM_DEVICE);
> 			if (ret)
> 				return ret;
> 		} else {
>@@ -709,17 +704,15 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
> 				len = qup_i2c_set_tags(tags, qup, msg, 1);
>
> 				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
>-						     tags,
>-						     &qup->start_tag, len,
>-						     qup, 0, 0);
>+						     tags, len,
>+						     qup, DMA_TO_DEVICE);
> 				if (ret)
> 					return ret;
>
> 				tx_len += len;
> 				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
> 						     &msg->buf[limit * i],
>-						     NULL, tlen, qup, 1,
>-						     DMA_TO_DEVICE);
>+						     tlen, qup, DMA_TO_DEVICE);
> 				if (ret)
> 					return ret;
> 				i++;
>@@ -738,8 +731,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
> 							QUP_BAM_FLUSH_STOP;
> 				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
> 						     &qup->btx.tag.start[0],
>-						     &qup->btx.tag, len,
>-						     qup, 0, 0);
>+						     len, qup, DMA_TO_DEVICE);
> 				if (ret)
> 					return ret;
> 				tx_nents += 1;
>@@ -1407,27 +1399,21 @@ static int qup_i2c_probe(struct platform_device *pdev)
>
> 		/* 2 tag bytes for each block + 5 for start, stop tags */
> 		size = blocks * 2 + 5;
>-		qup->dpool = dma_pool_create("qup_i2c-dma-pool", &pdev->dev,
>-					     size, 4, 0);
>
>-		qup->start_tag.start = dma_pool_alloc(qup->dpool, GFP_KERNEL,
>-						      &qup->start_tag.addr);
>+		qup->start_tag.start = devm_kzalloc(&pdev->dev,
>+						    size, GFP_KERNEL);
> 		if (!qup->start_tag.start) {
> 			ret = -ENOMEM;
> 			goto fail_dma;
> 		}
>
>-		qup->brx.tag.start = dma_pool_alloc(qup->dpool,
>-						    GFP_KERNEL,
>-						    &qup->brx.tag.addr);
>+		qup->brx.tag.start = devm_kzalloc(&pdev->dev, 2, GFP_KERNEL);
> 		if (!qup->brx.tag.start) {
> 			ret = -ENOMEM;
> 			goto fail_dma;
> 		}
>
>-		qup->btx.tag.start = dma_pool_alloc(qup->dpool,
>-						    GFP_KERNEL,
>-						    &qup->btx.tag.addr);
>+		qup->btx.tag.start = devm_kzalloc(&pdev->dev, 2, GFP_KERNEL);
> 		if (!qup->btx.tag.start) {
> 			ret = -ENOMEM;
> 			goto fail_dma;
>@@ -1566,13 +1552,6 @@ static int qup_i2c_remove(struct platform_device *pdev)
> 	struct qup_i2c_dev *qup = platform_get_drvdata(pdev);
>
> 	if (qup->is_dma) {
>-		dma_pool_free(qup->dpool, qup->start_tag.start,
>-			      qup->start_tag.addr);
>-		dma_pool_free(qup->dpool, qup->brx.tag.start,
>-			      qup->brx.tag.addr);
>-		dma_pool_free(qup->dpool, qup->btx.tag.start,
>-			      qup->btx.tag.addr);
>-		dma_pool_destroy(qup->dpool);
> 		dma_release_channel(qup->btx.dma);
> 		dma_release_channel(qup->brx.dma);
> 	}
>--
>QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux
>Foundation
>
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel