[PATCH 0/9] staging: unisys: visorinput fixes and enhancements

[PATCH 0/9] staging: unisys: visorinput fixes and enhancements

[Benjamin Romer]

This patch series fixes visorinput to remove the dependency on FB and
add dependency to INPUT, cleans up some formatting issues found with
checkpatch.pl, and adds the capability to change screen resolutions
without breaking mouse functionality.

Tim Sell (9):
  staging: unisys: visorinput: address checkpatch alignment issues
  staging: unisys: visorinput: fix comment format
  staging: unisys: visorinput: use kref ref-counting for device data
    struct
  staging: unisys: visorinput: remove need for 'depends on FB'
  staging: unisys: visorinput: ensure proper locking wrt creation & ints
  staging: unisys: visorinput: respond to resolution changes on-the-fly
  staging: unisys: visorinput: sanity check resolution changes
  staging: unisys: visorinput: add useful dev_dbg() and dev_info()
    messages
  staging: unisys: visorinput: add INPUT to dependent driver list

 drivers/staging/unisys/visorinput/Kconfig      |   2 +-
 drivers/staging/unisys/visorinput/visorinput.c | 216 +++++++++++++++++++++----
 2 files changed, 187 insertions(+), 31 deletions(-)

-- 
2.1.4

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

[PATCH 1/9] staging: unisys: visorinput: address checkpatch alignment issues

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Fix the alignment of function parameters to the parenthesis above.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 546e6be..81bb68a 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -620,7 +620,7 @@ out_locked:
 
 static int
 visorinput_pause(struct visor_device *dev,
-	       visorbus_state_complete_func complete_func)
+		 visorbus_state_complete_func complete_func)
 {
 	int rc;
 	struct visorinput_devdata *devdata = dev_get_drvdata(&dev->device);
@@ -646,7 +646,7 @@ out:
 
 static int
 visorinput_resume(struct visor_device *dev,
-		visorbus_state_complete_func complete_func)
+		  visorbus_state_complete_func complete_func)
 {
 	int rc;
 	struct visorinput_devdata *devdata = dev_get_drvdata(&dev->device);
-- 
2.1.4

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

[PATCH 2/9] staging: unisys: visorinput: fix comment format

[Benjamin Romer]

From: Tim Sell <timothy.sell@unisys.com>

Fix the multi-line comment formatting in visorinput.c.

Signed-off-by: Tim Sell <timothy.sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 81bb68a..d23c129 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -167,7 +167,8 @@ static const unsigned char visorkbd_keycode[KEYCODE_TABLE_BYTES] = {
 	[82] = KEY_KP0,
 	[83] = KEY_KPDOT,
 	[86] = KEY_102ND, /* enables UK backslash+pipe key,
-			   * and FR lessthan+greaterthan key */
+			   * and FR lessthan+greaterthan key
+			   */
 	[87] = KEY_F11,
 	[88] = KEY_F12,
 	[90] = KEY_KPLEFTPAREN,
-- 
2.1.4

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

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

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@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 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);
+}
+
 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(&devdata->lock_visor_dev);
+	kref_init(&devdata->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(&dev->device);
@@ -438,9 +462,8 @@ visorinput_remove(struct visor_device *dev)
 
 	down_write(&devdata->lock_visor_dev);
 	dev_set_drvdata(&dev->device, NULL);
-	unregister_client_input(devdata->visorinput_dev);
 	up_write(&devdata->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(&dev->device);
+	struct visorinput_devdata *devdata =
+		devdata_get(dev_get_drvdata(&dev->device));
 
 	if (!devdata)
 		return;
@@ -616,6 +640,7 @@ visorinput_channel_interrupt(struct visor_device *dev)
 		}
 	}
 out_locked:
+	devdata_put(devdata);
 	up_write(&devdata->lock_visor_dev);
 }
 
-- 
2.1.4

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

[PATCH 4/9] staging: unisys: visorinput: remove need for 'depends on FB'

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Previously, we used a hack to determine the max x,y resolution of the
visor virtual mouse: we just looked at the resolution of the
first-registered framebuffer device, using the currently-valid assumption
that in a Unisys s-Par guest environment the video will be provided by an
efifb framebuffer device.

This hack has been removed, by instead determining the default mouse
resolution by looking at fields within the visor mouse channel memory,
mouse.x_resolution and mouse.y_resolution.  If these fields are 0, a
default resolution of 1024x768 is assumed.  Note that in current
implementations, mouse.x_resolution and mouse.y_resolution are always just
initialized to 0 by the back-end, and only 1024x768 is supported, but
coding it this way will allow other resolutions to work in the future.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/Kconfig      |  2 +-
 drivers/staging/unisys/visorinput/visorinput.c | 63 +++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/unisys/visorinput/Kconfig b/drivers/staging/unisys/visorinput/Kconfig
index d83deb4..1c5a072 100644
--- a/drivers/staging/unisys/visorinput/Kconfig
+++ b/drivers/staging/unisys/visorinput/Kconfig
@@ -4,7 +4,7 @@
 
 config UNISYS_VISORINPUT
 	tristate "Unisys visorinput driver"
-	depends on UNISYSSPAR && UNISYS_VISORBUS && FB
+	depends on UNISYSSPAR && UNISYS_VISORBUS
 	---help---
 	If you say Y here, you will enable the Unisys visorinput driver.
 
diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 59641d7..9fa8071 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -47,8 +47,38 @@
 #define SPAR_MOUSE_CHANNEL_PROTOCOL_UUID_STR \
 	"addf07d4-94a9-46e2-81c3-61abcdbdbd87"
 
-#define PIXELS_ACROSS_DEFAULT	800
-#define PIXELS_DOWN_DEFAULT	600
+/* header of keyboard/mouse channels */
+struct spar_input_channel_protocol {
+	struct channel_header channel_header;
+	u32 n_input_reports;
+	union {
+		struct {
+			u16 x_resolution;
+			u16 y_resolution;
+		} mouse;
+		struct {
+			u32 flags;
+		} keyboard;
+	};
+} __packed;
+
+#define sizeofmemb(TYPE, MEMBER) sizeof(((TYPE *)0)->MEMBER)
+
+static unsigned read_input_channel_uint(struct visor_device *dev,
+					unsigned offset, unsigned size)
+{
+	unsigned v = 0;
+
+	if (visorbus_read_channel(dev, offset, &v, size)) {
+		dev_warn(&dev->device,
+			 "failed to read channel int at offset %u\n", offset);
+		return 0;
+	}
+	return v;
+}
+
+#define PIXELS_ACROSS_DEFAULT	1024
+#define PIXELS_DOWN_DEFAULT	768
 #define KEYCODE_TABLE_BYTES	256
 
 enum visorinput_device_type {
@@ -298,12 +328,11 @@ register_client_keyboard(void *devdata,  /* opaque on purpose */
 }
 
 static struct input_dev *
-register_client_mouse(void *devdata /* opaque on purpose */)
+register_client_mouse(void *devdata /* opaque on purpose */,
+		      unsigned xres, unsigned yres)
 {
 	int error;
 	struct input_dev *visorinput_dev = NULL;
-	int xres, yres;
-	struct fb_info *fb0;
 
 	visorinput_dev = input_allocate_device();
 	if (!visorinput_dev)
@@ -321,14 +350,10 @@ register_client_mouse(void *devdata /* opaque on purpose */)
 	set_bit(BTN_RIGHT, visorinput_dev->keybit);
 	set_bit(BTN_MIDDLE, visorinput_dev->keybit);
 
-	if (registered_fb[0]) {
-		fb0 = registered_fb[0];
-		xres = fb0->var.xres_virtual;
-		yres = fb0->var.yres_virtual;
-	} else {
+	if (xres == 0)
 		xres = PIXELS_ACROSS_DEFAULT;
+	if (yres == 0)
 		yres = PIXELS_DOWN_DEFAULT;
-	}
 	input_set_abs_params(visorinput_dev, ABS_X, 0, xres, 0, 0);
 	input_set_abs_params(visorinput_dev, ABS_Y, 0, yres, 0, 0);
 
@@ -381,6 +406,7 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 {
 	struct visorinput_devdata *devdata = NULL;
 	unsigned int extra_bytes = 0;
+	unsigned xres, yres;
 
 	if (devtype == visorinput_keyboard)
 		/* allocate room for devdata->keycode_table, filled in below */
@@ -408,7 +434,20 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 			goto cleanups_register;
 		break;
 	case visorinput_mouse:
-		devdata->visorinput_dev = register_client_mouse(devdata);
+		xres = read_input_channel_uint
+			(dev,
+			 offsetof(struct spar_input_channel_protocol,
+				  mouse.x_resolution),
+			 sizeofmemb(struct spar_input_channel_protocol,
+				    mouse.x_resolution));
+		yres = read_input_channel_uint
+			(dev,
+			 offsetof(struct spar_input_channel_protocol,
+				  mouse.y_resolution),
+			 sizeofmemb(struct spar_input_channel_protocol,
+				    mouse.y_resolution));
+		devdata->visorinput_dev =
+			register_client_mouse(devdata, xres, yres);
 		if (!devdata->visorinput_dev)
 			goto cleanups_register;
 		break;
-- 
2.1.4

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

[PATCH 5/9] staging: unisys: visorinput: ensure proper locking wrt creation & ints

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Ensure we properly lock between visorinput_channel_interrupt() and
devdata_create().  We now guarantee that interrupts will be disabled and
we will be holding lock_visor_dev at the time input_register_device() is
called (from register_client_keyboard() or register_client_mouse()), after
which we may quickly find ourselves in visorinput_open(), where we enable
interrupts, and hence may call visorinput_channel_interrupt().

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 9fa8071..3dc1e78 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -415,6 +415,8 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 	if (!devdata)
 		return NULL;
 	devdata->dev = dev;
+	init_rwsem(&devdata->lock_visor_dev);
+	down_write(&devdata->lock_visor_dev);
 
 	/*
 	 * This is an input device in a client guest partition,
@@ -453,12 +455,14 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 		break;
 	}
 
-	init_rwsem(&devdata->lock_visor_dev);
 	kref_init(&devdata->kref);
+	dev_set_drvdata(&dev->device, devdata);
+	up_write(&devdata->lock_visor_dev);
 
 	return devdata;
 
 cleanups_register:
+	up_write(&devdata->lock_visor_dev);
 	kfree(devdata);
 	return NULL;
 }
@@ -466,7 +470,6 @@ cleanups_register:
 static int
 visorinput_probe(struct visor_device *dev)
 {
-	struct visorinput_devdata *devdata = NULL;
 	uuid_le guid;
 	enum visorinput_device_type devtype;
 
@@ -477,10 +480,9 @@ visorinput_probe(struct visor_device *dev)
 		devtype = visorinput_keyboard;
 	else
 		return -ENODEV;
-	devdata = devdata_create(dev, devtype);
-	if (!devdata)
+	visorbus_disable_channel_interrupts(dev);
+	if (!devdata_create(dev, devtype))
 		return -ENOMEM;
-	dev_set_drvdata(&dev->device, devdata);
 	return 0;
 }
 
-- 
2.1.4

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

[PATCH 6/9] staging: unisys: visorinput: respond to resolution changes on-the-fly

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Whenever the absolute x,y resolution of the video (and mouse) changes, the
back-end sends an inputaction_set_max_xy message indicating the new
resolution.  This commit adds the infrastructure to detect and correctly
respond to that message.

There were several reasons this wasn't as easy as it sounds:

* input_set_abs_params() is only effective if it is called prior to
  input_register_device().  So we need to free the input_dev and
  re-create it whenever the resolution changes.

* Because freeing the input_dev and re-creating it will take us thru
  visorinput_close() and visorinput_open() if someone in user-land has the
  device open, and because those will end up calling
  visorbus_enable_channel_interrupts() and
  visorbus_disable_channel_interrupts(), we canNOT just free the input_dev
  and re-create it inline within visorinput_channel_interrupt().  We need
  to use a workqueue to do it asynchronously.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 71 ++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 3dc1e78..cbca2dc 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -28,6 +28,7 @@
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/uuid.h>
+#include <linux/workqueue.h>
 
 #include "version.h"
 #include "visorbus.h"
@@ -86,6 +87,11 @@ enum visorinput_device_type {
 	visorinput_mouse,
 };
 
+struct change_resolution_work {
+	struct work_struct work;
+	unsigned xres, yres;
+};
+
 /*
  * This is the private data that we store for each device.
  * A pointer to this struct is maintained via
@@ -97,6 +103,8 @@ struct visorinput_devdata {
 	struct rw_semaphore lock_visor_dev; /* lock for dev */
 	struct input_dev *visorinput_dev;
 	bool paused;
+	struct workqueue_struct *wq;
+	struct change_resolution_work change_resolution_work_data;
 	unsigned int keycode_table_bytes; /* size of following array */
 	/* for keyboard devices: visorkbd_keycode[] + visorkbd_ext_keycode[] */
 	unsigned char keycode_table[0];
@@ -384,6 +392,10 @@ static void devdata_release(struct kref *kref)
 	struct visorinput_devdata *devdata =
 		container_of(kref, struct visorinput_devdata, kref);
 	unregister_client_input(devdata->visorinput_dev);
+	if (devdata->wq) {
+		flush_workqueue(devdata->wq);
+		destroy_workqueue(devdata->wq);
+	}
 	kfree(devdata);
 }
 
@@ -401,6 +413,51 @@ static void devdata_put(struct visorinput_devdata *devdata)
 		kref_put(&devdata->kref, devdata_release);
 }
 
+static void async_change_resolution(struct work_struct *work)
+{
+	struct change_resolution_work *p_change_resolution_work =
+		container_of(work, struct change_resolution_work, work);
+	struct visorinput_devdata *devdata =
+		container_of(p_change_resolution_work,
+			     struct visorinput_devdata,
+			     change_resolution_work_data);
+
+	down_write(&devdata->lock_visor_dev);
+
+	if (devdata->paused) /* don't touch device/channel when paused */
+		goto out_locked;
+	if (!devdata->visorinput_dev)
+		goto out_locked;
+
+	unregister_client_input(devdata->visorinput_dev);
+	/*
+	 * input_set_abs_params is only effective prior to
+	 * input_register_device().
+	 */
+	devdata->visorinput_dev =
+		register_client_mouse(devdata,
+				      p_change_resolution_work->xres,
+				      p_change_resolution_work->yres);
+	if (!devdata->visorinput_dev)
+		dev_err(&devdata->dev->device,
+			"failed create of new mouse input dev for new resolution %u,%u\n",
+			p_change_resolution_work->xres,
+			p_change_resolution_work->yres);
+
+out_locked:
+	up_write(&devdata->lock_visor_dev);
+	devdata_put(devdata);  /* from schedule_mouse_resolution_change() */
+}
+
+static void schedule_mouse_resolution_change(struct visorinput_devdata *devdata,
+					     unsigned xres, unsigned yres)
+{
+	devdata->change_resolution_work_data.xres = xres;
+	devdata->change_resolution_work_data.yres = yres;
+	devdata_get(devdata);  /* don't go away until work processed */
+	queue_work(devdata->wq, &devdata->change_resolution_work_data.work);
+}
+
 static struct visorinput_devdata *
 devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 {
@@ -415,6 +472,9 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 	if (!devdata)
 		return NULL;
 	devdata->dev = dev;
+	devdata->wq = alloc_ordered_workqueue("visorinput", 0);
+	INIT_WORK(&devdata->change_resolution_work_data.work,
+		  async_change_resolution);
 	init_rwsem(&devdata->lock_visor_dev);
 	down_write(&devdata->lock_visor_dev);
 
@@ -678,6 +738,17 @@ visorinput_channel_interrupt(struct visor_device *dev)
 			input_report_rel(visorinput_dev, REL_WHEEL, -1);
 			input_sync(visorinput_dev);
 			break;
+		case inputaction_set_max_xy:
+			/*
+			 * we can NOT handle this inline, because this may go
+			 * thru a close() path, which will attempt to stop the
+			 * worker thread we are running on, which will end up
+			 * deadlocking
+			 */
+			schedule_mouse_resolution_change(devdata,
+							 r.activity.arg1,
+							 r.activity.arg2);
+			break;
 		}
 	}
 out_locked:
-- 
2.1.4

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

[PATCH 7/9] staging: unisys: visorinput: sanity check resolution changes

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

This commit sanity checks so that if a change resolution request is ever
received for a non-mouse device, that an error message will be logged and
the message will be ignored.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index cbca2dc..40b32b7 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -100,6 +100,7 @@ struct change_resolution_work {
 struct visorinput_devdata {
 	struct kref kref;
 	struct visor_device *dev;
+	enum visorinput_device_type devtype;
 	struct rw_semaphore lock_visor_dev; /* lock for dev */
 	struct input_dev *visorinput_dev;
 	bool paused;
@@ -472,6 +473,7 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 	if (!devdata)
 		return NULL;
 	devdata->dev = dev;
+	devdata->devtype = devtype;
 	devdata->wq = alloc_ordered_workqueue("visorinput", 0);
 	INIT_WORK(&devdata->change_resolution_work_data.work,
 		  async_change_resolution);
@@ -739,6 +741,11 @@ visorinput_channel_interrupt(struct visor_device *dev)
 			input_sync(visorinput_dev);
 			break;
 		case inputaction_set_max_xy:
+			if (devdata->devtype != visorinput_mouse) {
+				dev_err(&dev->device,
+					"mouse resolution change for NON-mouse device!\n");
+				continue;
+			}
 			/*
 			 * we can NOT handle this inline, because this may go
 			 * thru a close() path, which will attempt to stop the
-- 
2.1.4

[PATCH 8/9] staging: unisys: visorinput: add useful dev_dbg() and dev_info() messages

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

The dev_info() messages at init time are particularly useful for mapping
visor devices to input devices.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 40b32b7..d30a988 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -392,6 +392,7 @@ static void devdata_release(struct kref *kref)
 {
 	struct visorinput_devdata *devdata =
 		container_of(kref, struct visorinput_devdata, kref);
+	dev_dbg(&devdata->dev->device, "releasing resources\n");
 	unregister_client_input(devdata->visorinput_dev);
 	if (devdata->wq) {
 		flush_workqueue(devdata->wq);
@@ -444,6 +445,8 @@ static void async_change_resolution(struct work_struct *work)
 			"failed create of new mouse input dev for new resolution %u,%u\n",
 			p_change_resolution_work->xres,
 			p_change_resolution_work->yres);
+	dev_info(&devdata->dev->device, "created mouse %s\n",
+		 dev_name(&devdata->visorinput_dev->dev));
 
 out_locked:
 	up_write(&devdata->lock_visor_dev);
@@ -496,6 +499,8 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 			(devdata, devdata->keycode_table);
 		if (!devdata->visorinput_dev)
 			goto cleanups_register;
+		dev_info(&devdata->dev->device, "created keyboard %s\n",
+			 dev_name(&devdata->visorinput_dev->dev));
 		break;
 	case visorinput_mouse:
 		xres = read_input_channel_uint
@@ -514,6 +519,8 @@ devdata_create(struct visor_device *dev, enum visorinput_device_type devtype)
 			register_client_mouse(devdata, xres, yres);
 		if (!devdata->visorinput_dev)
 			goto cleanups_register;
+		dev_info(&devdata->dev->device, "created mouse %s\n",
+			 dev_name(&devdata->visorinput_dev->dev));
 		break;
 	}
 
@@ -746,6 +753,10 @@ visorinput_channel_interrupt(struct visor_device *dev)
 					"mouse resolution change for NON-mouse device!\n");
 				continue;
 			}
+			dev_info(&dev->device,
+				 "mouse resolution change to %ux%u\n",
+				 r.activity.arg1,
+				 r.activity.arg2);
 			/*
 			 * we can NOT handle this inline, because this may go
 			 * thru a close() path, which will attempt to stop the
-- 
2.1.4

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

[PATCH 9/9] staging: unisys: visorinput: add INPUT to dependent driver list

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

This was an obvious omission, as visorinput is an input-class driver.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorinput/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorinput/Kconfig b/drivers/staging/unisys/visorinput/Kconfig
index 1c5a072..d70da14 100644
--- a/drivers/staging/unisys/visorinput/Kconfig
+++ b/drivers/staging/unisys/visorinput/Kconfig
@@ -4,7 +4,7 @@
 
 config UNISYS_VISORINPUT
 	tristate "Unisys visorinput driver"
-	depends on UNISYSSPAR && UNISYS_VISORBUS
+	depends on UNISYSSPAR && UNISYS_VISORBUS && INPUT
 	---help---
 	If you say Y here, you will enable the Unisys visorinput driver.
 
-- 
2.1.4

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

[Dan Carpenter]

On Fri, Oct 16, 2015 at 10:06:48AM -0400, Benjamin Romer wrote:
> @@ -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(&dev->device);
> +	struct visorinput_devdata *devdata =
> +		devdata_get(dev_get_drvdata(&dev->device));
>  
>  	if (!devdata)
>  		return;
> @@ -616,6 +640,7 @@ visorinput_channel_interrupt(struct visor_device *dev)
>  		}
>  	}
>  out_locked:
> +	devdata_put(devdata);
>  	up_write(&devdata->lock_visor_dev);
>  }

Always release resources in the reverse order from how they were taken.
So we call devdata_put() after up_write().  Otherwise it looks like a
use after free or something.

regards,
dan carpenter

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

[Sell, Timothy C]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, October 16, 2015 9:03 PM
> To: Romer, Benjamin M
> Cc: gregkh@linuxfoundation.org; *S-Par-Maintainer; driverdev-
> devel@linuxdriverproject.org; 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:
> > @@ -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(&dev-
> >device);
> > +	struct visorinput_devdata *devdata =
> > +		devdata_get(dev_get_drvdata(&dev->device));
> >
> >  	if (!devdata)
> >  		return;
> > @@ -616,6 +640,7 @@ visorinput_channel_interrupt(struct visor_device
> *dev)
> >  		}
> >  	}
> >  out_locked:
> > +	devdata_put(devdata);
> >  	up_write(&devdata->lock_visor_dev);
> >  }
> 
> Always release resources in the reverse order from how they were taken.
> So we call devdata_put() after up_write().  Otherwise it looks like a
> use after free or something.
> 
> regards,
> dan carpenter

Good point; thanks.  I generally follow that rule, but somehow missed it this
time.  We'll re-submit this patch with those lines reversed, as you suggest.

Tim Sell

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

[Greg KH]

On Fri, Oct 16, 2015 at 10:06:48AM -0400, Benjamin Romer wrote:
> From: Tim Sell <Timothy.Sell@unisys.com>
> 
> 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@unisys.com>
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> ---
>  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

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

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Saturday, October 17, 2015 1:59 AM
> To: Romer, Benjamin M
> Cc: *S-Par-Maintainer; driverdev-devel@linuxdriverproject.org; 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@unisys.com>
> >
> > 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@unisys.com>
> > Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> > ---
> >  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@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Greg KH]

On Sat, Oct 17, 2015 at 03:04:37PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Saturday, October 17, 2015 1:59 AM
> > To: Romer, Benjamin M
> > Cc: *S-Par-Maintainer; driverdev-devel@linuxdriverproject.org; 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@unisys.com>
> > >
> > > 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@unisys.com>
> > > Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> > > ---
> > >  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

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

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Saturday, October 17, 2015 11:42 AM
> To: Sell, Timothy C
> Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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@linuxfoundation.org]
> > > Sent: Saturday, October 17, 2015 1:59 AM
> > > To: Romer, Benjamin M
> > > Cc: *S-Par-Maintainer; driverdev-devel@linuxdriverproject.org; 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@unisys.com>
> > > >
> > > > 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@unisys.com>
> > > > Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> > > > ---
> > > >  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@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Greg KH]

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

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

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Saturday, October 17, 2015 12:51 PM
> To: Sell, Timothy C
> Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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 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

AGREED!

Are you also suggesting that I should use one of the kref locking
variants here?   I can do that NO problem, as I'm sure it can't hurt
anything, but as I explained above, I just don't see why I need it.
(I'm NOT above admitting my "why I don't need a lock" logic could be
flawed...)  

Tim Sell

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

[Greg KH]

On Sat, Oct 17, 2015 at 05:04:43PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Saturday, October 17, 2015 12:51 PM
> > To: Sell, Timothy C
> > Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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 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
> 
> AGREED!
> 
> Are you also suggesting that I should use one of the kref locking
> variants here?   I can do that NO problem, as I'm sure it can't hurt
> anything, but as I explained above, I just don't see why I need it.
> (I'm NOT above admitting my "why I don't need a lock" logic could be
> flawed...)  

If your reference counting model follows another reference counted
object, they why need/use a kref at all?  I don't see why you are even
using a kref, you haven't justified that yet.

thanks,

greg k-h

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

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Saturday, October 17, 2015 1:41 PM
> To: Sell, Timothy C
> Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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 05:04:43PM +0000, Sell, Timothy C wrote:
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Saturday, October 17, 2015 12:51 PM
> > > To: Sell, Timothy C
> > > Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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 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
> >
> > AGREED!
> >
> > Are you also suggesting that I should use one of the kref locking
> > variants here?   I can do that NO problem, as I'm sure it can't hurt
> > anything, but as I explained above, I just don't see why I need it.
> > (I'm NOT above admitting my "why I don't need a lock" logic could be
> > flawed...)
> 
> If your reference counting model follows another reference counted
> object, they why need/use a kref at all?  I don't see why you are even
> using a kref, you haven't justified that yet.
> 
> thanks,
> 
> greg k-h

The kref isn't functionally required for proper behavior until
"[PATCH 6/9] staging: unisys: visorinput: respond to resolution changes
on-the-fly", because that patch adds a reference to the kref-counted data
from a work queue.  That absolutely requires a kref_get() before
adding a pointer to the data for access by the work queue, and a
corresponding kref_put() after the work is processed.

Tim Sell

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

[Sell, Timothy C]

What should I do with this patchset?
 
Some history: it was fixing this one little line in
drivers/staging/unisys/visorinput/Kconfig that triggered this patchset:

	depends on UNISYSSPAR && UNISYS_VISORBUS && FB

Adding "INPUT" was easy, but it turned out that removing "FB" was hard.  ;-(
Removing FB is at the crux of this patchset.

Rarely a week passes where I don't get a complaint from the
community about the fact that I need to add "INPUT" to that line to fix
errors when building with randconfig.

Thanks.

Tim Sell

> -----Original Message-----
> From: Sell, Timothy C
> Sent: Saturday, October 17, 2015 2:10 PM
> To: 'Greg KH'
> Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer
> Subject: RE: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting for
> device data struct
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Saturday, October 17, 2015 1:41 PM
> > To: Sell, Timothy C
> > Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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 05:04:43PM +0000, Sell, Timothy C wrote:
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Saturday, October 17, 2015 12:51 PM
> > > > To: Sell, Timothy C
> > > > Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *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 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
> > >
> > > AGREED!
> > >
> > > Are you also suggesting that I should use one of the kref locking
> > > variants here?   I can do that NO problem, as I'm sure it can't hurt
> > > anything, but as I explained above, I just don't see why I need it.
> > > (I'm NOT above admitting my "why I don't need a lock" logic could be
> > > flawed...)
> >
> > If your reference counting model follows another reference counted
> > object, they why need/use a kref at all?  I don't see why you are even
> > using a kref, you haven't justified that yet.
> >
> > thanks,
> >
> > greg k-h
> 
> The kref isn't functionally required for proper behavior until
> "[PATCH 6/9] staging: unisys: visorinput: respond to resolution changes
> on-the-fly", because that patch adds a reference to the kref-counted data
> from a work queue.  That absolutely requires a kref_get() before
> adding a pointer to the data for access by the work queue, and a
> corresponding kref_put() after the work is processed.
> 
> Tim Sell

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

[Greg KH]

On Thu, Nov 12, 2015 at 09:07:24PM +0000, Sell, Timothy C wrote:
> What should I do with this patchset?

What patchset?

> Some history: it was fixing this one little line in
> drivers/staging/unisys/visorinput/Kconfig that triggered this patchset:
> 
> 	depends on UNISYSSPAR && UNISYS_VISORBUS && FB
> 
> Adding "INPUT" was easy, but it turned out that removing "FB" was hard.  ;-(
> Removing FB is at the crux of this patchset.
> 
> Rarely a week passes where I don't get a complaint from the
> community about the fact that I need to add "INPUT" to that line to fix
> errors when building with randconfig.

Then submit a patch that fixes that! :)

thanks,

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

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

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, November 12, 2015 4:12 PM
> To: Sell, Timothy C
> Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer
> Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting
> for device data struct
> 
> On Thu, Nov 12, 2015 at 09:07:24PM +0000, Sell, Timothy C wrote:
> > What should I do with this patchset?
> 
> What patchset?

"[PATCH 0/9] staging: unisys: visorinput fixes and enhancements", 
submitted by Ben Romer Fri 10/16/2015 @ 10:07 AM.

> > Some history: it was fixing this one little line in
> > drivers/staging/unisys/visorinput/Kconfig that triggered this patchset:
> >
> > 	depends on UNISYSSPAR && UNISYS_VISORBUS && FB
> >
> > Adding "INPUT" was easy, but it turned out that removing "FB" was hard.
> ;-(
> > Removing FB is at the crux of this patchset.
> >
> > Rarely a week passes where I don't get a complaint from the
> > community about the fact that I need to add "INPUT" to that line to fix
> > errors when building with randconfig.
> 
> Then submit a patch that fixes that! :)

That's what I did, originally.  But you told me I had to get rid of the FB
requirement.  So  that's why we submitted 
"[PATCH 0/9] staging: unisys: visorinput fixes and enhancements", which
includes all the stuff we needed to get rid of FB as well as the last
patch in the series, which adds INPUT.
Ben Romer actually submitted the patchset Fri 10/16/2015 @ 10:07 AM.

Tim Sell

> 
> thanks,
> 
> greg k-h

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

[Greg KH]

On Thu, Nov 12, 2015 at 09:21:14PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Thursday, November 12, 2015 4:12 PM
> > To: Sell, Timothy C
> > Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *S-Par-
> > Maintainer
> > Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting
> > for device data struct
> > 
> > On Thu, Nov 12, 2015 at 09:07:24PM +0000, Sell, Timothy C wrote:
> > > What should I do with this patchset?
> > 
> > What patchset?
> 
> "[PATCH 0/9] staging: unisys: visorinput fixes and enhancements", 
> submitted by Ben Romer Fri 10/16/2015 @ 10:07 AM.

I don't see this in my "to-review" queue at all, sorry.  If this was
some old patchset, you are going to have to resend as my memory is
totally gone (remember, I get 1000 emails a day, and my to-review mbox
is right now 1637 emails to process once the merge window opens up...)

> > > Some history: it was fixing this one little line in
> > > drivers/staging/unisys/visorinput/Kconfig that triggered this patchset:
> > >
> > > 	depends on UNISYSSPAR && UNISYS_VISORBUS && FB
> > >
> > > Adding "INPUT" was easy, but it turned out that removing "FB" was hard.
> > ;-(
> > > Removing FB is at the crux of this patchset.
> > >
> > > Rarely a week passes where I don't get a complaint from the
> > > community about the fact that I need to add "INPUT" to that line to fix
> > > errors when building with randconfig.
> > 
> > Then submit a patch that fixes that! :)
> 
> That's what I did, originally.  But you told me I had to get rid of the FB
> requirement.  So  that's why we submitted 
> "[PATCH 0/9] staging: unisys: visorinput fixes and enhancements", which
> includes all the stuff we needed to get rid of FB as well as the last
> patch in the series, which adds INPUT.
> Ben Romer actually submitted the patchset Fri 10/16/2015 @ 10:07 AM.

It's not in my queue, so I have no idea what happened to it.  Did I
apply it?  Reject it?  something else?

thanks,

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

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

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, November 12, 2015 4:28 PM
> To: Sell, Timothy C
> Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer
> Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting
> for device data struct
> 
> On Thu, Nov 12, 2015 at 09:21:14PM +0000, Sell, Timothy C wrote:
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Thursday, November 12, 2015 4:12 PM
> > > To: Sell, Timothy C
> > > Cc: Romer, Benjamin M; driverdev-devel@linuxdriverproject.org; *S-Par-
> > > Maintainer
> > > Subject: Re: [PATCH 3/9] staging: unisys: visorinput: use kref ref-counting
> > > for device data struct
> > >
> > > On Thu, Nov 12, 2015 at 09:07:24PM +0000, Sell, Timothy C wrote:
> > > > What should I do with this patchset?
> > >
> > > What patchset?
> >
> > "[PATCH 0/9] staging: unisys: visorinput fixes and enhancements",
> > submitted by Ben Romer Fri 10/16/2015 @ 10:07 AM.
> 
> I don't see this in my "to-review" queue at all, sorry.  If this was
> some old patchset, you are going to have to resend as my memory is
> totally gone (remember, I get 1000 emails a day, and my to-review mbox
> is right now 1637 emails to process once the merge window opens up...)
> 
> > > > Some history: it was fixing this one little line in
> > > > drivers/staging/unisys/visorinput/Kconfig that triggered this patchset:
> > > >
> > > > 	depends on UNISYSSPAR && UNISYS_VISORBUS && FB
> > > >
> > > > Adding "INPUT" was easy, but it turned out that removing "FB" was
> hard.
> > > ;-(
> > > > Removing FB is at the crux of this patchset.
> > > >
> > > > Rarely a week passes where I don't get a complaint from the
> > > > community about the fact that I need to add "INPUT" to that line to fix
> > > > errors when building with randconfig.
> > >
> > > Then submit a patch that fixes that! :)
> >
> > That's what I did, originally.  But you told me I had to get rid of the FB
> > requirement.  So  that's why we submitted
> > "[PATCH 0/9] staging: unisys: visorinput fixes and enhancements", which
> > includes all the stuff we needed to get rid of FB as well as the last
> > patch in the series, which adds INPUT.
> > Ben Romer actually submitted the patchset Fri 10/16/2015 @ 10:07 AM.
> 
> It's not in my queue, so I have no idea what happened to it.  Did I
> apply it?  Reject it?  something else?
> 
> thanks,
> 
> greg k-h

We were bantering back-and-forth about a lock, but the thread stopped
without a conclusion.  You definitely did NOT apply it.

No problem.  We'll just re-submit the set.

Thanks.

Tim Sell