RE: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting for device data struct

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux