> -----Original Message----- > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > Sent: Saturday, October 17, 2015 11:42 AM > 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 03:04:37PM +0000, Sell, Timothy C wrote: > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > > > Sent: Saturday, October 17, 2015 1:59 AM > > > To: Romer, Benjamin M > > > Cc: *S-Par-Maintainer; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; Sell, > > > Timothy C > > > Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting > > > for device data struct > > > > > > On Fri, Oct 16, 2015 at 10:06:48AM -0400, Benjamin Romer wrote: > > > > From: Tim Sell <Timothy.Sell@xxxxxxxxxx> > > > > > > > > This is NOT technically required for the code as it stands now, but will > > > > be needed for subsequent patches. > > > > > > > > Signed-off-by: Tim Sell <Timothy.Sell@xxxxxxxxxx> > > > > Signed-off-by: Benjamin Romer <benjamin.romer@xxxxxxxxxx> > > > > --- > > > > drivers/staging/unisys/visorinput/visorinput.c | 45 > > > ++++++++++++++++++++------ > > > > 1 file changed, 35 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c > > > b/drivers/staging/unisys/visorinput/visorinput.c > > > > index d23c129..59641d7 100644 > > > > --- a/drivers/staging/unisys/visorinput/visorinput.c > > > > +++ b/drivers/staging/unisys/visorinput/visorinput.c > > > > @@ -62,6 +62,7 @@ enum visorinput_device_type { > > > > * dev_get_drvdata() / dev_set_drvdata() for each struct device. > > > > */ > > > > struct visorinput_devdata { > > > > + struct kref kref; > > > > struct visor_device *dev; > > > > struct rw_semaphore lock_visor_dev; /* lock for dev */ > > > > struct input_dev *visorinput_dev; > > > > @@ -346,6 +347,35 @@ register_client_mouse(void *devdata /* > opaque > > > on purpose */) > > > > return visorinput_dev; > > > > } > > > > > > > > +static void > > > > +unregister_client_input(struct input_dev *visorinput_dev) > > > > +{ > > > > + if (visorinput_dev) > > > > + input_unregister_device(visorinput_dev); > > > > +} > > > > + > > > > +static void devdata_release(struct kref *kref) > > > > +{ > > > > + struct visorinput_devdata *devdata = > > > > + container_of(kref, struct visorinput_devdata, kref); > > > > + unregister_client_input(devdata->visorinput_dev); > > > > + kfree(devdata); > > > > +} > > > > + > > > > +static struct visorinput_devdata * > > > > +devdata_get(struct visorinput_devdata *devdata) > > > > +{ > > > > + if (devdata) > > > > + kref_get(&devdata->kref); > > > > + return devdata; > > > > +} > > > > + > > > > +static void devdata_put(struct visorinput_devdata *devdata) > > > > +{ > > > > + if (devdata) > > > > + kref_put(&devdata->kref, devdata_release); > > > > > > Are you sure this is safe? Where is your lock protecting two release > > > functions from happening at the same time? > > > > > > Please use the kref-with-a-lock functions if at all possible, unless you > > > can guarantee that they are safe to call without one. > > > > > > thanks, > > > > > > greg k-h > > > > Yes, this is safe. I've looked at the kref rules in Documentation/kref.txt, > and > > can guarantee that the code never "attempts to gain a reference to a > > kref-ed structure without already holding a valid pointer". In other words, > > at the time of the kref_put() that finally decrements the kref to 0, there > are > > no other existing threads of execution that could possibly access the kref. > > 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... > > thanks, > > greg k-h 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(). * "[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. Thanks. Tim Sell _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
