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]

 



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



[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