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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
