> -----Original Message----- > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > Sent: Saturday, October 17, 2015 1:41 PM > To: Sell, Timothy C > Cc: Romer, Benjamin M; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par- > Maintainer > Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting > for device data struct > > On Sat, Oct 17, 2015 at 05:04:43PM +0000, Sell, Timothy C wrote: > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > > > Sent: Saturday, October 17, 2015 12:51 PM > > > To: Sell, Timothy C > > > Cc: Romer, Benjamin M; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par- > > > Maintainer > > > Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting > > > for device data struct > > > > > > On Sat, Oct 17, 2015 at 04:34:28PM +0000, Sell, Timothy C wrote: > > > > > How can you guarantee it? Please document that somehow, I don't > see > > > it > > > > > here in this patch how that can be true, but maybe I'm not looking > close > > > > > enough... > > > > > > > > Sure; I'll add some code comments. Basically they will amount to this: > > > > * The kref is initialized to 1 in _probe(), and the book-end for that is > > > > the devdata_put() in _remove(). > > > > * devdata_get() / devdata_put() is called to keep the ref valid during > > > > interrupt processing in visorinput_channel_interrupt(). The visorbus > > > > model guarantees that at most 1 thread of execution will ever be > > > > running this function at a time. > > > > * We are guaranteed that the kref will be >= 1 when entering > > > > visorinput_channel_interrupt(), and that no other thread of execution > can > > > > do a kref_put() at that point, because the devdata_put() > > > > in _remove() is only done AFTER disabling interrupts. Once > > > > interrupts are disabled, NO thread of execution will be running > > > > visorinput_channel_interrupt(). > > > > > > So the lifetime of this kref mirrors the lifetime of the struct device? > > > Why have a kref at all then? > > > > > > > * "[PATCH 6/9] staging: unisys: visorinput: respond to resolution > changes > > > on-the-fly" > > > > (a subsequent patch in this series) > > > > requires that devdata be accessed from a workqueue, but does the > > > > required devdata_get() before queuing the work (which occurs in > > > > visorinput_channel_interrupt(), where we know the kref count is > actually > > > >= 2), > > > > and devdata_put() after the work is processed. Because devdata_get() > is > > > > NEVER done at a time when it is possible that the kref is 0, we do NOT > > > > require a lock. > > > > * In actuality, there is only 1 scenario when devdata_get()/_put() could > be > > > > running on > 1 thread of execution simultaneously: The devdata_put() > > > > performed after processing of the workitem can occur asynchronously > > > with > > > > the devdata_put in _remove(). But since those both involve only > > > kref_put(), > > > > we are safe calling them on multiple threads without a lock, as only the > > > last > > > > one will be calling release(), due to atomic operation in kref_put(). > > > > > > > > I don't see a case for needing a lock, but of course it's entirely possible > > > > that I've missed something. > > > > > > > > Honestly, I was a bit surprised to see how FEW usages of > > > > kref_put_mutex() and kref_put_spinlock_irqsave() there actually were > > > > in the kernel. > > > > > > Well, most of the usages are probably wrong, let's not continue to write > > > bad code :) > > > > > > thanks, > > > > > > greg k-h > > > > AGREED! > > > > Are you also suggesting that I should use one of the kref locking > > variants here? I can do that NO problem, as I'm sure it can't hurt > > anything, but as I explained above, I just don't see why I need it. > > (I'm NOT above admitting my "why I don't need a lock" logic could be > > flawed...) > > If your reference counting model follows another reference counted > object, they why need/use a kref at all? I don't see why you are even > using a kref, you haven't justified that yet. > > thanks, > > greg k-h The kref isn't functionally required for proper behavior until "[PATCH 6/9] staging: unisys: visorinput: respond to resolution changes on-the-fly", because that patch adds a reference to the kref-counted data from a work queue. That absolutely requires a kref_get() before adding a pointer to the data for access by the work queue, and a corresponding kref_put() after the work is processed. Tim Sell _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
