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