Re: [PATCH v2 1/7] staging: unisys: visorinput: use kref ref-counting for device data struct

[Greg KH]

On Mon, Nov 16, 2015 at 03:22:11PM -0500, Benjamin Romer wrote:
> From: Tim Sell 
> 
> This is NOT technically required for the code as it stands now, but will
> be needed for subsequent patches.
> 
> Signed-off-by: Tim Sell 
> Signed-off-by: Benjamin Romer 
> 
> ---
> v2: resources are released in the reverse order they were acquired, as per
> Dan Carpenter's comment.
> ---
>  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 5c16f66..238a132 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(>kref);
> + return devdata;
> +}
> +
> +static void devdata_put(struct visorinput_devdata *devdata)
> +{
> + if (devdata)
> + kref_put(>kref, devdata_release);

Where is the lock that protects this from racing?  Please either
document the heck out of this, or use the proper api calls (i.e. the
locked version of the kref release function.)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/7] staging: unisys: visorinput: use kref ref-counting for device data struct

[Benjamin Romer]

From: Tim Sell 

This is NOT technically required for the code as it stands now, but will
be needed for subsequent patches.

Signed-off-by: Tim Sell 
Signed-off-by: Benjamin Romer 

---
v2: resources are released in the reverse order they were acquired, as per
Dan Carpenter's comment.
---
 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 5c16f66..238a132 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(>kref);
+   return devdata;
+}
+
+static void devdata_put(struct visorinput_devdata *devdata)
+{
+   if (devdata)
+   kref_put(>kref, devdata_release);
+}
+
 static struct visorinput_devdata *
 devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 {
@@ -385,6 +415,7 @@ devdata_create(struct visor_device *dev, enum 
visorinput_device_type devtype)
}
 
init_rwsem(>lock_visor_dev);
+   kref_init(>kref);
 
return devdata;
 
@@ -415,13 +446,6 @@ visorinput_probe(struct visor_device *dev)
 }
 
 static void
-unregister_client_input(struct input_dev *visorinput_dev)
-{
-   if (visorinput_dev)
-   input_unregister_device(visorinput_dev);
-}
-
-static void
 visorinput_remove(struct visor_device *dev)
 {
struct visorinput_devdata *devdata = dev_get_drvdata(>device);
@@ -438,9 +462,8 @@ visorinput_remove(struct visor_device *dev)
 
down_write(>lock_visor_dev);
dev_set_drvdata(>device, NULL);
-   unregister_client_input(devdata->visorinput_dev);
up_write(>lock_visor_dev);
-   kfree(devdata);
+   devdata_put(devdata);
 }
 
 /*
@@ -526,7 +549,8 @@ visorinput_channel_interrupt(struct visor_device *dev)
int xmotion, ymotion, zmotion, button;
int i;
 
-   struct visorinput_devdata *devdata = dev_get_drvdata(>device);
+   struct visorinput_devdata *devdata =
+   devdata_get(dev_get_drvdata(>device));
 
if (!devdata)
return;
@@ -617,6 +641,7 @@ visorinput_channel_interrupt(struct visor_device *dev)
}
 out_locked:
up_write(>lock_visor_dev);
+   devdata_put(devdata);
 }
 
 static int
-- 
2.5.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel