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