What should I do with this patchset? Some history: it was fixing this one little line in drivers/staging/unisys/visorinput/Kconfig that triggered this patchset: depends on UNISYSSPAR && UNISYS_VISORBUS && FB Adding "INPUT" was easy, but it turned out that removing "FB" was hard. ;-( Removing FB is at the crux of this patchset. Rarely a week passes where I don't get a complaint from the community about the fact that I need to add "INPUT" to that line to fix errors when building with randconfig. Thanks. Tim Sell > -----Original Message----- > From: Sell, Timothy C > Sent: Saturday, October 17, 2015 2:10 PM > To: 'Greg KH' > 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 > > > -----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
