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

Greg KH gregkh at linuxfoundation.org
Sat Oct 17 16:51:25 UTC 2015


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


More information about the devel mailing list