From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751546AbbKINVR (ORCPT ); Mon, 9 Nov 2015 08:21:17 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:57431 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750856AbbKINVM convert rfc822-to-8bit (ORCPT ); Mon, 9 Nov 2015 08:21:12 -0500 X-Greylist: delayed 531 seconds by postgrey-1.27 at vger.kernel.org; Mon, 09 Nov 2015 08:21:11 EST Message-ID: <1447074736.22840.35.camel@collabora.co.uk> Subject: Re: [PATCH] dmaengine: pl330: Fix race in residue reporting From: Sjoerd Simons To: Krzysztof Kozlowski , Vinod Koul Cc: k.kozlowski.k@gmail.com, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, dmaengine@vger.kernel.org, linux-arm-kernel@vger.kernel.org Date: Mon, 09 Nov 2015 14:12:16 +0100 In-Reply-To: <563DF151.2010307@samsung.com> References: <1446808295-23149-1-git-send-email-sjoerd.simons@collabora.co.uk> <563DF151.2010307@samsung.com> Organization: Collabora Ltd. Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT X-Mailer: Evolution 3.18.1-1 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 2015-11-07 at 21:40 +0900, Krzysztof Kozlowski wrote: > W dniu 06.11.2015 o 20:11, Sjoerd Simons pisze: > > When a transfer completes there is a small window between the > > descriptor > > being unset as the current active one in the thread and it being > > marked > > as done. This causes the residue to be incorrectly set when > > pl330_tx_status is run in that window. Practically this caused > > issue for me with audio playback as the residue goes up during a > > transfer (as the in-progress transfer is no longer accounted for), > > which makes the higher levels think the audio ringbuffer wrapped > > around > > and thus did a sudden big jump forward. > > > > To resolve this, add a field protected by the dma engine lock to > > indicate the transfer is done but the status hasn't been updated > > yet. > > > > Also fix the locking in pl330_tx_status, as the function inspects > > the > > threads req_running field and queries the dma engine for the > > current > > state of the running transfer the dma engine lock needs to be held > > to > > ensure the active descriptor doesn't change underneath it. > > > > Signed-off-by: Sjoerd Simons > > > > --- > > > >  drivers/dma/pl330.c | 10 +++++++++- > >  1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index 17ee758..6c8243b 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -503,6 +503,8 @@ struct dma_pl330_desc { > >   struct pl330_reqcfg rqcfg; > >   > >   enum desc_status status; > > + /* Transfer completed, but not yet moved to DONE state */ > > + bool xferred; > >   > >   int bytes_requested; > >   bool last; > > @@ -1463,6 +1465,9 @@ static void dma_pl330_rqcb(struct > > dma_pl330_desc *desc, enum pl330_op_err err) > >   spin_lock_irqsave(&pch->lock, flags); > >   > >   desc->status = DONE; > > + spin_lock(&pch->thread->dmac->lock); > > + desc->xferred = false; > > + spin_unlock(&pch->thread->dmac->lock); > >   > >   spin_unlock_irqrestore(&pch->lock, flags); > >   > > @@ -1595,6 +1600,7 @@ static int pl330_update(struct pl330_dmac > > *pl330) > >   > >   /* Detach the req */ > >   descdone = thrd->req[active].desc; > > + descdone->xferred = true; > >   thrd->req[active].desc = NULL; > > Looking at the code indeed the small window could happen. How can I > reproduce it? Can you describe your system? I'm using a Rock 2 square (RK3288 based) board, running Debian Jessie and simply playing back audio using vlc & pulseaudio. For some reason when using vlc, pulseaudio is polling the hw pointer position very very often and seems to hit this tiny window a few times a minute depending a bit on system load. This causes some audiable issues and eventually underflowing (as vlc fills the pulseaudio buffer at a constant rate, but due to this, pulse pushes the data faster then the actual playback rate every so often). > As for the change itself, how about adding a new value to > desc_status? > Now you are actually introducing a semi-status field. The reason i picked this way this was was due to the current locking scheme. The locking order is channel, then controller (when locking both). While processing the events (and update the active descriptor), the controller is locked so the status can't be directly updated (as that's protected by the channel lock). And we can't grab the channel lock without dopping the controller one first :). Keeping one status field might well be possible, but would require at least reworking the locking here. > Best regards, > Krzysztof > > >   > >   thrd->req_running = -1; > > @@ -2250,13 +2256,14 @@ pl330_tx_status(struct dma_chan *chan, > > dma_cookie_t cookie, > >   goto out; > >   > >   spin_lock_irqsave(&pch->lock, flags); > > + spin_lock(&pch->thread->dmac->lock); > >   > >   if (pch->thread->req_running != -1) > >   running = pch->thread->req[pch->thread- > > >req_running].desc; > >   > >   /* Check in pending list */ > >   list_for_each_entry(desc, &pch->work_list, node) { > > - if (desc->status == DONE) > > + if (desc->xferred || desc->status == DONE) > >   transferred = desc->bytes_requested; > >   else if (running && desc == running) > >   transferred = > > @@ -2281,6 +2288,7 @@ pl330_tx_status(struct dma_chan *chan, > > dma_cookie_t cookie, > >   if (desc->last) > >   residual = 0; > >   } > > + spin_unlock(&pch->thread->dmac->lock); > >   spin_unlock_irqrestore(&pch->lock, flags); > >   > >  out: > > > -- Sjoerd Simons Collabora Ltd. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sjoerd Simons Subject: Re: [PATCH] dmaengine: pl330: Fix race in residue reporting Date: Mon, 09 Nov 2015 14:12:16 +0100 Message-ID: <1447074736.22840.35.camel@collabora.co.uk> References: <1446808295-23149-1-git-send-email-sjoerd.simons@collabora.co.uk> <563DF151.2010307@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <563DF151.2010307-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Krzysztof Kozlowski , Vinod Koul Cc: linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, k.kozlowski.k-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-samsung-soc@vger.kernel.org T24gU2F0LCAyMDE1LTExLTA3IGF0IDIxOjQwICswOTAwLCBLcnp5c3p0b2YgS296bG93c2tpIHdy b3RlOgo+IFcgZG5pdSAwNi4xMS4yMDE1IG8gMjA6MTEsIFNqb2VyZCBTaW1vbnMgcGlzemU6Cj4g PiBXaGVuIGEgdHJhbnNmZXIgY29tcGxldGVzIHRoZXJlIGlzIGEgc21hbGwgd2luZG93IGJldHdl ZW4gdGhlCj4gPiBkZXNjcmlwdG9yCj4gPiBiZWluZyB1bnNldCBhcyB0aGUgY3VycmVudCBhY3Rp dmUgb25lIGluIHRoZSB0aHJlYWQgYW5kIGl0IGJlaW5nCj4gPiBtYXJrZWQKPiA+IGFzIGRvbmUu IFRoaXMgY2F1c2VzIHRoZSByZXNpZHVlIHRvIGJlIGluY29ycmVjdGx5IHNldCB3aGVuCj4gPiBw bDMzMF90eF9zdGF0dXMgaXMgcnVuIGluIHRoYXQgd2luZG93LiBQcmFjdGljYWxseSB0aGlzIGNh dXNlZAo+ID4gaXNzdWUgZm9yIG1lIHdpdGggYXVkaW8gcGxheWJhY2sgYXMgdGhlIHJlc2lkdWUg Z29lcyB1cCBkdXJpbmcgYQo+ID4gdHJhbnNmZXIgKGFzIHRoZSBpbi1wcm9ncmVzcyB0cmFuc2Zl ciBpcyBubyBsb25nZXIgYWNjb3VudGVkIGZvciksCj4gPiB3aGljaCBtYWtlcyB0aGUgaGlnaGVy IGxldmVscyB0aGluayB0aGUgYXVkaW8gcmluZ2J1ZmZlciB3cmFwcGVkCj4gPiBhcm91bmQKPiA+ IGFuZCB0aHVzIGRpZCBhIHN1ZGRlbiBiaWcganVtcCBmb3J3YXJkLgo+ID4gCj4gPiBUbyByZXNv bHZlIHRoaXMsIGFkZCBhIGZpZWxkIHByb3RlY3RlZCBieSB0aGUgZG1hIGVuZ2luZSBsb2NrIHRv Cj4gPiBpbmRpY2F0ZSB0aGUgdHJhbnNmZXIgaXMgZG9uZSBidXQgdGhlIHN0YXR1cyBoYXNuJ3Qg YmVlbiB1cGRhdGVkCj4gPiB5ZXQuCj4gPiAKPiA+IEFsc28gZml4IHRoZSBsb2NraW5nIGluIHBs MzMwX3R4X3N0YXR1cywgYXMgdGhlIGZ1bmN0aW9uIGluc3BlY3RzCj4gPiB0aGUKPiA+IHRocmVh ZHMgcmVxX3J1bm5pbmcgZmllbGQgYW5kIHF1ZXJpZXMgdGhlIGRtYSBlbmdpbmUgZm9yIHRoZQo+ ID4gY3VycmVudAo+ID4gc3RhdGUgb2YgdGhlIHJ1bm5pbmcgdHJhbnNmZXIgdGhlIGRtYSBlbmdp bmUgbG9jayBuZWVkcyB0byBiZSBoZWxkCj4gPiB0bwo+ID4gZW5zdXJlIHRoZSBhY3RpdmUgZGVz Y3JpcHRvciBkb2Vzbid0IGNoYW5nZSB1bmRlcm5lYXRoIGl0Lgo+ID4gCj4gPiBTaWduZWQtb2Zm LWJ5OiBTam9lcmQgU2ltb25zIDxzam9lcmQuc2ltb25zQGNvbGxhYm9yYS5jby51az4KPiA+IAo+ ID4gLS0tCj4gPiAKPiA+IMKgZHJpdmVycy9kbWEvcGwzMzAuYyB8IDEwICsrKysrKysrKy0KPiA+ IMKgMSBmaWxlIGNoYW5nZWQsIDkgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+ID4gCj4g PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9kbWEvcGwzMzAuYyBiL2RyaXZlcnMvZG1hL3BsMzMwLmMK PiA+IGluZGV4IDE3ZWU3NTguLjZjODI0M2IgMTAwNjQ0Cj4gPiAtLS0gYS9kcml2ZXJzL2RtYS9w bDMzMC5jCj4gPiArKysgYi9kcml2ZXJzL2RtYS9wbDMzMC5jCj4gPiBAQCAtNTAzLDYgKzUwMyw4 IEBAIHN0cnVjdCBkbWFfcGwzMzBfZGVzYyB7Cj4gPiDCoAlzdHJ1Y3QgcGwzMzBfcmVxY2ZnIHJx Y2ZnOwo+ID4gwqAKPiA+IMKgCWVudW0gZGVzY19zdGF0dXMgc3RhdHVzOwo+ID4gKwkvKiBUcmFu c2ZlciBjb21wbGV0ZWQsIGJ1dCBub3QgeWV0IG1vdmVkIHRvIERPTkUgc3RhdGUgKi8KPiA+ICsJ Ym9vbCB4ZmVycmVkOwo+ID4gwqAKPiA+IMKgCWludCBieXRlc19yZXF1ZXN0ZWQ7Cj4gPiDCoAli b29sIGxhc3Q7Cj4gPiBAQCAtMTQ2Myw2ICsxNDY1LDkgQEAgc3RhdGljIHZvaWQgZG1hX3BsMzMw X3JxY2Ioc3RydWN0Cj4gPiBkbWFfcGwzMzBfZGVzYyAqZGVzYywgZW51bSBwbDMzMF9vcF9lcnIg ZXJyKQo+ID4gwqAJc3Bpbl9sb2NrX2lycXNhdmUoJnBjaC0+bG9jaywgZmxhZ3MpOwo+ID4gwqAK PiA+IMKgCWRlc2MtPnN0YXR1cyA9IERPTkU7Cj4gPiArCXNwaW5fbG9jaygmcGNoLT50aHJlYWQt PmRtYWMtPmxvY2spOwo+ID4gKwlkZXNjLT54ZmVycmVkID0gZmFsc2U7Cj4gPiArCXNwaW5fdW5s b2NrKCZwY2gtPnRocmVhZC0+ZG1hYy0+bG9jayk7Cj4gPiDCoAo+ID4gwqAJc3Bpbl91bmxvY2tf aXJxcmVzdG9yZSgmcGNoLT5sb2NrLCBmbGFncyk7Cj4gPiDCoAo+ID4gQEAgLTE1OTUsNiArMTYw MCw3IEBAIHN0YXRpYyBpbnQgcGwzMzBfdXBkYXRlKHN0cnVjdCBwbDMzMF9kbWFjCj4gPiAqcGwz MzApCj4gPiDCoAo+ID4gwqAJCQkvKiBEZXRhY2ggdGhlIHJlcSAqLwo+ID4gwqAJCQlkZXNjZG9u ZSA9IHRocmQtPnJlcVthY3RpdmVdLmRlc2M7Cj4gPiArCQkJZGVzY2RvbmUtPnhmZXJyZWQgPSB0 cnVlOwo+ID4gwqAJCQl0aHJkLT5yZXFbYWN0aXZlXS5kZXNjID0gTlVMTDsKPiAKPiBMb29raW5n IGF0IHRoZSBjb2RlIGluZGVlZCB0aGUgc21hbGwgd2luZG93IGNvdWxkIGhhcHBlbi4gSG93IGNh biBJCj4gcmVwcm9kdWNlIGl0PyBDYW4geW91IGRlc2NyaWJlIHlvdXIgc3lzdGVtPwoKSSdtIHVz aW5nIGEgUm9jayAyIHNxdWFyZSAoUkszMjg4IGJhc2VkKSBib2FyZCwgcnVubmluZyBEZWJpYW4g SmVzc2llCmFuZCBzaW1wbHkgcGxheWluZyBiYWNrIGF1ZGlvIHVzaW5nIHZsYyAmIHB1bHNlYXVk aW8uCgpGb3Igc29tZSByZWFzb24gd2hlbiB1c2luZyB2bGMsIHB1bHNlYXVkaW8gaXMgcG9sbGlu ZyB0aGUgaHcgcG9pbnRlcgpwb3NpdGlvbiB2ZXJ5IHZlcnkgb2Z0ZW4gYW5kIHNlZW1zIHRvIGhp dCB0aGlzIHRpbnkgd2luZG93IGEgZmV3IHRpbWVzCmEgbWludXRlIGRlcGVuZGluZyBhIGJpdCBv biBzeXN0ZW0gbG9hZC4gVGhpcyBjYXVzZXMgc29tZSBhdWRpYWJsZQppc3N1ZXMgYW5kIGV2ZW50 dWFsbHkgdW5kZXJmbG93aW5nIChhcyB2bGMgZmlsbHMgdGhlIHB1bHNlYXVkaW8gYnVmZmVyCmF0 IGEgY29uc3RhbnQgcmF0ZSwgYnV0IGR1ZSB0byB0aGlzLCBwdWxzZSBwdXNoZXMgdGhlIGRhdGEg ZmFzdGVyIHRoZW4KdGhlIGFjdHVhbCBwbGF5YmFjayByYXRlIGV2ZXJ5IHNvIG9mdGVuKS4KCj4g QXMgZm9yIHRoZSBjaGFuZ2UgaXRzZWxmLCBob3cgYWJvdXQgYWRkaW5nIGEgbmV3IHZhbHVlIHRv Cj4gZGVzY19zdGF0dXM/Cj4gTm93IHlvdSBhcmUgYWN0dWFsbHkgaW50cm9kdWNpbmcgYSBzZW1p LXN0YXR1cyBmaWVsZC4KClRoZSByZWFzb24gaSBwaWNrZWQgdGhpcyB3YXkgdGhpcyB3YXMgd2Fz IGR1ZSB0byB0aGUgY3VycmVudCBsb2NraW5nCnNjaGVtZS4gVGhlIGxvY2tpbmcgb3JkZXIgaXMg Y2hhbm5lbCwgdGhlbiBjb250cm9sbGVyICh3aGVuIGxvY2tpbmcKYm90aCkuIFdoaWxlIHByb2Nl c3NpbmcgdGhlIGV2ZW50cyAoYW5kIHVwZGF0ZSB0aGUgYWN0aXZlIGRlc2NyaXB0b3IpLAp0aGUg Y29udHJvbGxlciBpcyBsb2NrZWQgc28gdGhlIHN0YXR1cyBjYW4ndCBiZSBkaXJlY3RseSB1cGRh dGVkIChhcwp0aGF0J3MgcHJvdGVjdGVkIGJ5IHRoZSBjaGFubmVsIGxvY2spLiBBbmQgd2UgY2Fu J3QgZ3JhYiB0aGUgY2hhbm5lbApsb2NrIHdpdGhvdXQgZG9wcGluZyB0aGUgY29udHJvbGxlciBv bmUgZmlyc3QgOikuCgpLZWVwaW5nIG9uZSBzdGF0dXMgZmllbGQgbWlnaHQgd2VsbCBiZSBwb3Nz aWJsZSwgYnV0IHdvdWxkIHJlcXVpcmUgYXQKbGVhc3QgcmV3b3JraW5nIHRoZSBsb2NraW5nIGhl cmUuCgoKCj4gQmVzdCByZWdhcmRzLAo+IEtyenlzenRvZgo+IAo+ID4gwqAKPiA+IMKgCQkJdGhy ZC0+cmVxX3J1bm5pbmcgPSAtMTsKPiA+IEBAIC0yMjUwLDEzICsyMjU2LDE0IEBAIHBsMzMwX3R4 X3N0YXR1cyhzdHJ1Y3QgZG1hX2NoYW4gKmNoYW4sCj4gPiBkbWFfY29va2llX3QgY29va2llLAo+ ID4gwqAJCWdvdG8gb3V0Owo+ID4gwqAKPiA+IMKgCXNwaW5fbG9ja19pcnFzYXZlKCZwY2gtPmxv Y2ssIGZsYWdzKTsKPiA+ICsJc3Bpbl9sb2NrKCZwY2gtPnRocmVhZC0+ZG1hYy0+bG9jayk7Cj4g PiDCoAo+ID4gwqAJaWYgKHBjaC0+dGhyZWFkLT5yZXFfcnVubmluZyAhPSAtMSkKPiA+IMKgCQly dW5uaW5nID0gcGNoLT50aHJlYWQtPnJlcVtwY2gtPnRocmVhZC0KPiA+ID5yZXFfcnVubmluZ10u ZGVzYzsKPiA+IMKgCj4gPiDCoAkvKiBDaGVjayBpbiBwZW5kaW5nIGxpc3QgKi8KPiA+IMKgCWxp c3RfZm9yX2VhY2hfZW50cnkoZGVzYywgJnBjaC0+d29ya19saXN0LCBub2RlKSB7Cj4gPiAtCQlp ZiAoZGVzYy0+c3RhdHVzID09IERPTkUpCj4gPiArCQlpZiAoZGVzYy0+eGZlcnJlZCB8fCBkZXNj LT5zdGF0dXMgPT0gRE9ORSkKPiA+IMKgCQkJdHJhbnNmZXJyZWQgPSBkZXNjLT5ieXRlc19yZXF1 ZXN0ZWQ7Cj4gPiDCoAkJZWxzZSBpZiAocnVubmluZyAmJiBkZXNjID09IHJ1bm5pbmcpCj4gPiDC oAkJCXRyYW5zZmVycmVkID0KPiA+IEBAIC0yMjgxLDYgKzIyODgsNyBAQCBwbDMzMF90eF9zdGF0 dXMoc3RydWN0IGRtYV9jaGFuICpjaGFuLAo+ID4gZG1hX2Nvb2tpZV90IGNvb2tpZSwKPiA+IMKg CQlpZiAoZGVzYy0+bGFzdCkKPiA+IMKgCQkJcmVzaWR1YWwgPSAwOwo+ID4gwqAJfQo+ID4gKwlz cGluX3VubG9jaygmcGNoLT50aHJlYWQtPmRtYWMtPmxvY2spOwo+ID4gwqAJc3Bpbl91bmxvY2tf aXJxcmVzdG9yZSgmcGNoLT5sb2NrLCBmbGFncyk7Cj4gPiDCoAo+ID4gwqBvdXQ6Cj4gPiAKPiAK Ci0tIApTam9lcmQgU2ltb25zCkNvbGxhYm9yYSBMdGQuCgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpMaW51eC1yb2NrY2hpcCBtYWlsaW5nIGxpc3QKTGlu dXgtcm9ja2NoaXBAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LXJvY2tjaGlwCg==