From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Kroah-Hartman Subject: Re: [RFC 1/2] core: Add generic object registry implementation Date: Wed, 5 Nov 2014 18:18:15 -0800 Message-ID: <20141106021815.GA17253@kroah.com> References: <1415118568-18771-1-git-send-email-thierry.reding@gmail.com> <20141104163845.GA369@kroah.com> <20141105091351.GA12033@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) by gabe.freedesktop.org (Postfix) with ESMTP id 8EB5D6ECDA for ; Wed, 5 Nov 2014 18:18:16 -0800 (PST) Content-Disposition: inline In-Reply-To: <20141105091351.GA12033@ulmo> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thierry Reding Cc: Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCBOb3YgMDUsIDIwMTQgYXQgMTA6MTM6NTNBTSArMDEwMCwgVGhpZXJyeSBSZWRpbmcg d3JvdGU6Cj4gT24gVHVlLCBOb3YgMDQsIDIwMTQgYXQgMDg6Mzg6NDVBTSAtMDgwMCwgR3JlZyBL cm9haC1IYXJ0bWFuIHdyb3RlOgo+ID4gT24gVHVlLCBOb3YgMDQsIDIwMTQgYXQgMDU6Mjk6MjdQ TSArMDEwMCwgVGhpZXJyeSBSZWRpbmcgd3JvdGU6Cj4gWy4uLl0KPiA+ID4gZGlmZiAtLWdpdCBh L2RyaXZlcnMvYmFzZS9yZWdpc3RyeS5jIGIvZHJpdmVycy9iYXNlL3JlZ2lzdHJ5LmMKPiBbLi4u XQo+ID4gPiArLyoqCj4gPiA+ICsgKiByZWdpc3RyeV9yZWNvcmRfcmVmIC0gcmVmZXJlbmNlIG9u IHRoZSByZWdpc3RyeSByZWNvcmQKPiA+ID4gKyAqIEByZWNvcmQ6IHJlY29yZCB0byByZWZlcmVu Y2UKPiA+ID4gKyAqCj4gPiA+ICsgKiBJbmNyZWFzZXMgdGhlIHJlZmVyZW5jZSBjb3VudCBvbiB0 aGUgcmVjb3JkIGFuZCByZXR1cm5zIGEgcG9pbnRlciB0byBpdC4KPiA+ID4gKyAqCj4gPiA+ICsg KiBSZXR1cm46IEEgcG9pbnRlciB0byB0aGUgcmVjb3JkIG9uIHN1Y2Nlc3Mgb3IgTlVMTCBvbiBm YWlsdXJlLgo+ID4gPiArICovCj4gPiA+ICtzdHJ1Y3QgcmVnaXN0cnlfcmVjb3JkICpyZWdpc3Ry eV9yZWNvcmRfcmVmKHN0cnVjdCByZWdpc3RyeV9yZWNvcmQgKnJlY29yZCkKPiA+ID4gK3sKPiA+ ID4gKwlpZiAoIXJlY29yZCkKPiA+ID4gKwkJcmV0dXJuIE5VTEw7Cj4gPiA+ICsKPiA+ID4gKwkv Kgo+ID4gPiArCSAqIFJlZnVzZSB0byBnaXZlIG91dCBhbnkgbW9yZSByZWZlcmVuY2VzIGlmIHRo ZSBtb2R1bGUgb3duaW5nIHRoZQo+ID4gPiArCSAqIHJlY29yZCBpcyBiZWluZyByZW1vdmVkLgo+ ID4gPiArCSAqLwo+ID4gPiArCWlmICghdHJ5X21vZHVsZV9nZXQocmVjb3JkLT5vd25lcikpCj4g PiA+ICsJCXJldHVybiBOVUxMOwo+ID4gPiArCj4gPiA+ICsJa3JlZl9nZXQoJnJlY29yZC0+a3Jl Zik7Cj4gPiAKPiA+IFlvdSAicHJvdGVjdCIgZnJvbSBhIG1vZHVsZSBiZWluZyByZW1vdmVkLCBi dXQgbm90IGZyb20gc29tZW9uZSBlbHNlCj4gPiByZWxlYXNpbmcgdGhlIGtyZWYgYXQgdGhlIHNh bWUgdGltZS4gIFdoZXJlIGlzIHRoZSBsb2NrIHRoYXQgcHJldmVudHMKPiA+IHRoaXMgZnJvbSBo YXBwZW5pbmc/Cj4gCj4gWW91J3JlIHJpZ2h0LCB3ZSBuZWVkIGEgcmVjb3JkIGxvY2sgdG8gc2Vy aWFsaXplIHJlZi91bnJlZi4KPiAKPiA+IEFuZCBkbyB3ZSByZWFsbHkgY2FyZSBhYm91dCBtb2R1 bGUgcmVmZXJlbmNlIGNvdW50cyBoZXJlPwo+IAo+IFdlIG5lZWQgdGhpcyBzbyB0aGF0IHRoZSBj b2RlIG9mIHRoZSAucmVsZWFzZSgpIGNhbGxiYWNrIHN0YXlzIGluCj4gbWVtb3J5IGFzIGxvbmcg YXMgdGhlcmUgYXJlIGFueSByZWZlcmVuY2VzLgo+IAo+IEFsc28gd2UgbmVlZCB0aGUgbW9kdWxl IHJlZmVyZW5jZSBjb3VudCBmb3IgcmVnaXN0cmllcyBiZWNhdXNlIHRoZXkKPiB3b3VsZCB0eXBp Y2FsbHkgYmUgc3RhdGljYWxseSBhbGxvY2F0ZWQgYW5kIGdvIGF3YXkgYWxvbmcgd2l0aCBhIG1v ZHVsZQo+IHdoZW4gaXQgaXMgdW5sb2FkZWQuCgpOZXZlciB1c2UgYSAnc3RhdGljJyB2YXJpYWJs ZSBhcyBhIGR5bmFtaWMgb25lIHdpdGggYSBrcmVmLCB0aGF0J3MganVzdAphc2tpbmcgZm9yIHRy b3VibGUuCgo+ID4gPiArLyoqCj4gPiA+ICsgKiByZWdpc3RyeV9hZGQgLSBhZGQgYSByZWNvcmQg dG8gYSByZWdpc3RyeQo+ID4gPiArICogQHJlZ2lzdHJ5OiByZWdpc3RyeSB0byBhZGQgdGhlIHJl Y29yZCB0bwo+ID4gPiArICogQHJlY29yZDogcmVjb3JkIHRvIGFkZAo+ID4gPiArICoKPiA+ID4g KyAqIFRyaWVzIHRvIGluY3JlYXNlIHRoZSByZWZlcmVuY2UgY291bnQgb2YgdGhlIG1vZHVsZSBv d25pbmcgdGhlIHJlZ2lzdHJ5LiBJZgo+ID4gPiArICogc3VjY2Vzc2Z1bCBhZGRzIHRoZSBuZXcg cmVjb3JkIHRvIHRoZSByZWdpc3RyeS4KPiA+ID4gKyAqCj4gPiA+ICsgKiBSZXR1cm46IDAgb24g c3VjY2VzcyBvciBhIG5lZ2F0aXZlIGVycm9yIGNvZGUgb24gZmFpbHVyZS4KPiA+ID4gKyAqLwo+ ID4gPiAraW50IHJlZ2lzdHJ5X2FkZChzdHJ1Y3QgcmVnaXN0cnkgKnJlZ2lzdHJ5LCBzdHJ1Y3Qg cmVnaXN0cnlfcmVjb3JkICpyZWNvcmQpCj4gPiA+ICt7Cj4gPiA+ICsJaWYgKCF0cnlfbW9kdWxl X2dldChyZWdpc3RyeS0+b3duZXIpKQo+ID4gPiArCQlyZXR1cm4gLUVOT0RFVjsKPiA+ID4gKwo+ ID4gPiArCW11dGV4X2xvY2soJnJlZ2lzdHJ5LT5sb2NrKTsKPiA+ID4gKwlsaXN0X2FkZF90YWls KCZyZWNvcmQtPmxpc3QsICZyZWdpc3RyeS0+bGlzdCk7Cj4gPiA+ICsJbXV0ZXhfdW5sb2NrKCZy ZWdpc3RyeS0+bG9jayk7Cj4gPiAKPiA+IE5vIGluY3JlbWVudGluZyBvZiB0aGUgcmVmZXJlbmNl IG9mIHRoZSByZWNvcmQgYXQgYWxsPwo+IAo+IEknbSBub3Qgc3VyZSBpZiB3ZSByZWFsbHkgbmVl ZCBvbmUuIERyaXZlcnMgd2lsbCBoYXZlIHRvIHJlbW92ZSB0aGUKPiByZWNvcmQgZnJvbSB0aGUg cmVnaXN0cnkgYmVmb3JlIGRyb3BwaW5nIHRoZWlyIHJlZmVyZW5jZS4gSSBndWVzcyB3ZQo+IGNv dWxkIHRocm93IGluIGFub3RoZXIga3JlZl9nZXQoKSBoZXJlIChhbmQgYSBrcmVmX3B1dCgpIGlu Cj4gcmVnaXN0cnlfcmVtb3ZlKCkpIGZvciBnb29kIG1lYXN1cmUgdG8gcHJldmVudCBicmVha2Fn ZSB3aXRoIGJ1Z2d5Cj4gZHJpdmVycy4KCk9yIGF0IGxlYXN0IHdhcm4gcGVvcGxlIHRoYXQgdGhl eSBuZWVkIHRvIGNsZWFuIHN0dWZmIHVwIHByb3Blcmx5IGlmCnRoZXkgZG8gbm90LCBvdGhlcndp c2UgdGhleSB3aWxsIGdldCBpdCB3cm9uZy4gIFlvdSBuZWVkIHRvIG1ha2UgaXQgdmVyeQpoYXJk IHRvIGdldCB3cm9uZy4KCj4gPiBZb3Ugc2VlbSB0byBiZSB1c2luZyAyIHJlZmVyZW5jZSBjb3Vu dHMgZm9yIHRoZSByZWNvcmQgLyByZWdpc3RyeSwgYQo+ID4gbW9kdWxlIGNvdW50LCBhbmQgYSBr cmVmIGNvdW50LCB3aGljaCBjYW4gY2F1c2UgY29uZnVzaW9uLi4uCj4gCj4gVGhlcmUgaXMgb25l IHJlZmVyZW5jZSBjb3VudCAoa3JlZiBhY3R1YWxseSkgcGVyIHJlZ2lzdHJ5IHJlY29yZCBhbmQg b25lCj4gbW9kdWxlIGNvdW50IGZvciB0aGUgcmVjb3JkIG93bmVyIGFuZCB0aGUgcmVnaXN0cnkg b3duZXIuCgpCdXQgYm90aCBjb3VudHMgYXJlIGluIHRoZSBzYW1lIHN0cnVjdHVyZSwgd2hpY2gg aXMgYSBtZXNzLgoKPiBDYW4geW91IGVsYWJvcmF0ZSB3aGF0IHlvdSB0aGluayBpcyBjb25mdXNp bmc/IFBlcmhhcHMgSSBjYW4gYWRkIG1vcmUKPiBrZXJuZWxkb2MgdG8gY2xhcmlmeS4KCllvdSBo YXZlIDIgcmVmZXJlbmNlcyBpbiB0aGUgc2FtZSBzdHJ1Y3R1cmUsIHdpdGggZGlmZmVyZW50IGxp ZmVjeWNsZXMsCmNhdXNpbmcgY29uZnVzaW9uLCBhbmQgb2RkcyBhcmUsIGJ1Z3MuLi4KClN1cmUs IGRvY3VtZW50IGl0IGJldHRlciBpZiB5b3Ugd2FudCwgYnV0IEkgdGhpbmsgc29tZXRoaW5nIG5l ZWRzIHRvIGJlCmRvbmUgZGlmZmVyZW50bHkgaWYgYXQgYWxsIHBvc3NpYmxlLgoKdGhhbmtzLAoK Z3JlZyBrLWgKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18K ZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0 dHA6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751602AbaKFCSR (ORCPT ); Wed, 5 Nov 2014 21:18:17 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:38972 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750999AbaKFCSQ (ORCPT ); Wed, 5 Nov 2014 21:18:16 -0500 Date: Wed, 5 Nov 2014 18:18:15 -0800 From: Greg Kroah-Hartman To: Thierry Reding Cc: Daniel Vetter , David Herrmann , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [RFC 1/2] core: Add generic object registry implementation Message-ID: <20141106021815.GA17253@kroah.com> References: <1415118568-18771-1-git-send-email-thierry.reding@gmail.com> <20141104163845.GA369@kroah.com> <20141105091351.GA12033@ulmo> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141105091351.GA12033@ulmo> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 05, 2014 at 10:13:53AM +0100, Thierry Reding wrote: > On Tue, Nov 04, 2014 at 08:38:45AM -0800, Greg Kroah-Hartman wrote: > > On Tue, Nov 04, 2014 at 05:29:27PM +0100, Thierry Reding wrote: > [...] > > > diff --git a/drivers/base/registry.c b/drivers/base/registry.c > [...] > > > +/** > > > + * registry_record_ref - reference on the registry record > > > + * @record: record to reference > > > + * > > > + * Increases the reference count on the record and returns a pointer to it. > > > + * > > > + * Return: A pointer to the record on success or NULL on failure. > > > + */ > > > +struct registry_record *registry_record_ref(struct registry_record *record) > > > +{ > > > + if (!record) > > > + return NULL; > > > + > > > + /* > > > + * Refuse to give out any more references if the module owning the > > > + * record is being removed. > > > + */ > > > + if (!try_module_get(record->owner)) > > > + return NULL; > > > + > > > + kref_get(&record->kref); > > > > You "protect" from a module being removed, but not from someone else > > releasing the kref at the same time. Where is the lock that prevents > > this from happening? > > You're right, we need a record lock to serialize ref/unref. > > > And do we really care about module reference counts here? > > We need this so that the code of the .release() callback stays in > memory as long as there are any references. > > Also we need the module reference count for registries because they > would typically be statically allocated and go away along with a module > when it is unloaded. Never use a 'static' variable as a dynamic one with a kref, that's just asking for trouble. > > > +/** > > > + * registry_add - add a record to a registry > > > + * @registry: registry to add the record to > > > + * @record: record to add > > > + * > > > + * Tries to increase the reference count of the module owning the registry. If > > > + * successful adds the new record to the registry. > > > + * > > > + * Return: 0 on success or a negative error code on failure. > > > + */ > > > +int registry_add(struct registry *registry, struct registry_record *record) > > > +{ > > > + if (!try_module_get(registry->owner)) > > > + return -ENODEV; > > > + > > > + mutex_lock(®istry->lock); > > > + list_add_tail(&record->list, ®istry->list); > > > + mutex_unlock(®istry->lock); > > > > No incrementing of the reference of the record at all? > > I'm not sure if we really need one. Drivers will have to remove the > record from the registry before dropping their reference. I guess we > could throw in another kref_get() here (and a kref_put() in > registry_remove()) for good measure to prevent breakage with buggy > drivers. Or at least warn people that they need to clean stuff up properly if they do not, otherwise they will get it wrong. You need to make it very hard to get wrong. > > You seem to be using 2 reference counts for the record / registry, a > > module count, and a kref count, which can cause confusion... > > There is one reference count (kref actually) per registry record and one > module count for the record owner and the registry owner. But both counts are in the same structure, which is a mess. > Can you elaborate what you think is confusing? Perhaps I can add more > kerneldoc to clarify. You have 2 references in the same structure, with different lifecycles, causing confusion, and odds are, bugs... Sure, document it better if you want, but I think something needs to be done differently if at all possible. thanks, greg k-h