From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752036AbbCaFUb (ORCPT ); Tue, 31 Mar 2015 01:20:31 -0400 Received: from mail-ie0-f171.google.com ([209.85.223.171]:35892 "EHLO mail-ie0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750706AbbCaFU2 (ORCPT ); Tue, 31 Mar 2015 01:20:28 -0400 MIME-Version: 1.0 In-Reply-To: <551A173B.4010704@broadcom.com> References: <1427414105-3480-1-git-send-email-sbranden@broadcom.com> <20150330172546.GI7192@intel.com> <551A173B.4010704@broadcom.com> Date: Tue, 31 Mar 2015 10:50:27 +0530 Message-ID: Subject: Re: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. From: Jassi Brar To: Scott Branden Cc: Vinod Koul , Dan Williams , Linux Kernel Mailing List , ismail , Anatol Pomazao , Dmitry Torokhov , bcm-kernel-feedback-list@broadcom.com, "linux-arm-kernel@lists.infradead.org" Content-Type: multipart/mixed; boundary=20cf304271b0ab0f4205128ec183 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --20cf304271b0ab0f4205128ec183 Content-Type: text/plain; charset=UTF-8 On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden wrote: > Hi Vinod, Jassi, > > Some details on the problem encountered. > > > On 15-03-30 10:25 AM, Vinod Koul wrote: >> >> On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >>> >>> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden >>> wrote: >>>> >>>> From: ismail >>>> >>>> Update the thread running index before issuing the >>>> GO command to the DMAC. >>>> >>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed >>>> Reviewed-by: Ray Jui >>>> Reviewed-by: Arun Parameswaran >>>> Reviewed-by: Scott Branden >>>> Signed-off-by: Scott Branden >>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed >>>> --- >>>> drivers/dma/pl330.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>> index 0e1f567..631642d 100644 >>>> --- a/drivers/dma/pl330.c >>>> +++ b/drivers/dma/pl330.c >>>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>>> /* Set to generate interrupts for SEV */ >>>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>>> >>>> + thrd->req_running = idx; >>>> + >>>> /* Only manager can execute GO */ >>>> _execute_DBGINSN(thrd, insn, true); >>>> >>>> - thrd->req_running = idx; >>>> - >>> >>> It would help to know what the behavior looks like before and after >>> the patch. If anything we should look at locking rather the >>> reordering. >> >> Yes that ia fair request, looking at changelog it is hard to understand >> the >> issue seen? >> > We encountered this problem as we modified the driver to make SMC calls to a > TZ handler. This slowed down the driver to the point where DMA transactions > easily failed. I believe the same could be accomplished by adding a delay > between the GOCMD and update of the req_running and running the built in > dmatest. > > The DMA transaction is broken if the interrupt occurs before the > thrd->req_running is updated. > > The pl330 issues a GOCMD (in _trigger function) to start a new transfer. > > The issue of GOCMD generates an interrupt and the IRQ handler will call the > pl330_update function to process the interrupt. > > The pl330_update function will verify the thread running index and break the > transaction, if the thread running index is not set. > As I suspected the locking seems screwed up. The following patch should fix the race properly. Can you please test the attached patches instead? Thanks. --20cf304271b0ab0f4205128ec183 Content-Type: text/x-patch; charset=US-ASCII; name="0001-dma-pl330-change-busy-marker-for-threads.patch" Content-Disposition: attachment; filename="0001-dma-pl330-change-busy-marker-for-threads.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_i7wuvhpx0 RnJvbSAxYzQ0ZmM5MzZkMDVmZWYzMjU5MzU0ZGExNTc0YzUzNmVkMTY5MWM3IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKYXNzaSBCcmFyIDxqYXN3aW5kZXIuc2luZ2hAbGluYXJvLm9y Zz4KRGF0ZTogVHVlLCAzMSBNYXIgMjAxNSAxMDoxNjo0NiArMDUzMApTdWJqZWN0OiBbUEFUQ0gg MS8yXSBkbWE6IHBsMzMwOiBjaGFuZ2UgYnVzeSBtYXJrZXIgZm9yIHRocmVhZHMKCkluc3RlYWQg b2YgYSBib29sZWFuIGZsYWcgdG8gbWFyayBhIHRocmVhZCBidXN5LCB1c2UgdGhlIG93bmVyIG9m CnRoZSB0aHJlYWQgYXMgdGhlIG1hcmtlci4gRm9yIGZyZWUvYXZhaWxhYmxlIHRocmVhZHMsIHRo ZSBvd25lciBpcwpOVUxMLiBUaGlzIHdpbGwgYmUgdXNlZnVsIGluIGZpbmRpbmcgd2hpY2ggY2hh bm5lbCBvd25zIGEgZ2l2ZW4KdGhyZWFkLgoKU2lnbmVkLW9mZi1ieTogSmFzc2kgQnJhciA8amFz d2luZGVyLnNpbmdoQGxpbmFyby5vcmc+Ci0tLQogZHJpdmVycy9kbWEvcGwzMzAuYyB8IDIwICsr KysrKysrKystLS0tLS0tLS0tCiAxIGZpbGUgY2hhbmdlZCwgMTAgaW5zZXJ0aW9ucygrKSwgMTAg ZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvZHJpdmVycy9kbWEvcGwzMzAuYyBiL2RyaXZlcnMv ZG1hL3BsMzMwLmMKaW5kZXggMGUxZjU2Ny4uZDFmNzc3ZSAxMDA2NDQKLS0tIGEvZHJpdmVycy9k bWEvcGwzMzAuYworKysgYi9kcml2ZXJzL2RtYS9wbDMzMC5jCkBAIC0zNjksOCArMzY5LDcgQEAg c3RydWN0IF9wbDMzMF90YmQgewogc3RydWN0IHBsMzMwX3RocmVhZCB7CiAJdTggaWQ7CiAJaW50 IGV2OwotCS8qIElmIHRoZSBjaGFubmVsIGlzIG5vdCB5ZXQgYWNxdWlyZWQgYnkgYW55IGNsaWVu dCAqLwotCWJvb2wgZnJlZTsKKwlzdHJ1Y3QgZG1hX3BsMzMwX2NoYW4gKnBjaDsKIAkvKiBQYXJl bnQgRE1BQyAqLwogCXN0cnVjdCBwbDMzMF9kbWFjICpkbWFjOwogCS8qIE9ubHkgdHdvIGF0IGEg dGltZSAqLwpAQCAtMTY0OCw3ICsxNjQ3LDggQEAgc3RhdGljIGJvb2wgX2NoYW5fbnMoY29uc3Qg c3RydWN0IHBsMzMwX2RtYWMgKnBsMzMwLCBpbnQgaSkKIC8qIFVwb24gc3VjY2VzcywgcmV0dXJu cyBJZGVudGl0eVRva2VuIGZvciB0aGUKICAqIGFsbG9jYXRlZCBjaGFubmVsLCBOVUxMIG90aGVy d2lzZS4KICAqLwotc3RhdGljIHN0cnVjdCBwbDMzMF90aHJlYWQgKnBsMzMwX3JlcXVlc3RfY2hh bm5lbChzdHJ1Y3QgcGwzMzBfZG1hYyAqcGwzMzApCitzdGF0aWMgc3RydWN0IHBsMzMwX3RocmVh ZCAqcGwzMzBfcmVxdWVzdF9jaGFubmVsKHN0cnVjdCBwbDMzMF9kbWFjICpwbDMzMCwKKwkJCQkJ CSAgc3RydWN0IGRtYV9wbDMzMF9jaGFuICpwY2gpCiB7CiAJc3RydWN0IHBsMzMwX3RocmVhZCAq dGhyZCA9IE5VTEw7CiAJdW5zaWduZWQgbG9uZyBmbGFnczsKQEAgLTE2NjMsMTEgKzE2NjMsMTEg QEAgc3RhdGljIHN0cnVjdCBwbDMzMF90aHJlYWQgKnBsMzMwX3JlcXVlc3RfY2hhbm5lbChzdHJ1 Y3QgcGwzMzBfZG1hYyAqcGwzMzApCiAKIAlmb3IgKGkgPSAwOyBpIDwgY2hhbnM7IGkrKykgewog CQl0aHJkID0gJnBsMzMwLT5jaGFubmVsc1tpXTsKLQkJaWYgKCh0aHJkLT5mcmVlKSAmJiAoIV9t YW5hZ2VyX25zKHRocmQpIHx8CisJCWlmICghdGhyZC0+cGNoICYmICghX21hbmFnZXJfbnModGhy ZCkgfHwKIAkJCQkJX2NoYW5fbnMocGwzMzAsIGkpKSkgewogCQkJdGhyZC0+ZXYgPSBfYWxsb2Nf ZXZlbnQodGhyZCk7CiAJCQlpZiAodGhyZC0+ZXYgPj0gMCkgewotCQkJCXRocmQtPmZyZWUgPSBm YWxzZTsKKwkJCQl0aHJkLT5wY2ggPSBwY2g7CiAJCQkJdGhyZC0+bHN0ZW5xID0gMTsKIAkJCQl0 aHJkLT5yZXFbMF0uZGVzYyA9IE5VTEw7CiAJCQkJdGhyZC0+cmVxWzFdLmRlc2MgPSBOVUxMOwpA QCAtMTY5OSw3ICsxNjk5LDcgQEAgc3RhdGljIHZvaWQgcGwzMzBfcmVsZWFzZV9jaGFubmVsKHN0 cnVjdCBwbDMzMF90aHJlYWQgKnRocmQpCiAJc3RydWN0IHBsMzMwX2RtYWMgKnBsMzMwOwogCXVu c2lnbmVkIGxvbmcgZmxhZ3M7CiAKLQlpZiAoIXRocmQgfHwgdGhyZC0+ZnJlZSkKKwlpZiAoIXRo cmQgfHwgIXRocmQtPnBjaCkKIAkJcmV0dXJuOwogCiAJX3N0b3AodGhyZCk7CkBAIC0xNzExLDcg KzE3MTEsNyBAQCBzdGF0aWMgdm9pZCBwbDMzMF9yZWxlYXNlX2NoYW5uZWwoc3RydWN0IHBsMzMw X3RocmVhZCAqdGhyZCkKIAogCXNwaW5fbG9ja19pcnFzYXZlKCZwbDMzMC0+bG9jaywgZmxhZ3Mp OwogCV9mcmVlX2V2ZW50KHRocmQsIHRocmQtPmV2KTsKLQl0aHJkLT5mcmVlID0gdHJ1ZTsKKwl0 aHJkLT5wY2ggPSBOVUxMOwogCXNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJnBsMzMwLT5sb2NrLCBm bGFncyk7CiB9CiAKQEAgLTE3OTcsMTQgKzE3OTcsMTQgQEAgc3RhdGljIGludCBkbWFjX2FsbG9j X3RocmVhZHMoc3RydWN0IHBsMzMwX2RtYWMgKnBsMzMwKQogCQl0aHJkLT5pZCA9IGk7CiAJCXRo cmQtPmRtYWMgPSBwbDMzMDsKIAkJX3Jlc2V0X3RocmVhZCh0aHJkKTsKLQkJdGhyZC0+ZnJlZSA9 IHRydWU7CisJCXRocmQtPnBjaCA9IE5VTEw7CiAJfQogCiAJLyogTUFOQUdFUiBpcyBpbmRleGVk IGF0IHRoZSBlbmQgKi8KIAl0aHJkID0gJnBsMzMwLT5jaGFubmVsc1tjaGFuc107CiAJdGhyZC0+ aWQgPSBjaGFuczsKIAl0aHJkLT5kbWFjID0gcGwzMzA7Ci0JdGhyZC0+ZnJlZSA9IGZhbHNlOwor CXRocmQtPnBjaCA9IE5VTEw7CiAJcGwzMzAtPm1hbmFnZXIgPSB0aHJkOwogCiAJcmV0dXJuIDA7 CkBAIC0yMDgyLDcgKzIwODIsNyBAQCBzdGF0aWMgaW50IHBsMzMwX2FsbG9jX2NoYW5fcmVzb3Vy Y2VzKHN0cnVjdCBkbWFfY2hhbiAqY2hhbikKIAlkbWFfY29va2llX2luaXQoY2hhbik7CiAJcGNo LT5jeWNsaWMgPSBmYWxzZTsKIAotCXBjaC0+dGhyZWFkID0gcGwzMzBfcmVxdWVzdF9jaGFubmVs KHBsMzMwKTsKKwlwY2gtPnRocmVhZCA9IHBsMzMwX3JlcXVlc3RfY2hhbm5lbChwbDMzMCwgcGNo KTsKIAlpZiAoIXBjaC0+dGhyZWFkKSB7CiAJCXNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJnBjaC0+ bG9jaywgZmxhZ3MpOwogCQlyZXR1cm4gLUVOT01FTTsKLS0gCjEuOS4xCgo= --20cf304271b0ab0f4205128ec183 Content-Type: text/x-patch; charset=US-ASCII; name="0002-dma-pl330-fix-race-between-trigger-and-completion.patch" Content-Disposition: attachment; filename="0002-dma-pl330-fix-race-between-trigger-and-completion.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_i7wuvhqi1 RnJvbSAyY2Q2YmY2NzQ4ZjI4MDA4YTE2NTBkY2E1N2E4ZjE0YjI3MjgzODAzIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKYXNzaSBCcmFyIDxqYXN3aW5kZXIuc2luZ2hAbGluYXJvLm9y Zz4KRGF0ZTogVHVlLCAzMSBNYXIgMjAxNSAxMDoyMToxNCArMDUzMApTdWJqZWN0OiBbUEFUQ0gg Mi8yXSBkbWE6IHBsMzMwOiBmaXggcmFjZSBiZXR3ZWVuIHRyaWdnZXIgYW5kIGNvbXBsZXRpb24K CldlIG5lZWQgdG8gaG9sZCB0aGUgbG9jayBvbiBjaGFubmVsIGluIElTUiB0byBwcmV2ZW50IGl0 CnJhY2luZyBhZ2FpbnN0IHRoZSB0cmlnZ2VyIGNhbGwgb24gdGhlIGNoYW5uZWwuCgpSZXBvcnRl ZC1ieTogU2NvdHQgQnJhbmRlbiA8c2JyYW5kZW5AYnJvYWRjb20uY29tPgpTaWduZWQtb2ZmLWJ5 OiBKYXNzaSBCcmFyIDxqYXN3aW5kZXIuc2luZ2hAbGluYXJvLm9yZz4KLS0tCiBkcml2ZXJzL2Rt YS9wbDMzMC5jIHwgNyArKysrKystCiAxIGZpbGUgY2hhbmdlZCwgNiBpbnNlcnRpb25zKCspLCAx IGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvZHJpdmVycy9kbWEvcGwzMzAuYyBiL2RyaXZlcnMv ZG1hL3BsMzMwLmMKaW5kZXggZDFmNzc3ZS4uY2U0MDY3NyAxMDA2NDQKLS0tIGEvZHJpdmVycy9k bWEvcGwzMzAuYworKysgYi9kcml2ZXJzL2RtYS9wbDMzMC5jCkBAIC0xNTczLDYgKzE1NzMsNyBA QCBzdGF0aWMgaW50IHBsMzMwX3VwZGF0ZShzdHJ1Y3QgcGwzMzBfZG1hYyAqcGwzMzApCiAJCWlm ICh2YWwgJiAoMSA8PCBldikpIHsgLyogRXZlbnQgb2NjdXJyZWQgKi8KIAkJCXN0cnVjdCBwbDMz MF90aHJlYWQgKnRocmQ7CiAJCQl1MzIgaW50ZW4gPSByZWFkbChyZWdzICsgSU5URU4pOworCQkJ dW5zaWduZWQgbG9uZyBmbGFnOwogCQkJaW50IGFjdGl2ZTsKIAogCQkJLyogQ2xlYXIgdGhlIGV2 ZW50ICovCkBAIC0xNTg0LDEwICsxNTg1LDEzIEBAIHN0YXRpYyBpbnQgcGwzMzBfdXBkYXRlKHN0 cnVjdCBwbDMzMF9kbWFjICpwbDMzMCkKIAkJCWlkID0gcGwzMzAtPmV2ZW50c1tldl07CiAKIAkJ CXRocmQgPSAmcGwzMzAtPmNoYW5uZWxzW2lkXTsKKwkJCXNwaW5fbG9ja19pcnFzYXZlKCZ0aHJk LT5wY2gtPmxvY2ssIGZsYWcpOwogCiAJCQlhY3RpdmUgPSB0aHJkLT5yZXFfcnVubmluZzsKLQkJ CWlmIChhY3RpdmUgPT0gLTEpIC8qIEFib3J0ZWQgKi8KKwkJCWlmIChhY3RpdmUgPT0gLTEpIHsg LyogQWJvcnRlZCAqLworCQkJCXNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJnRocmQtPnBjaC0+bG9j aywgZmxhZyk7CiAJCQkJY29udGludWU7CisJCQl9CiAKIAkJCS8qIERldGFjaCB0aGUgcmVxICov CiAJCQlkZXNjZG9uZSA9IHRocmQtPnJlcVthY3RpdmVdLmRlc2M7CkBAIC0xNjAwLDYgKzE2MDQs NyBAQCBzdGF0aWMgaW50IHBsMzMwX3VwZGF0ZShzdHJ1Y3QgcGwzMzBfZG1hYyAqcGwzMzApCiAK IAkJCS8qIEZvciBub3csIGp1c3QgbWFrZSBhIGxpc3Qgb2YgY2FsbGJhY2tzIHRvIGJlIGRvbmUg Ki8KIAkJCWxpc3RfYWRkX3RhaWwoJmRlc2Nkb25lLT5ycWQsICZwbDMzMC0+cmVxX2RvbmUpOwor CQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmdGhyZC0+cGNoLT5sb2NrLCBmbGFnKTsKIAkJfQog CX0KIAotLSAKMS45LjEKCg== --20cf304271b0ab0f4205128ec183-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: jassisinghbrar@gmail.com (Jassi Brar) Date: Tue, 31 Mar 2015 10:50:27 +0530 Subject: [PATCH] dmaengine: pl330: fix the race condition in pl330 driver. In-Reply-To: <551A173B.4010704@broadcom.com> References: <1427414105-3480-1-git-send-email-sbranden@broadcom.com> <20150330172546.GI7192@intel.com> <551A173B.4010704@broadcom.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Mar 31, 2015 at 9:10 AM, Scott Branden wrote: > Hi Vinod, Jassi, > > Some details on the problem encountered. > > > On 15-03-30 10:25 AM, Vinod Koul wrote: >> >> On Mon, Mar 30, 2015 at 10:17:17PM +0530, Jassi Brar wrote: >>> >>> On Fri, Mar 27, 2015 at 5:25 AM, Scott Branden >>> wrote: >>>> >>>> From: ismail >>>> >>>> Update the thread running index before issuing the >>>> GO command to the DMAC. >>>> >>>> Tested-by: Mohamed Ismail Abdul Packir Mohamed >>>> Reviewed-by: Ray Jui >>>> Reviewed-by: Arun Parameswaran >>>> Reviewed-by: Scott Branden >>>> Signed-off-by: Scott Branden >>>> Signed-off-by: Mohamed Ismail Abdul Packir Mohamed >>>> --- >>>> drivers/dma/pl330.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>> index 0e1f567..631642d 100644 >>>> --- a/drivers/dma/pl330.c >>>> +++ b/drivers/dma/pl330.c >>>> @@ -1072,11 +1072,11 @@ static bool _trigger(struct pl330_thread *thrd) >>>> /* Set to generate interrupts for SEV */ >>>> writel(readl(regs + INTEN) | (1 << thrd->ev), regs + INTEN); >>>> >>>> + thrd->req_running = idx; >>>> + >>>> /* Only manager can execute GO */ >>>> _execute_DBGINSN(thrd, insn, true); >>>> >>>> - thrd->req_running = idx; >>>> - >>> >>> It would help to know what the behavior looks like before and after >>> the patch. If anything we should look at locking rather the >>> reordering. >> >> Yes that ia fair request, looking at changelog it is hard to understand >> the >> issue seen? >> > We encountered this problem as we modified the driver to make SMC calls to a > TZ handler. This slowed down the driver to the point where DMA transactions > easily failed. I believe the same could be accomplished by adding a delay > between the GOCMD and update of the req_running and running the built in > dmatest. > > The DMA transaction is broken if the interrupt occurs before the > thrd->req_running is updated. > > The pl330 issues a GOCMD (in _trigger function) to start a new transfer. > > The issue of GOCMD generates an interrupt and the IRQ handler will call the > pl330_update function to process the interrupt. > > The pl330_update function will verify the thread running index and break the > transaction, if the thread running index is not set. > As I suspected the locking seems screwed up. The following patch should fix the race properly. Can you please test the attached patches instead? Thanks. -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-dma-pl330-change-busy-marker-for-threads.patch Type: text/x-patch Size: 3065 bytes Desc: not available URL: -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-dma-pl330-fix-race-between-trigger-and-completion.patch Type: text/x-patch Size: 1612 bytes Desc: not available URL: