From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH] android: fix warning when releasing active sync point Date: Wed, 16 Dec 2015 09:36:11 +0100 Message-ID: <5671227B.3060708@linux.intel.com> References: <20151215012955.GA28277@dtor-ws> <566FE4E1.2040005@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 3265A6E3F3 for ; Wed, 16 Dec 2015 00:36:30 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Dmitry Torokhov Cc: devel@driverdev.osuosl.org, Andrew Bresticker , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOl?= , dri-devel@lists.freedesktop.org, "linux-kernel@vger.kernel.org" , Riley Andrews , linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org T3AgMTUtMTItMTUgb20gMTg6MTkgc2NocmVlZiBEbWl0cnkgVG9yb2tob3Y6Cj4gT24gVHVlLCBE ZWMgMTUsIDIwMTUgYXQgMjowMSBBTSwgTWFhcnRlbiBMYW5raG9yc3QKPiA8bWFhcnRlbi5sYW5r aG9yc3RAbGludXguaW50ZWwuY29tPiB3cm90ZToKPj4gT3AgMTUtMTItMTUgb20gMDI6Mjkgc2No cmVlZiBEbWl0cnkgVG9yb2tob3Y6Cj4+PiBVc2Vyc3BhY2UgY2FuIGNsb3NlIHRoZSBzeW5jIGRl dmljZSB3aGlsZSB0aGVyZSBhcmUgc3RpbGwgYWN0aXZlIGZlbmNlCj4+PiBwb2ludHMsIGluIHdo aWNoIGNhc2Uga2VybmVsIHByb2R1Y2VzIHRoZSBmb2xsb3dpbmcgd2FybmluZzoKPj4+Cj4+PiBb ICAgNDMuODUzMTc2XSAtLS0tLS0tLS0tLS1bIGN1dCBoZXJlIF0tLS0tLS0tLS0tLS0KPj4+IFsg ICA0My44NTc4MzRdIFdBUk5JTkc6IENQVTogMCBQSUQ6IDg5MiBhdCAvbW50L2hvc3Qvc291cmNl L3NyYy90aGlyZF9wYXJ0eS9rZXJuZWwvdjMuMTgvZHJpdmVycy9zdGFnaW5nL2FuZHJvaWQvc3lu Yy5jOjQzOSBhbmRyb2lkX2ZlbmNlX3JlbGVhc2UrMHg4OC8weDEwNCgpCj4+PiBbICAgNDMuODcx NzQxXSBDUFU6IDAgUElEOiA4OTIgQ29tbTogQmluZGVyXzUgVGFpbnRlZDogRyAgICAgVSAzLjE4 LjAtMDc2NjEtZzA1NTBjZTkgIzEKPj4+IFsgICA0My44ODAxNzZdIEhhcmR3YXJlIG5hbWU6IEdv b2dsZSBUZWdyYTIxMCBTbWF1ZyBSZXYgMSsgKERUKQo+Pj4gWyAgIDQzLjg4NTgzNF0gQ2FsbCB0 cmFjZToKPj4+IFsgICA0My44ODgyOTRdIFs8ZmZmZmZmYzAwMDIwNzQ2ND5dIGR1bXBfYmFja3Ry YWNlKzB4MC8weDEwYwo+Pj4gWyAgIDQzLjg5MzY5N10gWzxmZmZmZmZjMDAwMjA3NTgwPl0gc2hv d19zdGFjaysweDEwLzB4MWMKPj4+IFsgICA0My44OTg3NTZdIFs8ZmZmZmZmYzAwMGFiMTI1OD5d IGR1bXBfc3RhY2srMHg3NC8weGI4Cj4+PiBbICAgNDMuOTAzODE0XSBbPGZmZmZmZmMwMDAyMWQ0 MTQ+XSB3YXJuX3Nsb3dwYXRoX2NvbW1vbisweDg0LzB4YjAKPj4+IFsgICA0My45MDk3MzZdIFs8 ZmZmZmZmYzAwMDIxZDUzMD5dIHdhcm5fc2xvd3BhdGhfbnVsbCsweDE0LzB4MjAKPj4+IFsgICA0 My45MTU0ODJdIFs8ZmZmZmZmYzAwMDg4YWVmYz5dIGFuZHJvaWRfZmVuY2VfcmVsZWFzZSsweDg0 LzB4MTA0Cj4+PiBbICAgNDMuOTIxNTgyXSBbPGZmZmZmZmMwMDA2NzFjYzQ+XSBmZW5jZV9yZWxl YXNlKzB4MTA0LzB4MTM0Cj4+PiBbICAgNDMuOTI3MDY2XSBbPGZmZmZmZmMwMDA4OGIwY2M+XSBz eW5jX2ZlbmNlX2ZyZWUrMHg3NC8weDljCj4+PiBbICAgNDMuOTMyNTUyXSBbPGZmZmZmZmMwMDA4 OGIxMjg+XSBzeW5jX2ZlbmNlX3JlbGVhc2UrMHgzNC8weDQ4Cj4+PiBbICAgNDMuOTM4MzA0XSBb PGZmZmZmZmMwMDAzMTdiYmM+XSBfX2ZwdXQrMHgxMDAvMHgxYjgKPj4+IFsgICA0My45NDMxODVd IFs8ZmZmZmZmYzAwMDMxN2NjOD5dIF9fX19mcHV0KzB4OC8weDE0Cj4+PiBbICAgNDMuOTQ3OTgy XSBbPGZmZmZmZmMwMDAyMzdmMzg+XSB0YXNrX3dvcmtfcnVuKzB4YjAvMHhlNAo+Pj4gWyAgIDQz Ljk1MzI5N10gWzxmZmZmZmZjMDAwMjA3MDc0Pl0gZG9fbm90aWZ5X3Jlc3VtZSsweDQ0LzB4NWMK Pj4+IFsgICA0My45NTg4NjddIC0tLVsgZW5kIHRyYWNlIDVhMmFhNDAyN2NjNWQxNzEgXS0tLQo+ Pj4KPj4+IExldCdzIGZpeCBpdCBieSBpbnRyb2R1Y2luZyBhIG5ldyBvcHRpb25hbCBjYWxsYmFj ayAoZGlzYWJsZV9zaWduYWxpbmcpCj4+PiB0byBmZW5jZSBvcGVyYXRpb25zIHNvIHRoYXQgZHJp dmVycyBjYW4gZG8gcHJvcGVyIGNsZWFuIHVwcyB3aGVuIHdlCj4+PiByZW1vdmUgbGFzdCBjYWxs YmFjayBmb3IgZ2l2ZW4gZmVuY2UuCj4+Pgo+Pj4gUmV2aWV3ZWQtYnk6IEFuZHJldyBCcmVzdGlj a2VyIDxhYnJlc3RpY0BjaHJvbWl1bS5vcmc+Cj4+PiBTaWduZWQtb2ZmLWJ5OiBEbWl0cnkgVG9y b2tob3YgPGR0b3JAY2hyb21pdW0ub3JnPgo+Pj4KPj4gTkFDSyEgVGhlcmUncyBubyB3YXkgdG8g ZG8gdGhpcyByYWNlIGZyZWUuCj4gQ2FuIHlvdSBwbGVhc2UgZXhwbGFpbiB0aGUgcmFjZSBiZWNh dXNlIGFzIGZhciBhcyBJIGNhbiBzZWUgdGhlcmUgaXMgbm90IG9uZS5cClRoZSBlbnRpcmUgY29k ZSBpbiBmZW5jZS5jIGFzc3VtZXMgdGhhdCBhIGZlbmNlIGNhbiBvbmx5IGdvIGZyb20gbm90IGVu YWJsZV9zaWduYWxpbmcgdG8gLmVuYWJsZV9zaWduYWxpbmcuIC5lbmFibGVfc2lnbmFsaW5nIGlz IG5vdCByZWZjb3VudGVkIHNvIDIgY2FsbHMgdG8gLmVuYWJsZV9kaXNhYmxpbmcgYW5kIDEgdG8g LmRpc2FibGVfc2lnbmFsaW5nIHdvdWxkIG1lc3MgdXAuCkZ1cnRoZXJtb3JlIHdlIHRyeSB0byBt YWtlIHN1cmUgdGhhdCBmZW5jZV9zaWduYWwgZG9lc24ndCB0YWtlIGxvY2tzIGlmIGl0cyB1bm5l ZWRlZC4gV2l0aCBhIGRpc2FibGVfc2lnbmFsaW5nIGNhbGxiYWNrIHlvdSB3b3VsZCBhbHdheXMg bmVlZCBsb2Nrcy4KClRvIGdldCByaWQgb2YgdGhlc2Ugd2FybmluZ3MgbWFrZSBzdXJlIHRoYXQg dGhlcmUncyBhIHJlZmNvdW50IG9uIHRoZSBmZW5jZSB1bnRpbCBpdCdzIHNpZ25hbGVkLgo+PiBU aGUgZHJpdmVyIHNob3VsZCBob2xkIGEgcmVmY291bnQgdW50aWwgZmVuY2UgaXMgc2lnbmFsZWQu Cj4gSWYgd2UgYXJlIG5vIGxvbmdlciBpbnRlcmVzdGVkIGluIGZlbmNlIHdoeSBkbyB3ZSBuZWVk IHRvIHdhaXQgZm9yIHRoZQo+IGZlbmNlIHRvIGJlIHNpZ25hbGVkPwpJdCdzIHRoZSBwYXJ0IG9m IHRoZSBkZXNpZ24uIEEgZHJpdmVyIHRyYWNrcyBpdHMgb3V0c3RhbmRpbmcgcmVxdWVzdHMgYW5k IHN1Ym1pc3Npb25zLCBhbmQgZXZlcnkgc3VibWlzc2lvbiBoYXMgaXRzIG93biBmZW5jZS4gQmVm b3JlIHRoZSBkcml2ZXIgcmVsZWFzZXMgaXRzIGZpbmFsIHJlZiB0aGUgcmVxdWVzdCBzaG91bGQg YmUgY29tcGxldGVkIG9yIGFib3J0ZWQuIEluIGVpdGhlciBjYXNlIGl0IG11c3QgY2FsbCBmZW5j ZV9zaWduYWwuCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f CmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpo dHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mga11.intel.com ([192.55.52.93]:33355 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753635AbbLPIga (ORCPT ); Wed, 16 Dec 2015 03:36:30 -0500 Subject: Re: [PATCH] android: fix warning when releasing active sync point To: Dmitry Torokhov References: <20151215012955.GA28277@dtor-ws> <566FE4E1.2040005@linux.intel.com> Cc: Greg Kroah-Hartman , Sumit Semwal , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOl?= , Riley Andrews , Andrew Bresticker , linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, "linux-kernel@vger.kernel.org" , devel@driverdev.osuosl.org From: Maarten Lankhorst Message-ID: <5671227B.3060708@linux.intel.com> Date: Wed, 16 Dec 2015 09:36:11 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Op 15-12-15 om 18:19 schreef Dmitry Torokhov: > On Tue, Dec 15, 2015 at 2:01 AM, Maarten Lankhorst > wrote: >> Op 15-12-15 om 02:29 schreef Dmitry Torokhov: >>> Userspace can close the sync device while there are still active fence >>> points, in which case kernel produces the following warning: >>> >>> [ 43.853176] ------------[ cut here ]------------ >>> [ 43.857834] WARNING: CPU: 0 PID: 892 at /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439 android_fence_release+0x88/0x104() >>> [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 3.18.0-07661-g0550ce9 #1 >>> [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT) >>> [ 43.885834] Call trace: >>> [ 43.888294] [] dump_backtrace+0x0/0x10c >>> [ 43.893697] [] show_stack+0x10/0x1c >>> [ 43.898756] [] dump_stack+0x74/0xb8 >>> [ 43.903814] [] warn_slowpath_common+0x84/0xb0 >>> [ 43.909736] [] warn_slowpath_null+0x14/0x20 >>> [ 43.915482] [] android_fence_release+0x84/0x104 >>> [ 43.921582] [] fence_release+0x104/0x134 >>> [ 43.927066] [] sync_fence_free+0x74/0x9c >>> [ 43.932552] [] sync_fence_release+0x34/0x48 >>> [ 43.938304] [] __fput+0x100/0x1b8 >>> [ 43.943185] [] ____fput+0x8/0x14 >>> [ 43.947982] [] task_work_run+0xb0/0xe4 >>> [ 43.953297] [] do_notify_resume+0x44/0x5c >>> [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]--- >>> >>> Let's fix it by introducing a new optional callback (disable_signaling) >>> to fence operations so that drivers can do proper clean ups when we >>> remove last callback for given fence. >>> >>> Reviewed-by: Andrew Bresticker >>> Signed-off-by: Dmitry Torokhov >>> >> NACK! There's no way to do this race free. > Can you please explain the race because as far as I can see there is not one.\ The entire code in fence.c assumes that a fence can only go from not enable_signaling to .enable_signaling. .enable_signaling is not refcounted so 2 calls to .enable_disabling and 1 to .disable_signaling would mess up. Furthermore we try to make sure that fence_signal doesn't take locks if its unneeded. With a disable_signaling callback you would always need locks. To get rid of these warnings make sure that there's a refcount on the fence until it's signaled. >> The driver should hold a refcount until fence is signaled. > If we are no longer interested in fence why do we need to wait for the > fence to be signaled? It's the part of the design. A driver tracks its outstanding requests and submissions, and every submission has its own fence. Before the driver releases its final ref the request should be completed or aborted. In either case it must call fence_signal.