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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
