[PATCH v2 00/27] Fixed issues raised by tglx, then move visorbus to drivers/virt

[PATCH v2 00/27] Fixed issues raised by tglx, then move visorbus to drivers/virt

[David Kershner]

tglx: The following patchset fixes issues you raised during your
code review of visorbus on 5/18.

Greg: Please drop all other patch series sent in from me as this
patch series incorporates the required patches from the previous
series. 

Converts visorbus to use a kernel timer for periodic device-specific
callbacks instead of a workqueue, making the implementation in
periodic_work.c and periodic_work.h no longer necessary.  These files
are then deleted.

The visordriver_callback_lock has been switched to a mutex.

Several module parameters and structures were removed that were no
longer being used.

Changes since v1: 

 - Added the patch staging: unisys: visorbus change -1 return values
 - Added the patch staging: unisys: visorchipset change -1 return value
 - Added the patch staging: unisys: iovmcall_gnuc.h change -1 return values

Bryan Thompson (4):
  staging: unisys: visorbus: Make visordriver_callback_lock a mutex
  staging: unisys: visorbus: Remove unnecessary EXPORT_SYMBOL statements
  staging: unisys: visorbus: Remove unused functions
  staging: unisys: Remove reference to unused STANDALONE_CLIENT

David Binder (12):
  staging: unisys: visorbus: remove unused module parameters
  staging: unisys: visorbus: remove unused struct
  staging: unisys: visorbus: modify format string to match argument
  staging: unisys: visornic: Correct comment spelling mistake
  staging: unisys: include: Remove thread-related enum members
  staging: unisys: visorbus: vbusdeviceinfo function descriptions more
    kerneldoc-like
  staging: unisys: visorbus: make function descriptions more
    kerneldoc-like
  staging: unisys: visorbus: make visorbus_private.h function
    descriptions more kerneldoc-like
  staging: unisys: visorbus: make visorchannel function descriptions
    more kerneldoc-like
  staging: unisys: visorbus: make visorchipset function descriptions
    more kerneldoc-like
  staging: unisys: visorbus: Move visorbus-unique functions to private
    header
  staging: unisys: visorbus: Add kerneldoc-style comments for visorbus
    API

David Kershner (4):
  staging: unisys: Move vbushelper.h to visorbus directory
  include: linux: visorbus: Add visorbus to include/linux directory
  Documentation: Move visorbus documentation from staging to
    Documentation/
  drivers: Add visorbus to the drivers directory

Erik Arfvidson (3):
  staging: unisys: visorbus change -1 return values
  staging: unisys: visorchipset change -1 return value
  staging: unisys: iovmcall_gnuc.h change -1 return values

Tim Sell (4):
  staging: unisys: visorbus: removed unused periodic_test_workqueue
  staging: unisys: visorinput: remove unnecessary locking
  staging: unisys: visorbus: use kernel timer instead of workqueue
  staging: unisys: visorbus: remove periodic_work.h/.c

 .../ABI/stable/sysfs-bus-visorbus                  |    0
 .../overview.txt => Documentation/visorbus.txt     |    0
 drivers/staging/unisys/Kconfig                     |    3 +-
 drivers/staging/unisys/MAINTAINERS                 |    2 +-
 drivers/staging/unisys/Makefile                    |    1 -
 drivers/staging/unisys/include/periodic_work.h     |   40 -
 drivers/staging/unisys/include/visorbus.h          |  234 ----
 drivers/staging/unisys/visorbus/Makefile           |   12 -
 drivers/staging/unisys/visorbus/periodic_work.c    |  204 ---
 drivers/staging/unisys/visorbus/visorbus_main.c    | 1344 --------------------
 drivers/staging/unisys/visorbus/visorbus_private.h |   68 -
 drivers/staging/unisys/visorbus/visorchannel.c     |  635 ---------
 drivers/staging/unisys/visorhba/Makefile           |    2 -
 drivers/staging/unisys/visorhba/visorhba_main.c    |    5 +-
 drivers/staging/unisys/visorinput/Makefile         |    2 -
 drivers/staging/unisys/visorinput/visorinput.c     |   63 +-
 drivers/staging/unisys/visornic/Makefile           |    2 -
 drivers/staging/unisys/visornic/visornic_main.c    |    7 +-
 drivers/virt/Kconfig                               |    2 +
 drivers/virt/Makefile                              |    1 +
 drivers/{staging/unisys => virt}/visorbus/Kconfig  |    0
 drivers/virt/visorbus/Makefile                     |    9 +
 .../unisys => virt}/visorbus/controlvmchannel.h    |    2 +-
 .../visorbus/controlvmcompletionstatus.h           |    0
 .../unisys => virt}/visorbus/iovmcall_gnuc.h       |    4 +-
 .../unisys => virt}/visorbus/vbuschannel.h         |    3 +-
 .../unisys => virt}/visorbus/vbusdeviceinfo.h      |   11 +-
 .../unisys/include => virt/visorbus}/vbushelper.h  |    0
 drivers/virt/visorbus/visorbus_main.c              | 1260 ++++++++++++++++++
 drivers/virt/visorbus/visorbus_private.h           |   96 ++
 drivers/virt/visorbus/visorchannel.c               |  459 +++++++
 .../unisys => virt}/visorbus/visorchipset.c        |   54 +-
 .../unisys => virt}/visorbus/vmcallinterface.h     |    5 +-
 .../include => include/linux/visorbus}/channel.h   |    0
 .../linux/visorbus}/channel_guid.h                 |    0
 .../linux/visorbus}/diagchannel.h                  |    0
 .../linux/visorbus}/guestlinuxdebug.h              |    4 +-
 .../include => include/linux/visorbus}/iochannel.h |    0
 .../include => include/linux/visorbus}/version.h   |    0
 include/linux/visorbus/visorbus.h                  |  328 +++++
 40 files changed, 2257 insertions(+), 2605 deletions(-)
 rename drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset => Documentation/ABI/stable/sysfs-bus-visorbus (100%)
 rename drivers/staging/unisys/Documentation/overview.txt => Documentation/visorbus.txt (100%)
 delete mode 100644 drivers/staging/unisys/include/periodic_work.h
 delete mode 100644 drivers/staging/unisys/include/visorbus.h
 delete mode 100644 drivers/staging/unisys/visorbus/Makefile
 delete mode 100644 drivers/staging/unisys/visorbus/periodic_work.c
 delete mode 100644 drivers/staging/unisys/visorbus/visorbus_main.c
 delete mode 100644 drivers/staging/unisys/visorbus/visorbus_private.h
 delete mode 100644 drivers/staging/unisys/visorbus/visorchannel.c
 rename drivers/{staging/unisys => virt}/visorbus/Kconfig (100%)
 create mode 100644 drivers/virt/visorbus/Makefile
 rename drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h (99%)
 rename drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h (97%)
 rename drivers/{staging/unisys => virt}/visorbus/vbuschannel.h (99%)
 rename drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h (95%)
 rename drivers/{staging/unisys/include => virt/visorbus}/vbushelper.h (100%)
 create mode 100644 drivers/virt/visorbus/visorbus_main.c
 create mode 100644 drivers/virt/visorbus/visorbus_private.h
 create mode 100644 drivers/virt/visorbus/visorchannel.c
 rename drivers/{staging/unisys => virt}/visorbus/visorchipset.c (98%)
 rename drivers/{staging/unisys => virt}/visorbus/vmcallinterface.h (98%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/channel.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/channel_guid.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/diagchannel.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/guestlinuxdebug.h (98%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/iochannel.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/version.h (100%)
 create mode 100644 include/linux/visorbus/visorbus.h

-- 
1.9.1

[PATCH v2 01/27] staging: unisys: visorbus change -1 return values

[David Kershner]

From: Erik Arfvidson <erik.arfvidson@unisys.com>

This patch changes the vague -1 return values to -EFAULT since
it would be the most appropriate, given that this error
would only occur in an unexpected bad offset field.
Resulting in a bad address.

Signed-off-by: Erik Arfvidson <erik.arfvidson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 3a147db..d32b898 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -876,10 +876,10 @@ write_vbus_chp_info(struct visorchannel *chan,
 	int off = sizeof(struct channel_header) + hdr_info->chp_info_offset;
 
 	if (hdr_info->chp_info_offset == 0)
-		return -1;
+		return -EFAULT;
 
 	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-		return -1;
+		return -EFAULT;
 	return 0;
 }
 
@@ -895,10 +895,10 @@ write_vbus_bus_info(struct visorchannel *chan,
 	int off = sizeof(struct channel_header) + hdr_info->bus_info_offset;
 
 	if (hdr_info->bus_info_offset == 0)
-		return -1;
+		return -EFAULT;
 
 	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-		return -1;
+		return -EFAULT;
 	return 0;
 }
 
@@ -915,10 +915,10 @@ write_vbus_dev_info(struct visorchannel *chan,
 	    (hdr_info->device_info_struct_bytes * devix);
 
 	if (hdr_info->dev_info_offset == 0)
-		return -1;
+		return -EFAULT;
 
 	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-		return -1;
+		return -EFAULT;
 	return 0;
 }
 
-- 
1.9.1

[PATCH v2 02/27] staging: unisys: visorchipset change -1 return value

[David Kershner]

From: Erik Arfvidson <erik.arfvidson@unisys.com>

This patch changes the vague -1 return value to -EINVAL

Signed-off-by: Erik Arfvidson <erik.arfvidson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 5ba5936..d248c94 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -1613,7 +1613,7 @@ parahotplug_request_complete(int id, u16 active)
 	}
 
 	spin_unlock(&parahotplug_request_list_lock);
-	return -1;
+	return -EINVAL;
 }
 
 /*
-- 
1.9.1

[PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1 return values

[David Kershner]

From: Erik Arfvidson <erik.arfvidson@unisys.com>

This patch changes the vague -1 return values to -EPERM.
This operation is not supported is a good alternative
to -1 because the return is basically telling the caller
that the processor doesn't support vmcall operations.

Signed-off-by: Erik Arfvidson <erik.arfvidson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/iovmcall_gnuc.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/iovmcall_gnuc.h b/drivers/staging/unisys/visorbus/iovmcall_gnuc.h
index b08b6ec..98ea7f3 100644
--- a/drivers/staging/unisys/visorbus/iovmcall_gnuc.h
+++ b/drivers/staging/unisys/visorbus/iovmcall_gnuc.h
@@ -22,7 +22,7 @@ __unisys_vmcall_gnuc(unsigned long tuple, unsigned long reg_ebx,
 
 	cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
 	if (!(cpuid_ecx & 0x80000000))
-		return -1;
+		return -EPERM;
 
 	__asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
 		"a"(tuple), "b"(reg_ebx), "c"(reg_ecx));
@@ -40,7 +40,7 @@ __unisys_extended_vmcall_gnuc(unsigned long long tuple,
 
 	cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
 	if (!(cpuid_ecx & 0x80000000))
-		return -1;
+		return -EPERM;
 
 	__asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
 		"a"(tuple), "b"(reg_ebx), "c"(reg_ecx), "d"(reg_edx));
-- 
1.9.1

[PATCH v2 04/27] staging: unisys: visorbus: remove unused module parameters

[David Kershner]

From: David Binder <david.binder@unisys.com>

Removes unused module parameters from visorbus_main.c, in response to
findings by SonarQube.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index d32b898..8278624 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -27,10 +27,9 @@
 #define MYDRVNAME "visorbus"
 
 /* module parameters */
-static int visorbus_debug;
 static int visorbus_forcematch;
 static int visorbus_forcenomatch;
-static int visorbus_debugref;
+
 #define SERIALLOOPBACKCHANADDR (100 * 1024 * 1024)
 
 /* Display string that is guaranteed to be no longer the 99 characters*/
@@ -1329,9 +1328,6 @@ visorbus_exit(void)
 	remove_bus_type();
 }
 
-module_param_named(debug, visorbus_debug, int, S_IRUGO);
-MODULE_PARM_DESC(visorbus_debug, "1 to debug");
-
 module_param_named(forcematch, visorbus_forcematch, int, S_IRUGO);
 MODULE_PARM_DESC(visorbus_forcematch,
 		 "1 to force a successful dev <--> drv match");
@@ -1339,6 +1335,3 @@ MODULE_PARM_DESC(visorbus_forcematch,
 module_param_named(forcenomatch, visorbus_forcenomatch, int, S_IRUGO);
 MODULE_PARM_DESC(visorbus_forcenomatch,
 		 "1 to force an UNsuccessful dev <--> drv match");
-
-module_param_named(debugref, visorbus_debugref, int, S_IRUGO);
-MODULE_PARM_DESC(visorbus_debugref, "1 to debug reference counting");
-- 
1.9.1

[PATCH v2 05/27] staging: unisys: visorbus: remove unused struct

[David Kershner]

From: David Binder <david.binder@unisys.com>

Removes unused struct definition, channel_size_info, in response to
findings by SonarQube.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 8278624..cb08ce4 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -1257,12 +1257,6 @@ chipset_device_resume(struct visor_device *dev_info)
 	initiate_chipset_device_pause_resume(dev_info, false);
 }
 
-struct channel_size_info {
-	uuid_le guid;
-	unsigned long min_size;
-	unsigned long max_size;
-};
-
 int
 visorbus_init(void)
 {
-- 
1.9.1

[PATCH v2 06/27] staging: unisys: visorbus: modify format string to match argument

[David Kershner]

From: David Binder <david.binder@unisys.com>

Modifies the format string of snprintf to expect an unsigned int
instead of a signed one, per the supplied argument.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index cb08ce4..c30b4b2 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -433,7 +433,7 @@ static ssize_t client_bus_info_show(struct device *dev,
 		if (vdev->name)
 			partition_name = vdev->name;
 		shift = snprintf(pos, remain,
-				 "Client device / client driver info for %s eartition (vbus #%d):\n",
+				 "Client device / client driver info for %s eartition (vbus #%u):\n",
 				 partition_name, vdev->chipset_dev_no);
 		pos += shift;
 		remain -= shift;
-- 
1.9.1

[PATCH v2 07/27] staging: unisys: visornic: Correct comment spelling mistake

[David Kershner]

From: David Binder <david.binder@unisys.com>

Fixes a comment spelling mistake in visornic.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index fd7c9a6..9e5b258 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1587,7 +1587,7 @@ drain_resp_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
  *
  *	Drain the respones queue of any responses from the IO partition.
  *	Process the responses as we get them.
- *	Returns when response queue is empty or when the threadd stops.
+ *	Returns when response queue is empty or when the thread stops.
  */
 static void
 service_resp_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata,
-- 
1.9.1

[PATCH v2 08/27] staging: unisys: include: Remove thread-related enum members

[David Kershner]

From: David Binder <david.binder@unisys.com>

Code relating to ktheads was previously removed from s-Par driver code.
This patch cleans up lingering remnants of kthreads by removing thread-
related enum types.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/include/guestlinuxdebug.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/unisys/include/guestlinuxdebug.h b/drivers/staging/unisys/include/guestlinuxdebug.h
index b81287f..5af3f77 100644
--- a/drivers/staging/unisys/include/guestlinuxdebug.h
+++ b/drivers/staging/unisys/include/guestlinuxdebug.h
@@ -56,7 +56,7 @@ enum driver_pc {		/* POSTCODE driver identifier tuples */
 	UISLIB_PC = 0xD0,
 	UISLIB_PC_uislib_c = 0xD1,
 	UISLIB_PC_uisqueue_c = 0xD2,
-	UISLIB_PC_uisthread_c = 0xD3,
+	/* 0xD3 RESERVED */
 	UISLIB_PC_uisutils_c = 0xD4,
 };
 
@@ -91,7 +91,7 @@ enum event_pc {			/* POSTCODE event identifier tuples */
 	DRIVER_EXIT_PC = 0x0AC,
 	MALLOC_FAILURE_PC = 0x0AD,
 	QUEUE_DELAYED_WORK_PC = 0x0AE,
-	UISLIB_THREAD_FAILURE_PC = 0x0B7,
+	/* 0x0B7 RESERVED */
 	VBUS_CHANNEL_ENTRY_PC = 0x0B8,
 	VBUS_CHANNEL_FAILURE_PC = 0x0B9,
 	VBUS_CHANNEL_EXIT_PC = 0x0BA,
-- 
1.9.1

[PATCH v2 09/27] staging: unisys: visorbus: removed unused periodic_test_workqueue

[David Kershner]

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

periodic_test_workqueue was an unused relic from the past, and was removed.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index c30b4b2..608ca1a 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -116,14 +116,6 @@ struct bus_type visorbus_type = {
 	.bus_groups = visorbus_bus_groups,
 };
 
-static struct delayed_work periodic_work;
-
-/* YES, we need 2 workqueues.
- * The reason is, workitems on the test queue may need to cancel
- * workitems on the other queue.  You will be in for trouble if you try to
- * do this with workitems queued on the same workqueue.
- */
-static struct workqueue_struct *periodic_test_workqueue;
 static struct workqueue_struct *periodic_dev_workqueue;
 static long long bus_count;	/** number of bus instances */
 					/** ever-increasing */
@@ -1306,13 +1298,6 @@ visorbus_exit(void)
 	destroy_workqueue(periodic_dev_workqueue);
 	periodic_dev_workqueue = NULL;
 
-	if (periodic_test_workqueue) {
-		cancel_delayed_work(&periodic_work);
-		flush_workqueue(periodic_test_workqueue);
-		destroy_workqueue(periodic_test_workqueue);
-		periodic_test_workqueue = NULL;
-	}
-
 	list_for_each_safe(listentry, listtmp, &list_all_bus_instances) {
 		struct visor_device *dev = list_entry(listentry,
 						      struct visor_device,
-- 
1.9.1

[PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[David Kershner]

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

Locking in the _interrupt() function is NOT necessary so long as we ensure
that interrupts have been stopped whenever we need to pause or resume the
device, which we now do.

While a device is paused, we ensure that interrupts stay disabled, i.e.
that the _interrupt() function will NOT be called, yet remember the desired
state in devdata->interrupts_enabled if open() or close() are called are
called while the device is paused.  Then when the device is resumed, we
restore the actual state of interrupts (i.e., whether _interrupt() is going
to be called or not) to the desired state in devdata->interrupts_enabled.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/visorinput/visorinput.c | 57 +++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 12a3570..9c00710 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -66,6 +66,7 @@ struct visorinput_devdata {
 	struct rw_semaphore lock_visor_dev; /* lock for dev */
 	struct input_dev *visorinput_dev;
 	bool paused;
+	bool interrupts_enabled;
 	unsigned int keycode_table_bytes; /* size of following array */
 	/* for keyboard devices: visorkbd_keycode[] + visorkbd_ext_keycode[] */
 	unsigned char keycode_table[0];
@@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev *visorinput_dev)
 		return -EINVAL;
 	}
 	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
+
+	/*
+	 * If we're not paused, really enable interrupts.
+	 * Regardless of whether we are paused, set a flag indicating
+	 * interrupts should be enabled so when we resume, interrupts
+	 * will really be enabled.
+	 */
+	down_write(&devdata->lock_visor_dev);
+	devdata->interrupts_enabled = true;
+	if (devdata->paused)
+		goto out_unlock;
 	visorbus_enable_channel_interrupts(devdata->dev);
+
+out_unlock:
+	up_write(&devdata->lock_visor_dev);
 	return 0;
 }
 
@@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev *visorinput_dev)
 		return;
 	}
 	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
+
+	/*
+	 * If we're not paused, really disable interrupts.
+	 * Regardless of whether we are paused, set a flag indicating
+	 * interrupts should be disabled so when we resume we will
+	 * not re-enable them.
+	 */
+
+	down_write(&devdata->lock_visor_dev);
+	devdata->interrupts_enabled = false;
+	if (devdata->paused)
+		goto out_unlock;
 	visorbus_disable_channel_interrupts(devdata->dev);
+
+out_unlock:
+	up_write(&devdata->lock_visor_dev);
 }
 
 /*
@@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
 	 * in visorinput_channel_interrupt()
 	 */
 
-	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);
 }
 
@@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct visor_device *dev)
 	if (!devdata)
 		return;
 
-	down_write(&devdata->lock_visor_dev);
-	if (devdata->paused) /* don't touch device/channel when paused */
-		goto out_locked;
-
 	visorinput_dev = devdata->visorinput_dev;
-	if (!visorinput_dev)
-		goto out_locked;
 
 	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
 		scancode = r.activity.arg1;
@@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct visor_device *dev)
 			break;
 		}
 	}
-out_locked:
-	up_write(&devdata->lock_visor_dev);
 }
 
 static int
@@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
 		rc = -EBUSY;
 		goto out_locked;
 	}
+	if (devdata->interrupts_enabled)
+		visorbus_disable_channel_interrupts(dev);
+
+	/*
+	 * due to above, at this time no thread of execution will be
+	 * in visorinput_channel_interrupt()
+	 */
+
 	devdata->paused = true;
 	complete_func(dev, 0);
 	rc = 0;
@@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
 	}
 	devdata->paused = false;
 	complete_func(dev, 0);
+
+	/*
+	 * Re-establish calls to visorinput_channel_interrupt() if that is
+	 * the desired state that we've kept track of in interrupts_enabled
+	 * while the device was paused.
+	 */
+	if (devdata->interrupts_enabled)
+		visorbus_enable_channel_interrupts(dev);
+
 	rc = 0;
 out_locked:
 	up_write(&devdata->lock_visor_dev);
-- 
1.9.1

[PATCH v2 11/27] staging: unisys: visorbus: use kernel timer instead of workqueue

[David Kershner]

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

A kernel timer is now used as the vehicle to periodically call the
channel_interrupt function of registered visor drivers, instead of a
workqueue.

This simplifies a lot of things by making periodic_work.c and
periodic_work.h no longer necessary.  This change also means that the
channel_interrupt() callbacks registered by visor drivers (via
visorbus_register_visor_driver()) will now be called in atomic context
(i.e., canNOT sleep) rather than kernel thread context (CAN sleep).
Fortunately this did NOT necessitate any change to the existing
channel_interrupt() callbacks, because none of them ever perform any
operations that would be invalid in atomic context.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h       | 10 +++--
 drivers/staging/unisys/visorbus/visorbus_main.c | 54 +++++++------------------
 2 files changed, 21 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 9baf1ec..9bb88bb 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -34,8 +34,9 @@
 #include <linux/poll.h>
 #include <linux/kernel.h>
 #include <linux/uuid.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
 
-#include "periodic_work.h"
 #include "channel.h"
 
 struct visor_driver;
@@ -126,8 +127,8 @@ struct visor_driver {
  * device:			Device struct meant for use by the bus driver
  *				only.
  * list_all:			Used by the bus driver to enumerate devices.
- * periodic_work:		Device work queue. Private use by bus driver
- *				only.
+ * timer:		        Timer fired periodically to do interrupt-type
+ *				activity.
  * being_removed:		Indicates that the device is being removed from
  *				the bus. Private bus driver use only.
  * visordriver_callback_lock:	Used by the bus driver to lock when handling
@@ -157,7 +158,8 @@ struct visor_device {
 	/* These fields are for private use by the bus driver only. */
 	struct device device;
 	struct list_head list_all;
-	struct periodic_work *periodic_work;
+	struct timer_list timer;
+	bool timer_active;
 	bool being_removed;
 	struct semaphore visordriver_callback_lock;
 	bool pausing;
diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 608ca1a..24b27ff 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -19,7 +19,6 @@
 #include "visorbus.h"
 #include "visorbus_private.h"
 #include "version.h"
-#include "periodic_work.h"
 #include "vbuschannel.h"
 #include "guestlinuxdebug.h"
 #include "vmcallinterface.h"
@@ -116,7 +115,6 @@ struct bus_type visorbus_type = {
 	.bus_groups = visorbus_bus_groups,
 };
 
-static struct workqueue_struct *periodic_dev_workqueue;
 static long long bus_count;	/** number of bus instances */
 					/** ever-increasing */
 
@@ -222,10 +220,6 @@ visorbus_release_device(struct device *xdev)
 {
 	struct visor_device *dev = to_visor_device(xdev);
 
-	if (dev->periodic_work) {
-		visor_periodic_work_destroy(dev->periodic_work);
-		dev->periodic_work = NULL;
-	}
 	if (dev->visorchannel) {
 		visorchannel_destroy(dev->visorchannel);
 		dev->visorchannel = NULL;
@@ -530,35 +524,36 @@ unregister_driver_attributes(struct visor_driver *drv)
 }
 
 static void
-dev_periodic_work(void *xdev)
+dev_periodic_work(unsigned long __opaque)
 {
-	struct visor_device *dev = xdev;
+	struct visor_device *dev = (struct visor_device *)__opaque;
 	struct visor_driver *drv = to_visor_driver(dev->device.driver);
 
-	down(&dev->visordriver_callback_lock);
 	if (drv->channel_interrupt)
 		drv->channel_interrupt(dev);
-	up(&dev->visordriver_callback_lock);
-	if (!visor_periodic_work_nextperiod(dev->periodic_work))
-		put_device(&dev->device);
+	mod_timer(&dev->timer, jiffies + POLLJIFFIES_NORMALCHANNEL);
 }
 
 static void
 dev_start_periodic_work(struct visor_device *dev)
 {
-	if (dev->being_removed)
+	if (dev->being_removed || dev->timer_active)
 		return;
 	/* now up by at least 2 */
 	get_device(&dev->device);
-	if (!visor_periodic_work_start(dev->periodic_work))
-		put_device(&dev->device);
+	dev->timer.expires = jiffies + POLLJIFFIES_NORMALCHANNEL;
+	add_timer(&dev->timer);
+	dev->timer_active = true;
 }
 
 static void
 dev_stop_periodic_work(struct visor_device *dev)
 {
-	if (visor_periodic_work_stop(dev->periodic_work))
-		put_device(&dev->device);
+	if (!dev->timer_active)
+		return;
+	del_timer_sync(&dev->timer);
+	dev->timer_active = false;
+	put_device(&dev->device);
 }
 
 /** This is called automatically upon adding a visor_device (device_add), or
@@ -776,17 +771,9 @@ create_visor_device(struct visor_device *dev)
 	dev->device.release = visorbus_release_device;
 	/* keep a reference just for us (now 2) */
 	get_device(&dev->device);
-	dev->periodic_work =
-		visor_periodic_work_create(POLLJIFFIES_NORMALCHANNEL,
-					   periodic_dev_workqueue,
-					   dev_periodic_work,
-					   dev, dev_name(&dev->device));
-	if (!dev->periodic_work) {
-		POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, chipset_dev_no,
-				 DIAG_SEVERITY_ERR);
-		err = -EINVAL;
-		goto err_put;
-	}
+	init_timer(&dev->timer);
+	dev->timer.data = (unsigned long)(dev);
+	dev->timer.function = dev_periodic_work;
 
 	/* bus_id must be a unique name with respect to this bus TYPE
 	 * (NOT bus instance).  That's why we need to include the bus
@@ -1265,13 +1252,6 @@ visorbus_init(void)
 		goto error;
 	}
 
-	periodic_dev_workqueue = create_singlethread_workqueue("visorbus_dev");
-	if (!periodic_dev_workqueue) {
-		POSTCODE_LINUX_2(CREATE_WORKQUEUE_PC, DIAG_SEVERITY_ERR);
-		err = -ENOMEM;
-		goto error;
-	}
-
 	/* This enables us to receive notifications when devices appear for
 	 * which this service partition is to be a server for.
 	 */
@@ -1294,10 +1274,6 @@ visorbus_exit(void)
 	visorchipset_register_busdev(NULL, NULL, NULL);
 	remove_all_visor_devices();
 
-	flush_workqueue(periodic_dev_workqueue); /* better not be any work! */
-	destroy_workqueue(periodic_dev_workqueue);
-	periodic_dev_workqueue = NULL;
-
 	list_for_each_safe(listentry, listtmp, &list_all_bus_instances) {
 		struct visor_device *dev = list_entry(listentry,
 						      struct visor_device,
-- 
1.9.1

[PATCH v2 12/27] staging: unisys: visorbus: remove periodic_work.h/.c

[David Kershner]

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

These files were made no-longer-necessary by recent commits.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/include/periodic_work.h  |  40 -----
 drivers/staging/unisys/visorbus/Makefile        |   1 -
 drivers/staging/unisys/visorbus/periodic_work.c | 204 ------------------------
 drivers/staging/unisys/visorbus/visorchipset.c  |   1 -
 4 files changed, 246 deletions(-)
 delete mode 100644 drivers/staging/unisys/include/periodic_work.h
 delete mode 100644 drivers/staging/unisys/visorbus/periodic_work.c

diff --git a/drivers/staging/unisys/include/periodic_work.h b/drivers/staging/unisys/include/periodic_work.h
deleted file mode 100644
index 0b3335a..0000000
--- a/drivers/staging/unisys/include/periodic_work.h
+++ /dev/null
@@ -1,40 +0,0 @@
-/* periodic_work.h
- *
- * Copyright (C) 2010 - 2013 UNISYS CORPORATION
- * All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or (at
- * your option) any later version.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT.  See the GNU General Public License for more
- * details.
- */
-
-#ifndef __PERIODIC_WORK_H__
-#define __PERIODIC_WORK_H__
-
-#include <linux/seq_file.h>
-#include <linux/slab.h>
-
-/* PERIODIC_WORK an opaque structure to users.
- * Fields are declared only in the implementation .c files.
- */
-struct periodic_work;
-
-struct periodic_work *
-visor_periodic_work_create(ulong jiffy_interval,
-			   struct workqueue_struct *workqueue,
-			   void (*workfunc)(void *),
-			   void *workfuncarg,
-			   const char *devnam);
-void visor_periodic_work_destroy(struct periodic_work *pw);
-bool visor_periodic_work_nextperiod(struct periodic_work *pw);
-bool visor_periodic_work_start(struct periodic_work *pw);
-bool visor_periodic_work_stop(struct periodic_work *pw);
-
-#endif
diff --git a/drivers/staging/unisys/visorbus/Makefile b/drivers/staging/unisys/visorbus/Makefile
index fc790e7..f3730d8 100644
--- a/drivers/staging/unisys/visorbus/Makefile
+++ b/drivers/staging/unisys/visorbus/Makefile
@@ -7,6 +7,5 @@ obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus.o
 visorbus-y := visorbus_main.o
 visorbus-y += visorchannel.o
 visorbus-y += visorchipset.o
-visorbus-y += periodic_work.o
 
 ccflags-y += -Idrivers/staging/unisys/include
diff --git a/drivers/staging/unisys/visorbus/periodic_work.c b/drivers/staging/unisys/visorbus/periodic_work.c
deleted file mode 100644
index 00b1527..0000000
--- a/drivers/staging/unisys/visorbus/periodic_work.c
+++ /dev/null
@@ -1,204 +0,0 @@
-/* periodic_work.c
- *
- * Copyright (C) 2010 - 2015 UNISYS CORPORATION
- * All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT.  See the GNU General Public License for more
- * details.
- */
-
-/*
- *  Helper functions to schedule periodic work in Linux kernel mode.
- */
-#include <linux/sched.h>
-
-#include "periodic_work.h"
-
-#define MYDRVNAME "periodic_work"
-
-struct periodic_work {
-	rwlock_t lock;
-	struct delayed_work work;
-	void (*workfunc)(void *);
-	void *workfuncarg;
-	bool is_scheduled;
-	bool want_to_stop;
-	ulong jiffy_interval;
-	struct workqueue_struct *workqueue;
-	const char *devnam;
-};
-
-static void periodic_work_func(struct work_struct *work)
-{
-	struct periodic_work *pw;
-
-	pw = container_of(work, struct periodic_work, work.work);
-	(*pw->workfunc)(pw->workfuncarg);
-}
-
-struct periodic_work
-*visor_periodic_work_create(ulong jiffy_interval,
-			    struct workqueue_struct *workqueue,
-			    void (*workfunc)(void *),
-			    void *workfuncarg,
-			    const char *devnam)
-{
-	struct periodic_work *pw;
-
-	pw = kzalloc(sizeof(*pw), GFP_KERNEL | __GFP_NORETRY);
-	if (!pw)
-		return NULL;
-
-	rwlock_init(&pw->lock);
-	pw->jiffy_interval = jiffy_interval;
-	pw->workqueue = workqueue;
-	pw->workfunc = workfunc;
-	pw->workfuncarg = workfuncarg;
-	pw->devnam = devnam;
-	return pw;
-}
-EXPORT_SYMBOL_GPL(visor_periodic_work_create);
-
-void visor_periodic_work_destroy(struct periodic_work *pw)
-{
-	kfree(pw);
-}
-EXPORT_SYMBOL_GPL(visor_periodic_work_destroy);
-
-/** Call this from your periodic work worker function to schedule the next
- *  call.
- *  If this function returns false, there was a failure and the
- *  periodic work is no longer scheduled
- */
-bool visor_periodic_work_nextperiod(struct periodic_work *pw)
-{
-	bool rc = false;
-
-	write_lock(&pw->lock);
-	if (pw->want_to_stop) {
-		pw->is_scheduled = false;
-		pw->want_to_stop = false;
-		rc = true;  /* yes, true; see visor_periodic_work_stop() */
-		goto unlock;
-	} else if (!queue_delayed_work(pw->workqueue, &pw->work,
-				       pw->jiffy_interval)) {
-		pw->is_scheduled = false;
-		rc = false;
-		goto unlock;
-	}
-	rc = true;
-unlock:
-	write_unlock(&pw->lock);
-	return rc;
-}
-EXPORT_SYMBOL_GPL(visor_periodic_work_nextperiod);
-
-/** This function returns true iff new periodic work was actually started.
- *  If this function returns false, then no work was started
- *  (either because it was already started, or because of a failure).
- */
-bool visor_periodic_work_start(struct periodic_work *pw)
-{
-	bool rc = false;
-
-	write_lock(&pw->lock);
-	if (pw->is_scheduled) {
-		rc = false;
-		goto unlock;
-	}
-	if (pw->want_to_stop) {
-		rc = false;
-		goto unlock;
-	}
-	INIT_DELAYED_WORK(&pw->work, &periodic_work_func);
-	if (!queue_delayed_work(pw->workqueue, &pw->work,
-				pw->jiffy_interval)) {
-		rc = false;
-		goto unlock;
-	}
-	pw->is_scheduled = true;
-	rc = true;
-unlock:
-	write_unlock(&pw->lock);
-	return rc;
-}
-EXPORT_SYMBOL_GPL(visor_periodic_work_start);
-
-/** This function returns true iff your call actually stopped the periodic
- *  work.
- *
- *  -- PAY ATTENTION... this is important --
- *
- *  NO NO #1
- *
- *     Do NOT call this function from some function that is running on the
- *     same workqueue as the work you are trying to stop might be running
- *     on!  If you violate this rule, visor_periodic_work_stop() MIGHT work,
- *     but it also MIGHT get hung up in an infinite loop saying
- *     "waiting for delayed work...".  This will happen if the delayed work
- *     you are trying to cancel has been put in the workqueue list, but can't
- *     run yet because we are running that same workqueue thread right now.
- *
- *     Bottom line: If you need to call visor_periodic_work_stop() from a
- *     workitem, be sure the workitem is on a DIFFERENT workqueue than the
- *     workitem that you are trying to cancel.
- *
- *     If I could figure out some way to check for this "no no" condition in
- *     the code, I would.  It would have saved me the trouble of writing this
- *     long comment.  And also, don't think this is some "theoretical" race
- *     condition.  It is REAL, as I have spent the day chasing it.
- *
- *  NO NO #2
- *
- *     Take close note of the locks that you own when you call this function.
- *     You must NOT own any locks that are needed by the periodic work
- *     function that is currently installed.  If you DO, a deadlock may result,
- *     because stopping the periodic work often involves waiting for the last
- *     iteration of the periodic work function to complete.  Again, if you hit
- *     this deadlock, you will get hung up in an infinite loop saying
- *     "waiting for delayed work...".
- */
-bool visor_periodic_work_stop(struct periodic_work *pw)
-{
-	bool stopped_something = false;
-
-	write_lock(&pw->lock);
-	stopped_something = pw->is_scheduled && (!pw->want_to_stop);
-	while (pw->is_scheduled) {
-		pw->want_to_stop = true;
-		if (cancel_delayed_work(&pw->work)) {
-			/* We get here if the delayed work was pending as
-			 * delayed work, but was NOT run.
-			 */
-			WARN_ON(!pw->is_scheduled);
-			pw->is_scheduled = false;
-		} else {
-			/* If we get here, either the delayed work:
-			 * - was run, OR,
-			 * - is running RIGHT NOW on another processor, OR,
-			 * - wasn't even scheduled (there is a miniscule
-			 *   timing window where this could be the case)
-			 * flush_workqueue() would make sure it is finished
-			 * executing, but that still isn't very useful, which
-			 * explains the loop...
-			 */
-		}
-		if (pw->is_scheduled) {
-			write_unlock(&pw->lock);
-			schedule_timeout_interruptible(msecs_to_jiffies(10));
-			write_lock(&pw->lock);
-		} else {
-			pw->want_to_stop = false;
-		}
-	}
-	write_unlock(&pw->lock);
-	return stopped_something;
-}
-EXPORT_SYMBOL_GPL(visor_periodic_work_stop);
diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index d248c94..b6a4d21 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -29,7 +29,6 @@
 #include "controlvmchannel.h"
 #include "controlvmcompletionstatus.h"
 #include "guestlinuxdebug.h"
-#include "periodic_work.h"
 #include "version.h"
 #include "visorbus.h"
 #include "visorbus_private.h"
-- 
1.9.1

[PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock a mutex

[David Kershner]

From: Bryan Thompson <bryan.thompson@unisys.com>

visordriver_callback_lock is just a binary semaphore that logically
makes more sense as a mutex.

Signed-off-by: Bryan Thompson <bryan.thompson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h       |  3 ++-
 drivers/staging/unisys/visorbus/visorbus_main.c | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 9bb88bb..9da25c0 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -161,7 +161,8 @@ struct visor_device {
 	struct timer_list timer;
 	bool timer_active;
 	bool being_removed;
-	struct semaphore visordriver_callback_lock;
+	/* mutex to serialize visor_driver function callbacks */
+	struct mutex visordriver_callback_lock;
 	bool pausing;
 	bool resuming;
 	u32 chipset_bus_no;
diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 24b27ff..44609ee 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -574,7 +574,7 @@ visordriver_probe_device(struct device *xdev)
 	if (!drv->probe)
 		return -ENODEV;
 
-	down(&dev->visordriver_callback_lock);
+	mutex_lock(&dev->visordriver_callback_lock);
 	dev->being_removed = false;
 
 	res = drv->probe(dev);
@@ -584,7 +584,7 @@ visordriver_probe_device(struct device *xdev)
 		fix_vbus_dev_info(dev);
 	}
 
-	up(&dev->visordriver_callback_lock);
+	mutex_unlock(&dev->visordriver_callback_lock);
 	return res;
 }
 
@@ -600,11 +600,11 @@ visordriver_remove_device(struct device *xdev)
 
 	dev = to_visor_device(xdev);
 	drv = to_visor_driver(xdev->driver);
-	down(&dev->visordriver_callback_lock);
+	mutex_lock(&dev->visordriver_callback_lock);
 	dev->being_removed = true;
 	if (drv->remove)
 		drv->remove(dev);
-	up(&dev->visordriver_callback_lock);
+	mutex_unlock(&dev->visordriver_callback_lock);
 	dev_stop_periodic_work(dev);
 
 	put_device(&dev->device);
@@ -764,7 +764,7 @@ create_visor_device(struct visor_device *dev)
 	POSTCODE_LINUX_4(DEVICE_CREATE_ENTRY_PC, chipset_dev_no, chipset_bus_no,
 			 POSTCODE_SEVERITY_INFO);
 
-	sema_init(&dev->visordriver_callback_lock, 1);	/* unlocked */
+	mutex_init(&dev->visordriver_callback_lock);
 	dev->device.bus = &visorbus_type;
 	dev->device.groups = visorbus_channel_groups;
 	device_initialize(&dev->device);
-- 
1.9.1

[PATCH v2 14/27] staging: unisys: visorbus: Remove unnecessary EXPORT_SYMBOL statements

[David Kershner]

From: Bryan Thompson <bryan.thompson@unisys.com>

The driver that is now visorbus started out as multiple separate drivers,
and when they were merged the EXPORT_SYMBOL statements that were required
for separate drivers were left in the code. This patch removes those now
unnecessary exports.

Signed-off-by: Bryan Thompson <bryan.thompson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c |  1 -
 drivers/staging/unisys/visorbus/visorchannel.c  | 17 -----------------
 drivers/staging/unisys/visorbus/visorchipset.c  |  2 --
 3 files changed, 20 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 44609ee..247a9ad 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -721,7 +721,6 @@ visorbus_clear_channel(struct visor_device *dev, unsigned long offset, u8 ch,
 {
 	return visorchannel_clear(dev->visorchannel, offset, ch, nbytes);
 }
-EXPORT_SYMBOL_GPL(visorbus_clear_channel);
 
 /** We don't really have a real interrupt, so for now we just call the
  *  interrupt function periodically...
diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
index 4337358..1f626c3 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -148,7 +148,6 @@ visorchannel_create(u64 physaddr, unsigned long channel_bytes,
 	return visorchannel_create_guts(physaddr, channel_bytes, gfp, 0, guid,
 					false);
 }
-EXPORT_SYMBOL_GPL(visorchannel_create);
 
 struct visorchannel *
 visorchannel_create_with_lock(u64 physaddr, unsigned long channel_bytes,
@@ -157,7 +156,6 @@ visorchannel_create_with_lock(u64 physaddr, unsigned long channel_bytes,
 	return visorchannel_create_guts(physaddr, channel_bytes, gfp, 0, guid,
 					true);
 }
-EXPORT_SYMBOL_GPL(visorchannel_create_with_lock);
 
 void
 visorchannel_destroy(struct visorchannel *channel)
@@ -171,21 +169,18 @@ visorchannel_destroy(struct visorchannel *channel)
 	}
 	kfree(channel);
 }
-EXPORT_SYMBOL_GPL(visorchannel_destroy);
 
 u64
 visorchannel_get_physaddr(struct visorchannel *channel)
 {
 	return channel->physaddr;
 }
-EXPORT_SYMBOL_GPL(visorchannel_get_physaddr);
 
 ulong
 visorchannel_get_nbytes(struct visorchannel *channel)
 {
 	return channel->nbytes;
 }
-EXPORT_SYMBOL_GPL(visorchannel_get_nbytes);
 
 char *
 visorchannel_uuid_id(uuid_le *guid, char *s)
@@ -193,28 +188,24 @@ visorchannel_uuid_id(uuid_le *guid, char *s)
 	sprintf(s, "%pUL", guid);
 	return s;
 }
-EXPORT_SYMBOL_GPL(visorchannel_uuid_id);
 
 char *
 visorchannel_id(struct visorchannel *channel, char *s)
 {
 	return visorchannel_uuid_id(&channel->guid, s);
 }
-EXPORT_SYMBOL_GPL(visorchannel_id);
 
 char *
 visorchannel_zoneid(struct visorchannel *channel, char *s)
 {
 	return visorchannel_uuid_id(&channel->chan_hdr.zone_uuid, s);
 }
-EXPORT_SYMBOL_GPL(visorchannel_zoneid);
 
 u64
 visorchannel_get_clientpartition(struct visorchannel *channel)
 {
 	return channel->chan_hdr.partition_handle;
 }
-EXPORT_SYMBOL_GPL(visorchannel_get_clientpartition);
 
 int
 visorchannel_set_clientpartition(struct visorchannel *channel,
@@ -223,7 +214,6 @@ visorchannel_set_clientpartition(struct visorchannel *channel,
 	channel->chan_hdr.partition_handle = partition_handle;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(visorchannel_set_clientpartition);
 
 uuid_le
 visorchannel_get_uuid(struct visorchannel *channel)
@@ -243,7 +233,6 @@ visorchannel_read(struct visorchannel *channel, ulong offset,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(visorchannel_read);
 
 int
 visorchannel_write(struct visorchannel *channel, ulong offset,
@@ -265,7 +254,6 @@ visorchannel_write(struct visorchannel *channel, ulong offset,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(visorchannel_write);
 
 int
 visorchannel_clear(struct visorchannel *channel, ulong offset, u8 ch,
@@ -301,14 +289,12 @@ out_free_page:
 	free_page((unsigned long)buf);
 	return err;
 }
-EXPORT_SYMBOL_GPL(visorchannel_clear);
 
 void __iomem  *
 visorchannel_get_header(struct visorchannel *channel)
 {
 	return (void __iomem *)&channel->chan_hdr;
 }
-EXPORT_SYMBOL_GPL(visorchannel_get_header);
 
 /** Return offset of a specific SIGNAL_QUEUE_HEADER from the beginning of a
  *  channel header
@@ -522,7 +508,6 @@ visorchannel_signalqueue_slots_avail(struct visorchannel *channel, u32 queue)
 	slots_avail = sig_hdr.max_signals - slots_used;
 	return (int)slots_avail;
 }
-EXPORT_SYMBOL_GPL(visorchannel_signalqueue_slots_avail);
 
 int
 visorchannel_signalqueue_max_slots(struct visorchannel *channel, u32 queue)
@@ -533,7 +518,6 @@ visorchannel_signalqueue_max_slots(struct visorchannel *channel, u32 queue)
 		return 0;
 	return (int)sig_hdr.max_signals;
 }
-EXPORT_SYMBOL_GPL(visorchannel_signalqueue_max_slots);
 
 static void
 sigqueue_debug(struct signal_queue_header *q, int which, struct seq_file *seq)
@@ -632,4 +616,3 @@ visorchannel_debug(struct visorchannel *channel, int num_queues,
 	seq_printf(seq, "--- End   channel @0x%-16.16Lx for 0x%lx bytes ---\n",
 		   addr + off, nbytes);
 }
-EXPORT_SYMBOL_GPL(visorchannel_debug);
diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index b6a4d21..1da980f 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -682,7 +682,6 @@ struct visor_device *visorbus_get_device_by_id(u32 bus_no, u32 dev_no,
 		vdev = to_visor_device(dev);
 	return vdev;
 }
-EXPORT_SYMBOL(visorbus_get_device_by_id);
 
 void
 visorchipset_register_busdev(
@@ -707,7 +706,6 @@ visorchipset_register_busdev(
 
 	up(&notifier_lock);
 }
-EXPORT_SYMBOL_GPL(visorchipset_register_busdev);
 
 static void
 chipset_init(struct controlvm_message *inmsg)
-- 
1.9.1

[PATCH v2 15/27] staging: unisys: visorbus: Remove unused functions

[David Kershner]

From: Bryan Thompson <bryan.thompson@unisys.com>

Remove visorbus_clear_channel, visorchannel_signalqueue_slots_avail,
visorchannel_signalqueue_max_slots, visorchannel_clear, and
visorchannel_debug which are no longer called by any driver.

Signed-off-by: Bryan Thompson <bryan.thompson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h       |   9 --
 drivers/staging/unisys/visorbus/visorbus_main.c |   7 --
 drivers/staging/unisys/visorbus/visorchannel.c  | 161 ------------------------
 3 files changed, 177 deletions(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 9da25c0..2fd3016 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -186,8 +186,6 @@ int visorbus_read_channel(struct visor_device *dev,
 int visorbus_write_channel(struct visor_device *dev,
 			   unsigned long offset, void *src,
 			   unsigned long nbytes);
-int visorbus_clear_channel(struct visor_device *dev,
-			   unsigned long offset, u8 ch, unsigned long nbytes);
 void visorbus_enable_channel_interrupts(struct visor_device *dev);
 void visorbus_disable_channel_interrupts(struct visor_device *dev);
 #endif
@@ -207,17 +205,12 @@ int visorchannel_read(struct visorchannel *channel, ulong offset,
 		      void *local, ulong nbytes);
 int visorchannel_write(struct visorchannel *channel, ulong offset,
 		       void *local, ulong nbytes);
-int visorchannel_clear(struct visorchannel *channel, ulong offset,
-		       u8 ch, ulong nbytes);
 bool visorchannel_signalremove(struct visorchannel *channel, u32 queue,
 			       void *msg);
 bool visorchannel_signalinsert(struct visorchannel *channel, u32 queue,
 			       void *msg);
 bool visorchannel_signalempty(struct visorchannel *channel, u32 queue);
 
-int visorchannel_signalqueue_slots_avail(struct visorchannel *channel,
-					 u32 queue);
-int visorchannel_signalqueue_max_slots(struct visorchannel *channel, u32 queue);
 u64 visorchannel_get_physaddr(struct visorchannel *channel);
 ulong visorchannel_get_nbytes(struct visorchannel *channel);
 char *visorchannel_id(struct visorchannel *channel, char *s);
@@ -227,8 +220,6 @@ int visorchannel_set_clientpartition(struct visorchannel *channel,
 				     u64 partition_handle);
 uuid_le visorchannel_get_uuid(struct visorchannel *channel);
 char *visorchannel_uuid_id(uuid_le *guid, char *s);
-void visorchannel_debug(struct visorchannel *channel, int num_queues,
-			struct seq_file *seq, u32 off);
 void __iomem *visorchannel_get_header(struct visorchannel *channel);
 
 #define BUS_ROOT_DEVICE		UINT_MAX
diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 247a9ad..c3f53fb 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -715,13 +715,6 @@ visorbus_write_channel(struct visor_device *dev, unsigned long offset,
 }
 EXPORT_SYMBOL_GPL(visorbus_write_channel);
 
-int
-visorbus_clear_channel(struct visor_device *dev, unsigned long offset, u8 ch,
-		       unsigned long nbytes)
-{
-	return visorchannel_clear(dev->visorchannel, offset, ch, nbytes);
-}
-
 /** We don't really have a real interrupt, so for now we just call the
  *  interrupt function periodically...
  */
diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
index 1f626c3..43315c2 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -255,41 +255,6 @@ visorchannel_write(struct visorchannel *channel, ulong offset,
 	return 0;
 }
 
-int
-visorchannel_clear(struct visorchannel *channel, ulong offset, u8 ch,
-		   ulong nbytes)
-{
-	int err;
-	int bufsize = PAGE_SIZE;
-	int written = 0;
-	u8 *buf;
-
-	buf = (u8 *)__get_free_page(GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	memset(buf, ch, bufsize);
-
-	while (nbytes > 0) {
-		int thisbytes = bufsize;
-
-		if (nbytes < thisbytes)
-			thisbytes = nbytes;
-		err = visorchannel_write(channel, offset + written,
-					 buf, thisbytes);
-		if (err)
-			goto out_free_page;
-
-		written += thisbytes;
-		nbytes -= thisbytes;
-	}
-	err = 0;
-
-out_free_page:
-	free_page((unsigned long)buf);
-	return err;
-}
-
 void __iomem  *
 visorchannel_get_header(struct visorchannel *channel)
 {
@@ -490,129 +455,3 @@ visorchannel_signalinsert(struct visorchannel *channel, u32 queue, void *msg)
 	return rc;
 }
 EXPORT_SYMBOL_GPL(visorchannel_signalinsert);
-
-int
-visorchannel_signalqueue_slots_avail(struct visorchannel *channel, u32 queue)
-{
-	struct signal_queue_header sig_hdr;
-	u32 slots_avail, slots_used;
-	u32 head, tail;
-
-	if (!sig_read_header(channel, queue, &sig_hdr))
-		return 0;
-	head = sig_hdr.head;
-	tail = sig_hdr.tail;
-	if (head < tail)
-		head = head + sig_hdr.max_slots;
-	slots_used = head - tail;
-	slots_avail = sig_hdr.max_signals - slots_used;
-	return (int)slots_avail;
-}
-
-int
-visorchannel_signalqueue_max_slots(struct visorchannel *channel, u32 queue)
-{
-	struct signal_queue_header sig_hdr;
-
-	if (!sig_read_header(channel, queue, &sig_hdr))
-		return 0;
-	return (int)sig_hdr.max_signals;
-}
-
-static void
-sigqueue_debug(struct signal_queue_header *q, int which, struct seq_file *seq)
-{
-	seq_printf(seq, "Signal Queue #%d\n", which);
-	seq_printf(seq, "   VersionId          = %lu\n", (ulong)q->version);
-	seq_printf(seq, "   Type               = %lu\n", (ulong)q->chtype);
-	seq_printf(seq, "   oSignalBase        = %llu\n",
-		   (long long)q->sig_base_offset);
-	seq_printf(seq, "   SignalSize         = %lu\n", (ulong)q->signal_size);
-	seq_printf(seq, "   MaxSignalSlots     = %lu\n",
-		   (ulong)q->max_slots);
-	seq_printf(seq, "   MaxSignals         = %lu\n", (ulong)q->max_signals);
-	seq_printf(seq, "   FeatureFlags       = %-16.16Lx\n",
-		   (long long)q->features);
-	seq_printf(seq, "   NumSignalsSent     = %llu\n",
-		   (long long)q->num_sent);
-	seq_printf(seq, "   NumSignalsReceived = %llu\n",
-		   (long long)q->num_received);
-	seq_printf(seq, "   NumOverflows       = %llu\n",
-		   (long long)q->num_overflows);
-	seq_printf(seq, "   Head               = %lu\n", (ulong)q->head);
-	seq_printf(seq, "   Tail               = %lu\n", (ulong)q->tail);
-}
-
-void
-visorchannel_debug(struct visorchannel *channel, int num_queues,
-		   struct seq_file *seq, u32 off)
-{
-	u64 addr = 0;
-	ulong nbytes = 0, nbytes_region = 0;
-	struct channel_header hdr;
-	struct channel_header *phdr = &hdr;
-	int i = 0;
-	int errcode = 0;
-
-	if (!channel)
-		return;
-
-	addr = visorchannel_get_physaddr(channel);
-	nbytes_region = visorchannel_get_nbytes(channel);
-	errcode = visorchannel_read(channel, off,
-				    phdr, sizeof(struct channel_header));
-	if (errcode < 0) {
-		seq_printf(seq,
-			   "Read of channel header failed with errcode=%d)\n",
-			   errcode);
-		if (off == 0) {
-			phdr = &channel->chan_hdr;
-			seq_puts(seq, "(following data may be stale)\n");
-		} else {
-			return;
-		}
-	}
-	nbytes = (ulong)(phdr->size);
-	seq_printf(seq, "--- Begin channel @0x%-16.16Lx for 0x%lx bytes (region=0x%lx bytes) ---\n",
-		   addr + off, nbytes, nbytes_region);
-	seq_printf(seq, "Type            = %pUL\n", &phdr->chtype);
-	seq_printf(seq, "ZoneGuid        = %pUL\n", &phdr->zone_uuid);
-	seq_printf(seq, "Signature       = 0x%-16.16Lx\n",
-		   (long long)phdr->signature);
-	seq_printf(seq, "LegacyState     = %lu\n", (ulong)phdr->legacy_state);
-	seq_printf(seq, "SrvState        = %lu\n", (ulong)phdr->srv_state);
-	seq_printf(seq, "CliStateBoot    = %lu\n", (ulong)phdr->cli_state_boot);
-	seq_printf(seq, "CliStateOS      = %lu\n", (ulong)phdr->cli_state_os);
-	seq_printf(seq, "HeaderSize      = %lu\n", (ulong)phdr->header_size);
-	seq_printf(seq, "Size            = %llu\n", (long long)phdr->size);
-	seq_printf(seq, "Features        = 0x%-16.16llx\n",
-		   (long long)phdr->features);
-	seq_printf(seq, "PartitionHandle = 0x%-16.16llx\n",
-		   (long long)phdr->partition_handle);
-	seq_printf(seq, "Handle          = 0x%-16.16llx\n",
-		   (long long)phdr->handle);
-	seq_printf(seq, "VersionId       = %lu\n", (ulong)phdr->version_id);
-	seq_printf(seq, "oChannelSpace   = %llu\n",
-		   (long long)phdr->ch_space_offset);
-	if ((phdr->ch_space_offset == 0) || (errcode < 0))
-		;
-	else
-		for (i = 0; i < num_queues; i++) {
-			struct signal_queue_header q;
-
-			errcode = visorchannel_read(channel,
-						    off +
-						    phdr->ch_space_offset +
-						    (i * sizeof(q)),
-						    &q, sizeof(q));
-			if (errcode < 0) {
-				seq_printf(seq,
-					   "failed to read signal queue #%d from channel @0x%-16.16Lx errcode=%d\n",
-					   i, addr, errcode);
-				continue;
-			}
-			sigqueue_debug(&q, i, seq);
-		}
-	seq_printf(seq, "--- End   channel @0x%-16.16Lx for 0x%lx bytes ---\n",
-		   addr + off, nbytes);
-}
-- 
1.9.1

[PATCH v2 16/27] staging: unisys: Remove reference to unused STANDALONE_CLIENT

[David Kershner]

From: Bryan Thompson <bryan.thompson@unisys.com>

The STANDALONE_CLIENT define is no longer used by Unisys driver code.

Signed-off-by: Bryan Thompson <bryan.thompson@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 2fd3016..94dd48e 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -177,7 +177,6 @@ struct visor_device {
 
 #define to_visor_device(x) container_of(x, struct visor_device, device)
 
-#ifndef STANDALONE_CLIENT
 int visorbus_register_visor_driver(struct visor_driver *);
 void visorbus_unregister_visor_driver(struct visor_driver *);
 int visorbus_read_channel(struct visor_device *dev,
@@ -188,7 +187,6 @@ int visorbus_write_channel(struct visor_device *dev,
 			   unsigned long nbytes);
 void visorbus_enable_channel_interrupts(struct visor_device *dev);
 void visorbus_disable_channel_interrupts(struct visor_device *dev);
-#endif
 
 /* Note that for visorchannel_create()
  * <channel_bytes> and <guid> arguments may be 0 if we are a channel CLIENT.
-- 
1.9.1

[PATCH v2 17/27] staging: unisys: visorbus: vbusdeviceinfo function descriptions more kerneldoc-like

[David Kershner]

From: David Binder <david.binder@unisys.com>

Per audit feedback from Thomas Gleixner, function descriptions in
vbusdeviceinfo.h now utilize a more kerneldoc-like formatting. The
affected comments do not implement other kerneldoc requirements.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/vbusdeviceinfo.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/vbusdeviceinfo.h b/drivers/staging/unisys/visorbus/vbusdeviceinfo.h
index abdab4a..010ea68 100644
--- a/drivers/staging/unisys/visorbus/vbusdeviceinfo.h
+++ b/drivers/staging/unisys/visorbus/vbusdeviceinfo.h
@@ -34,7 +34,8 @@ struct ultra_vbus_deviceinfo {
 
 #pragma pack(pop)
 
-/* Reads chars from the buffer at <src> for <srcmax> bytes, and writes to
+/**
+ * Reads chars from the buffer at <src> for <srcmax> bytes, and writes to
  * the buffer at <p>, which is <remain> bytes long, ensuring never to
  * overflow the buffer at <p>, using the following rules:
  * - printable characters are simply copied from the buffer at <src> to the
@@ -92,7 +93,8 @@ vbuschannel_sanitize_buffer(char *p, int remain, char *src, int srcmax)
 		p++;  chars++;  remain--;	   \
 	} while (0)
 
-/* Converts the non-negative value at <num> to an ascii decimal string
+/**
+ * Converts the non-negative value at <num> to an ascii decimal string
  * at <p>, writing at most <remain> bytes.  Note there is NO '\0' termination
  * written to <p>.
  *
@@ -141,8 +143,9 @@ vbuschannel_itoa(char *p, int remain, int num)
 	return digits;
 }
 
-/* Reads <devInfo>, and converts its contents to a printable string at <p>,
- * writing at most <remain> bytes.  Note there is NO '\0' termination
+/**
+ * Reads <devInfo>, and converts its contents to a printable string at <p>,
+ * writing at most <remain> bytes. Note there is NO '\0' termination
  * written to <p>.
  *
  * Pass <devix> >= 0 if you want a device index presented.
-- 
1.9.1

[PATCH v2 18/27] staging: unisys: visorbus: make function descriptions more kerneldoc-like

[David Kershner]

From: David Binder <david.binder@unisys.com>

Per audit feedback from Thomas Gleixner, function descriptions in
visorbus_main.c now utilize a more kerneldoc-like formatting. The affected
comments do not implement other kerneldoc requirements.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 178 +++++++++++++-----------
 1 file changed, 98 insertions(+), 80 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index c3f53fb..850998e 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -166,7 +166,8 @@ visorbus_uevent(struct device *xdev, struct kobj_uevent_env *env)
 	return 0;
 }
 
-/* This is called automatically upon adding a visor_device (device_add), or
+/**
+ * This is called automatically upon adding a visor_device (device_add), or
  * adding a visor_driver (visorbus_register_visor_driver), and returns 1 iff the
  * provided driver can control the specified device.
  */
@@ -200,9 +201,10 @@ visorbus_match(struct device *xdev, struct device_driver *xdrv)
 	return 0;
 }
 
-/** This is called when device_unregister() is called for the bus device
- *  instance, after all other tasks involved with destroying the device
- *  are complete.
+/**
+ * This is called when device_unregister() is called for the bus device
+ * instance, after all other tasks involved with destroying the device
+ * are complete.
  */
 static void
 visorbus_release_busdevice(struct device *xdev)
@@ -212,8 +214,9 @@ visorbus_release_busdevice(struct device *xdev)
 	kfree(dev);
 }
 
-/** This is called when device_unregister() is called for each child
- *  device instance.
+/**
+ * This is called when device_unregister() is called for each child
+ * device instance.
  */
 static void
 visorbus_release_device(struct device *xdev)
@@ -556,10 +559,11 @@ dev_stop_periodic_work(struct visor_device *dev)
 	put_device(&dev->device);
 }
 
-/** This is called automatically upon adding a visor_device (device_add), or
- *  adding a visor_driver (visorbus_register_visor_driver), but only after
- *  visorbus_match has returned 1 to indicate a successful match between
- *  driver and device.
+/**
+ * This is called automatically upon adding a visor_device (device_add), or
+ * adding a visor_driver (visorbus_register_visor_driver), but only after
+ * visorbus_match has returned 1 to indicate a successful match between
+ * driver and device.
  */
 static int
 visordriver_probe_device(struct device *xdev)
@@ -588,9 +592,10 @@ visordriver_probe_device(struct device *xdev)
 	return res;
 }
 
-/** This is called when device_unregister() is called for each child device
- *  instance, to notify the appropriate visorbus_driver that the device is
- *  going away, and to decrease the reference count of the device.
+/**
+ * This is called when device_unregister() is called for each child device
+ * instance, to notify the appropriate visorbus_driver that the device is
+ * going away, and to decrease the reference count of the device.
  */
 static int
 visordriver_remove_device(struct device *xdev)
@@ -611,47 +616,47 @@ visordriver_remove_device(struct device *xdev)
 	return 0;
 }
 
-/** A particular type of visor driver calls this function to register
- *  the driver.  The caller MUST fill in the following fields within the
- *  #drv structure:
- *      name, version, owner, channel_types, probe, remove
+/**
+ * A particular type of visor driver calls this function to register
+ * the driver.  The caller MUST fill in the following fields within the
+ * #drv structure:
+ *     name, version, owner, channel_types, probe, remove
  *
- *  Here's how the whole Linux bus / driver / device model works.
+ * Here's how the whole Linux bus / driver / device model works.
  *
- *  At system start-up, the visorbus kernel module is loaded, which registers
- *  visorbus_type as a bus type, using bus_register().
+ * At system start-up, the visorbus kernel module is loaded, which registers
+ * visorbus_type as a bus type, using bus_register().
  *
- *  All kernel modules that support particular device types on a
- *  visorbus bus are loaded.  Each of these kernel modules calls
- *  visorbus_register_visor_driver() in their init functions, passing a
- *  visor_driver struct.  visorbus_register_visor_driver() in turn calls
- *  register_driver(&visor_driver.driver).  This .driver member is
- *  initialized with generic methods (like probe), whose sole responsibility
- *  is to act as a broker for the real methods, which are within the
- *  visor_driver struct.  (This is the way the subclass behavior is
- *  implemented, since visor_driver is essentially a subclass of the
- *  generic driver.)  Whenever a driver_register() happens, core bus code in
- *  the kernel does (see device_attach() in drivers/base/dd.c):
+ * All kernel modules that support particular device types on a
+ * visorbus bus are loaded.  Each of these kernel modules calls
+ * visorbus_register_visor_driver() in their init functions, passing a
+ * visor_driver struct.  visorbus_register_visor_driver() in turn calls
+ * register_driver(&visor_driver.driver).  This .driver member is
+ * initialized with generic methods (like probe), whose sole responsibility
+ * is to act as a broker for the real methods, which are within the
+ * visor_driver struct.  (This is the way the subclass behavior is
+ * implemented, since visor_driver is essentially a subclass of the
+ * generic driver.)  Whenever a driver_register() happens, core bus code in
+ * the kernel does (see device_attach() in drivers/base/dd.c):
  *
- *      for each dev associated with the bus (the bus that driver is on) that
- *      does not yet have a driver
- *          if bus.match(dev,newdriver) == yes_matched  ** .match specified
- *                                                 ** during bus_register().
- *              newdriver.probe(dev)  ** for visor drivers, this will call
- *                    ** the generic driver.probe implemented in visorbus.c,
- *                    ** which in turn calls the probe specified within the
- *                    ** struct visor_driver (which was specified by the
- *                    ** actual device driver as part of
- *                    ** visorbus_register_visor_driver()).
- *
- *  The above dance also happens when a new device appears.
- *  So the question is, how are devices created within the system?
- *  Basically, just call device_add(dev).  See pci_bus_add_devices().
- *  pci_scan_device() shows an example of how to build a device struct.  It
- *  returns the newly-created struct to pci_scan_single_device(), who adds it
- *  to the list of devices at PCIBUS.devices.  That list of devices is what
- *  is traversed by pci_bus_add_devices().
+ *     for each dev associated with the bus (the bus that driver is on) that
+ *     does not yet have a driver
+ *         if bus.match(dev,newdriver) == yes_matched  ** .match specified
+ *                                                ** during bus_register().
+ *             newdriver.probe(dev)  ** for visor drivers, this will call
+ *                   ** the generic driver.probe implemented in visorbus.c,
+ *                   ** which in turn calls the probe specified within the
+ *                   ** struct visor_driver (which was specified by the
+ *                   ** actual device driver as part of
+ *                   ** visorbus_register_visor_driver()).
  *
+ * The above dance also happens when a new device appears.
+ * So the question is, how are devices created within the system?
+ * Basically, just call device_add(dev).  See pci_bus_add_devices().
+ * pci_scan_device() shows an example of how to build a device struct.  It
+ * returns the newly-created struct to pci_scan_single_device(), who adds it
+ * to the list of devices at PCIBUS.devices.  That list of devices is what
+ * is traversed by pci_bus_add_devices().
  */
 int visorbus_register_visor_driver(struct visor_driver *drv)
 {
@@ -688,8 +693,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
 }
 EXPORT_SYMBOL_GPL(visorbus_register_visor_driver);
 
-/** A particular type of visor driver calls this function to unregister
- *  the driver, i.e., within its module_exit function.
+/**
+ * A particular type of visor driver calls this function to unregister
+ * the driver, i.e., within its module_exit function.
  */
 void
 visorbus_unregister_visor_driver(struct visor_driver *drv)
@@ -715,8 +721,9 @@ visorbus_write_channel(struct visor_device *dev, unsigned long offset,
 }
 EXPORT_SYMBOL_GPL(visorbus_write_channel);
 
-/** We don't really have a real interrupt, so for now we just call the
- *  interrupt function periodically...
+/**
+ * We don't really have a real interrupt, so for now we just call the
+ * interrupt function periodically...
  */
 void
 visorbus_enable_channel_interrupts(struct visor_device *dev)
@@ -732,19 +739,20 @@ visorbus_disable_channel_interrupts(struct visor_device *dev)
 }
 EXPORT_SYMBOL_GPL(visorbus_disable_channel_interrupts);
 
-/** This is how everything starts from the device end.
- *  This function is called when a channel first appears via a ControlVM
- *  message.  In response, this function allocates a visor_device to
- *  correspond to the new channel, and attempts to connect it the appropriate
- *  driver.  If the appropriate driver is found, the visor_driver.probe()
- *  function for that driver will be called, and will be passed the new
- *  visor_device that we just created.
+/**
+ * This is how everything starts from the device end.
+ * This function is called when a channel first appears via a ControlVM
+ * message.  In response, this function allocates a visor_device to
+ * correspond to the new channel, and attempts to connect it the appropriate
+ * driver.  If the appropriate driver is found, the visor_driver.probe()
+ * function for that driver will be called, and will be passed the new
+ * visor_device that we just created.
  *
- *  It's ok if the appropriate driver is not yet loaded, because in that case
- *  the new device struct will just stick around in the bus' list of devices.
- *  When the appropriate driver calls visorbus_register_visor_driver(), the
- *  visor_driver.probe() for the new driver will be called with the new
- *  device.
+ * It's ok if the appropriate driver is not yet loaded, because in that case
+ * the new device struct will just stick around in the bus' list of devices.
+ * When the appropriate driver calls visorbus_register_visor_driver(), the
+ * visor_driver.probe() for the new driver will be called with the new
+ * device.
  */
 static int
 create_visor_device(struct visor_device *dev)
@@ -834,10 +842,10 @@ get_vbus_header_info(struct visorchannel *chan,
 	return 0;
 }
 
-/* Write the contents of <info> to the struct
+/**
+ * Write the contents of <info> to the struct
  * spar_vbus_channel_protocol.chp_info.
  */
-
 static int
 write_vbus_chp_info(struct visorchannel *chan,
 		    struct spar_vbus_headerinfo *hdr_info,
@@ -853,10 +861,10 @@ write_vbus_chp_info(struct visorchannel *chan,
 	return 0;
 }
 
-/* Write the contents of <info> to the struct
+/**
+ * Write the contents of <info> to the struct
  * spar_vbus_channel_protocol.bus_info.
  */
-
 static int
 write_vbus_bus_info(struct visorchannel *chan,
 		    struct spar_vbus_headerinfo *hdr_info,
@@ -872,7 +880,8 @@ write_vbus_bus_info(struct visorchannel *chan,
 	return 0;
 }
 
-/* Write the contents of <info> to the
+/**
+ * Write the contents of <info> to the
  * struct spar_vbus_channel_protocol.dev_info[<devix>].
  */
 static int
@@ -892,7 +901,8 @@ write_vbus_dev_info(struct visorchannel *chan,
 	return 0;
 }
 
-/* For a child device just created on a client bus, fill in
+/**
+ * For a child device just created on a client bus, fill in
  * information about the driver that is controlling this device into
  * the the appropriate slot within the vbus channel of the bus
  * instance.
@@ -949,7 +959,8 @@ fix_vbus_dev_info(struct visor_device *visordev)
 			    &clientbus_driverinfo);
 }
 
-/** Create a device instance for the visor bus itself.
+/**
+ * Create a device instance for the visor bus itself.
  */
 static int
 create_bus_instance(struct visor_device *dev)
@@ -990,7 +1001,8 @@ create_bus_instance(struct visor_device *dev)
 	return 0;
 }
 
-/** Remove a device instance for the visor bus itself.
+/**
+ * Remove a device instance for the visor bus itself.
  */
 static void
 remove_bus_instance(struct visor_device *dev)
@@ -1012,8 +1024,9 @@ remove_bus_instance(struct visor_device *dev)
 	device_unregister(&dev->device);
 }
 
-/** Create and register the one-and-only one instance of
- *  the visor bus type (visorbus_type).
+/**
+ * Create and register the one-and-only one instance of
+ * the visor bus type (visorbus_type).
  */
 static int
 create_bus_type(void)
@@ -1022,7 +1035,8 @@ create_bus_type(void)
 	return busreg_rc;
 }
 
-/** Remove the one-and-only one instance of the visor bus type (visorbus_type).
+/**
+ * Remove the one-and-only one instance of the visor bus type (visorbus_type).
  */
 static void
 remove_bus_type(void)
@@ -1030,7 +1044,8 @@ remove_bus_type(void)
 	bus_unregister(&visorbus_type);
 }
 
-/** Remove all child visor bus device instances.
+/**
+ * Remove all child visor bus device instances.
  */
 static void
 remove_all_visor_devices(void)
@@ -1105,7 +1120,8 @@ chipset_device_destroy(struct visor_device *dev_info)
 		(*chipset_responders.device_destroy) (dev_info, 0);
 }
 
-/* This is the callback function specified for a function driver, to
+/**
+ * This is the callback function specified for a function driver, to
  * be called when a pending "pause device" operation has been
  * completed.
  */
@@ -1126,7 +1142,8 @@ pause_state_change_complete(struct visor_device *dev, int status)
 	(*chipset_responders.device_pause) (dev, status);
 }
 
-/* This is the callback function specified for a function driver, to
+/**
+ * This is the callback function specified for a function driver, to
  * be called when a pending "resume device" operation has been
  * completed.
  */
@@ -1147,7 +1164,8 @@ resume_state_change_complete(struct visor_device *dev, int status)
 	(*chipset_responders.device_resume) (dev, status);
 }
 
-/* Tell the subordinate function driver for a specific device to pause
+/**
+ * Tell the subordinate function driver for a specific device to pause
  * or resume that device.  Result is returned asynchronously via a
  * callback function.
  */
-- 
1.9.1

[PATCH v2 19/27] staging: unisys: visorbus: make visorbus_private.h function descriptions more kerneldoc-like

[David Kershner]

From: David Binder <david.binder@unisys.com>

Per audit feedback from Thomas Gleixner, function descriptions in
visorbus_private.h now utilize a more kerneldoc-like formatting. The
affected comments do not implement other kerneldoc requirements.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_private.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_private.h b/drivers/staging/unisys/visorbus/visorbus_private.h
index 39edd20..f48f230 100644
--- a/drivers/staging/unisys/visorbus/visorbus_private.h
+++ b/drivers/staging/unisys/visorbus/visorbus_private.h
@@ -51,10 +51,11 @@ struct visorchipset_busdev_responders {
 	void (*device_resume)(struct visor_device *p, int response);
 };
 
-/** Register functions (in the bus driver) to get called by visorchipset
- *  whenever a bus or device appears for which this guest is to be the
- *  client for.  visorchipset will fill in <responders>, to indicate
- *  functions the bus driver should call to indicate message responses.
+/**
+ * Register functions (in the bus driver) to get called by visorchipset
+ * whenever a bus or device appears for which this guest is to be the
+ * client for.  visorchipset will fill in <responders>, to indicate
+ * functions the bus driver should call to indicate message responses.
  */
 void
 visorchipset_register_busdev(
-- 
1.9.1

[PATCH v2 20/27] staging: unisys: visorbus: make visorchannel function descriptions more kerneldoc-like

[David Kershner]

From: David Binder <david.binder@unisys.com>

Per audit feedback from Thomas Gleixner, function descriptions in
visorchannel.c now utilize a more kerneldoc-like formatting. The affected
comments do not implement other kerneldoc requirements.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorchannel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
index 43315c2..b9f0b6b 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -55,7 +55,8 @@ struct visorchannel {
 	uuid_le inst;
 };
 
-/* Creates the struct visorchannel abstraction for a data area in memory,
+/**
+ * Creates the struct visorchannel abstraction for a data area in memory,
  * but does NOT modify this data area.
  */
 static struct visorchannel *
-- 
1.9.1

[PATCH v2 21/27] staging: unisys: visorbus: make visorchipset function descriptions more kerneldoc-like

[David Kershner]

From: David Binder <david.binder@unisys.com>

Per audit feedback from Thomas Gleixner, function descriptions in
visorchipset.c now utilize a more kerneldoc-like formatting. The affected
comments do not implement other kerneldoc requirements.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 41 +++++++++++++++-----------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 1da980f..189ba40 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -1300,7 +1300,8 @@ my_device_destroy(struct controlvm_message *inmsg)
 			      inmsg->hdr.flags.response_expected == 1, 1);
 }
 
-/* When provided with the physical address of the controlvm channel
+/**
+ * When provided with the physical address of the controlvm channel
  * (phys_addr), the offset to the payload area we need to manage
  * (offset), and the size of this payload area (bytes), fills in the
  * controlvm_payload_info struct.  Returns true for success or false
@@ -1368,8 +1369,9 @@ initialize_controlvm_payload(void)
 					  &controlvm_payload_info);
 }
 
-/*  Send ACTION=online for DEVPATH=/sys/devices/platform/visorchipset.
- *  Returns CONTROLVM_RESP_xxx code.
+/**
+ * Send ACTION=online for DEVPATH=/sys/devices/platform/visorchipset.
+ * Returns CONTROLVM_RESP_xxx code.
  */
 static int
 visorchipset_chipset_ready(void)
@@ -1390,8 +1392,9 @@ visorchipset_chipset_selftest(void)
 	return CONTROLVM_RESP_SUCCESS;
 }
 
-/*  Send ACTION=offline for DEVPATH=/sys/devices/platform/visorchipset.
- *  Returns CONTROLVM_RESP_xxx code.
+/**
+ * Send ACTION=offline for DEVPATH=/sys/devices/platform/visorchipset.
+ * Returns CONTROLVM_RESP_xxx code.
  */
 static int
 visorchipset_chipset_notready(void)
@@ -1433,7 +1436,8 @@ chipset_notready(struct controlvm_message_header *msg_hdr)
 		controlvm_respond(msg_hdr, rc);
 }
 
-/* This is your "one-stop" shop for grabbing the next message from the
+/**
+ * This is your "one-stop" shop for grabbing the next message from the
  * CONTROLVM_QUEUE_EVENT queue in the controlvm channel.
  */
 static bool
@@ -1464,7 +1468,7 @@ read_controlvm_event(struct controlvm_message *msg)
 
 #define PARAHOTPLUG_TIMEOUT_MS 2000
 
-/*
+/**
  * Generate unique int to match an outstanding CONTROLVM message with a
  * udev script /proc response
  */
@@ -1476,7 +1480,7 @@ parahotplug_next_id(void)
 	return atomic_inc_return(&id);
 }
 
-/*
+/**
  * Returns the time (in jiffies) when a CONTROLVM message on the list
  * should expire -- PARAHOTPLUG_TIMEOUT_MS in the future
  */
@@ -1486,7 +1490,7 @@ parahotplug_next_expiration(void)
 	return jiffies + msecs_to_jiffies(PARAHOTPLUG_TIMEOUT_MS);
 }
 
-/*
+/**
  * Create a parahotplug_request, which is basically a wrapper for a
  * CONTROLVM_MESSAGE that we can stick on a list
  */
@@ -1506,7 +1510,7 @@ parahotplug_request_create(struct controlvm_message *msg)
 	return req;
 }
 
-/*
+/**
  * Free a parahotplug_request.
  */
 static void
@@ -1515,7 +1519,7 @@ parahotplug_request_destroy(struct parahotplug_request *req)
 	kfree(req);
 }
 
-/*
+/**
  * Cause uevent to run the user level script to do the disable/enable
  * specified in (the CONTROLVM message in) the specified
  * parahotplug_request
@@ -1545,7 +1549,7 @@ parahotplug_request_kickoff(struct parahotplug_request *req)
 			   envp);
 }
 
-/*
+/**
  * Remove any request from the list that's been on there too long and
  * respond with an error.
  */
@@ -1576,7 +1580,7 @@ parahotplug_process_list(void)
 	spin_unlock(&parahotplug_request_list_lock);
 }
 
-/*
+/**
  * Called from the /proc handler, which means the user script has
  * finished the enable/disable.  Find the matching identifier, and
  * respond to the CONTROLVM message with success.
@@ -1613,7 +1617,7 @@ parahotplug_request_complete(int id, u16 active)
 	return -EINVAL;
 }
 
-/*
+/**
  * Enables or disables a PCI device by kicking off a udev script
  */
 static void
@@ -1656,7 +1660,8 @@ parahotplug_process_message(struct controlvm_message *inmsg)
 	}
 }
 
-/* Process a controlvm message.
+/**
+ * Process a controlvm message.
  * Return result:
  *    false - this function will return false only in the case where the
  *            controlvm message was NOT processed, but processing must be
@@ -2024,7 +2029,8 @@ device_resume_response(struct visor_device *dev_info, int response)
 	dev_info->pending_msg_hdr = NULL;
 }
 
-/* The parahotplug/devicedisabled interface gets called by our support script
+/**
+ * The parahotplug/devicedisabled interface gets called by our support script
  * when an SR-IOV device has been shut down. The ID is passed to the script
  * and then passed back when the device has been removed.
  */
@@ -2041,7 +2047,8 @@ static ssize_t devicedisabled_store(struct device *dev,
 	return count;
 }
 
-/* The parahotplug/deviceenabled interface gets called by our support script
+/**
+ * The parahotplug/deviceenabled interface gets called by our support script
  * when an SR-IOV device has been recovered. The ID is passed to the script
  * and then passed back when the device has been brought back up.
  */
-- 
1.9.1

[PATCH v2 22/27] staging: unisys: visorbus: Move visorbus-unique functions to private header

[David Kershner]

From: David Binder <david.binder@unisys.com>

Moves function prototypes that are unique to visorbus from
include/visorbus.h to visorbus/visorbus_private.h.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h          | 25 --------------------
 drivers/staging/unisys/visorbus/visorbus_private.h | 27 ++++++++++++++++++++++
 drivers/staging/unisys/visorbus/visorchannel.c     |  1 +
 3 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 94dd48e..2c103e8 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -188,37 +188,12 @@ int visorbus_write_channel(struct visor_device *dev,
 void visorbus_enable_channel_interrupts(struct visor_device *dev);
 void visorbus_disable_channel_interrupts(struct visor_device *dev);
 
-/* Note that for visorchannel_create()
- * <channel_bytes> and <guid> arguments may be 0 if we are a channel CLIENT.
- * In this case, the values can simply be read from the channel header.
- */
-struct visorchannel *visorchannel_create(u64 physaddr,
-					 unsigned long channel_bytes,
-					 gfp_t gfp, uuid_le guid);
-struct visorchannel *visorchannel_create_with_lock(u64 physaddr,
-						   unsigned long channel_bytes,
-						   gfp_t gfp, uuid_le guid);
-void visorchannel_destroy(struct visorchannel *channel);
-int visorchannel_read(struct visorchannel *channel, ulong offset,
-		      void *local, ulong nbytes);
-int visorchannel_write(struct visorchannel *channel, ulong offset,
-		       void *local, ulong nbytes);
 bool visorchannel_signalremove(struct visorchannel *channel, u32 queue,
 			       void *msg);
 bool visorchannel_signalinsert(struct visorchannel *channel, u32 queue,
 			       void *msg);
 bool visorchannel_signalempty(struct visorchannel *channel, u32 queue);
-
-u64 visorchannel_get_physaddr(struct visorchannel *channel);
-ulong visorchannel_get_nbytes(struct visorchannel *channel);
-char *visorchannel_id(struct visorchannel *channel, char *s);
-char *visorchannel_zoneid(struct visorchannel *channel, char *s);
-u64 visorchannel_get_clientpartition(struct visorchannel *channel);
-int visorchannel_set_clientpartition(struct visorchannel *channel,
-				     u64 partition_handle);
 uuid_le visorchannel_get_uuid(struct visorchannel *channel);
-char *visorchannel_uuid_id(uuid_le *guid, char *s);
-void __iomem *visorchannel_get_header(struct visorchannel *channel);
 
 #define BUS_ROOT_DEVICE		UINT_MAX
 struct visor_device *visorbus_get_device_by_id(u32 bus_no, u32 dev_no,
diff --git a/drivers/staging/unisys/visorbus/visorbus_private.h b/drivers/staging/unisys/visorbus/visorbus_private.h
index f48f230..0ee7904 100644
--- a/drivers/staging/unisys/visorbus/visorbus_private.h
+++ b/drivers/staging/unisys/visorbus/visorbus_private.h
@@ -66,4 +66,31 @@ visorchipset_register_busdev(
 /* visorbus init and exit functions */
 int visorbus_init(void);
 void visorbus_exit(void);
+
+/* Visorchannel access functions */
+
+/* Note that for visorchannel_create()
+ * <channel_bytes> and <guid> arguments may be 0 if we are a channel CLIENT.
+ * In this case, the values can simply be read from the channel header.
+ */
+struct visorchannel *visorchannel_create(u64 physaddr,
+					 unsigned long channel_bytes,
+					 gfp_t gfp, uuid_le guid);
+struct visorchannel *visorchannel_create_with_lock(u64 physaddr,
+						   unsigned long channel_bytes,
+						   gfp_t gfp, uuid_le guid);
+void visorchannel_destroy(struct visorchannel *channel);
+int visorchannel_read(struct visorchannel *channel, ulong offset,
+		      void *local, ulong nbytes);
+int visorchannel_write(struct visorchannel *channel, ulong offset,
+		       void *local, ulong nbytes);
+u64 visorchannel_get_physaddr(struct visorchannel *channel);
+ulong visorchannel_get_nbytes(struct visorchannel *channel);
+char *visorchannel_id(struct visorchannel *channel, char *s);
+char *visorchannel_zoneid(struct visorchannel *channel, char *s);
+u64 visorchannel_get_clientpartition(struct visorchannel *channel);
+int visorchannel_set_clientpartition(struct visorchannel *channel,
+				     u64 partition_handle);
+char *visorchannel_uuid_id(uuid_le *guid, char *s);
+void __iomem *visorchannel_get_header(struct visorchannel *channel);
 #endif
diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
index b9f0b6b..1b743d7 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -25,6 +25,7 @@
 #include "version.h"
 #include "visorbus.h"
 #include "controlvmchannel.h"
+#include "visorbus_private.h"
 
 #define MYDRVNAME "visorchannel"
 
-- 
1.9.1

[PATCH v2 23/27] staging: unisys: visorbus: Add kerneldoc-style comments for visorbus API

[David Kershner]

From: David Binder <david.binder@unisys.com>

Adds kerneldoc-style comments for those functions which may be used outside
of the visorbus driver.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h       | 127 ++++++++++++++++++++++++
 drivers/staging/unisys/visorbus/visorbus_main.c |  42 --------
 2 files changed, 127 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index 2c103e8..99c7beb 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -177,22 +177,149 @@ struct visor_device {
 
 #define to_visor_device(x) container_of(x, struct visor_device, device)
 
+/**
+ * visorbus_register_visor_driver() - registers the provided driver
+ * @struct visor_driver *: the driver to register
+ *
+ * A particular type of visor driver calls this function to register
+ * the driver.  The caller MUST fill in the following fields within the
+ * #drv structure:
+ *     name, version, owner, channel_types, probe, remove
+ *
+ * Here's how the whole Linux bus / driver / device model works.
+ *
+ * At system start-up, the visorbus kernel module is loaded, which registers
+ * visorbus_type as a bus type, using bus_register().
+ *
+ * All kernel modules that support particular device types on a
+ * visorbus bus are loaded.  Each of these kernel modules calls
+ * visorbus_register_visor_driver() in their init functions, passing a
+ * visor_driver struct.  visorbus_register_visor_driver() in turn calls
+ * register_driver(&visor_driver.driver).  This .driver member is
+ * initialized with generic methods (like probe), whose sole responsibility
+ * is to act as a broker for the real methods, which are within the
+ * visor_driver struct.  (This is the way the subclass behavior is
+ * implemented, since visor_driver is essentially a subclass of the
+ * generic driver.)  Whenever a driver_register() happens, core bus code in
+ * the kernel does (see device_attach() in drivers/base/dd.c):
+ *
+ *     for each dev associated with the bus (the bus that driver is on) that
+ *     does not yet have a driver
+ *         if bus.match(dev,newdriver) == yes_matched  ** .match specified
+ *                                                ** during bus_register().
+ *             newdriver.probe(dev)  ** for visor drivers, this will call
+ *                   ** the generic driver.probe implemented in visorbus.c,
+ *                   ** which in turn calls the probe specified within the
+ *                   ** struct visor_driver (which was specified by the
+ *                   ** actual device driver as part of
+ *                   ** visorbus_register_visor_driver()).
+ *
+ * The above dance also happens when a new device appears.
+ * So the question is, how are devices created within the system?
+ * Basically, just call device_add(dev).  See pci_bus_add_devices().
+ * pci_scan_device() shows an example of how to build a device struct.  It
+ * returns the newly-created struct to pci_scan_single_device(), who adds it
+ * to the list of devices at PCIBUS.devices.  That list of devices is what
+ * is traversed by pci_bus_add_devices().
+ *
+ * Return: integer indicating success (zero) or failure (non-zero)
+ */
 int visorbus_register_visor_driver(struct visor_driver *);
+
+/**
+ * visorbus_unregister_visor_driver() - unregisters the provided driver
+ * @struct visor_driver *: the driver to unregister
+ */
 void visorbus_unregister_visor_driver(struct visor_driver *);
+
+/**
+ * visorbus_read_channel() - reads from the designated channel into
+ *                           the provided buffer
+ * @dev:    the device whose channel is read from
+ * @offset: the offset into the channel at which reading starts
+ * @dest:   the destination buffer that is written into from the channel
+ * @nbytes: the number of bytes to read from the channel
+ *
+ * If receiving a message, use the visorchannel_signalremove()
+ * function instead.
+ *
+ * Return: integer indicating success (zero) or failure (non-zero)
+ */
 int visorbus_read_channel(struct visor_device *dev,
 			  unsigned long offset, void *dest,
 			  unsigned long nbytes);
+
+/**
+ * visorbus_write_channel() - writes the provided buffer into the designated
+ *                            channel
+ * @dev:    the device whose channel is written to
+ * @offset: the offset into the channel at which writing starts
+ * @src:    the source buffer that is written into the channel
+ * @nbytes: the number of bytes to write into the channel
+ *
+ * If sending a message, use the visorchannel_signalinsert()
+ * function instead.
+ *
+ * Return: integer indicating success (zero) or failure (non-zero)
+ */
 int visorbus_write_channel(struct visor_device *dev,
 			   unsigned long offset, void *src,
 			   unsigned long nbytes);
+/**
+ * visorbus_enable_channel_interrupts() - enables interrupts on the
+ *                                        designated device
+ * @dev: the device on which to enable interrupts
+ */
 void visorbus_enable_channel_interrupts(struct visor_device *dev);
+
+/**
+ * visorbus_disable_channel_interrupts() - disables interrupts on the
+ *                                         designated device
+ * @dev: the device on which to disable interrupts
+ */
 void visorbus_disable_channel_interrupts(struct visor_device *dev);
 
+/**
+ * visorchannel_signalremove() - removes a message from the designated
+ *                               channel/queue
+ * @channel: the channel the message will be removed from
+ * @queue:   the queue the message will be removed from
+ * @msg:     the message to remove
+ *
+ * Return: boolean indicating whether the removal succeeded or failed
+ */
 bool visorchannel_signalremove(struct visorchannel *channel, u32 queue,
 			       void *msg);
+
+/**
+ * visorchannel_signalinsert() - inserts a message into the designated
+ *                               channel/queue
+ * @channel: the channel the message will be added to
+ * @queue:   the queue the message will be added to
+ * @msg:     the message to insert
+ *
+ * Return: boolean indicating whether the insertion succeeded or failed
+ */
 bool visorchannel_signalinsert(struct visorchannel *channel, u32 queue,
 			       void *msg);
+
+/**
+ * visorchannel_signalempty() - checks if the designated channel/queue
+ *                              contains any messages
+ * @channel: the channel to query
+ * @queue:   the queue in the channel to query
+ *
+ * Return: boolean indicating whether any messages in the designated
+ *         channel/queue are present
+ */
 bool visorchannel_signalempty(struct visorchannel *channel, u32 queue);
+
+/**
+ * visorchannel_get_uuid() - queries the UUID of the designated channel
+ * @channel: the channel to query
+ *
+ * Return: the UUID of the provided channel
+ */
 uuid_le visorchannel_get_uuid(struct visorchannel *channel);
 
 #define BUS_ROOT_DEVICE		UINT_MAX
diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 850998e..0a537c7 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -616,48 +616,6 @@ visordriver_remove_device(struct device *xdev)
 	return 0;
 }
 
-/**
- * A particular type of visor driver calls this function to register
- * the driver.  The caller MUST fill in the following fields within the
- * #drv structure:
- *     name, version, owner, channel_types, probe, remove
- *
- * Here's how the whole Linux bus / driver / device model works.
- *
- * At system start-up, the visorbus kernel module is loaded, which registers
- * visorbus_type as a bus type, using bus_register().
- *
- * All kernel modules that support particular device types on a
- * visorbus bus are loaded.  Each of these kernel modules calls
- * visorbus_register_visor_driver() in their init functions, passing a
- * visor_driver struct.  visorbus_register_visor_driver() in turn calls
- * register_driver(&visor_driver.driver).  This .driver member is
- * initialized with generic methods (like probe), whose sole responsibility
- * is to act as a broker for the real methods, which are within the
- * visor_driver struct.  (This is the way the subclass behavior is
- * implemented, since visor_driver is essentially a subclass of the
- * generic driver.)  Whenever a driver_register() happens, core bus code in
- * the kernel does (see device_attach() in drivers/base/dd.c):
- *
- *     for each dev associated with the bus (the bus that driver is on) that
- *     does not yet have a driver
- *         if bus.match(dev,newdriver) == yes_matched  ** .match specified
- *                                                ** during bus_register().
- *             newdriver.probe(dev)  ** for visor drivers, this will call
- *                   ** the generic driver.probe implemented in visorbus.c,
- *                   ** which in turn calls the probe specified within the
- *                   ** struct visor_driver (which was specified by the
- *                   ** actual device driver as part of
- *                   ** visorbus_register_visor_driver()).
- *
- * The above dance also happens when a new device appears.
- * So the question is, how are devices created within the system?
- * Basically, just call device_add(dev).  See pci_bus_add_devices().
- * pci_scan_device() shows an example of how to build a device struct.  It
- * returns the newly-created struct to pci_scan_single_device(), who adds it
- * to the list of devices at PCIBUS.devices.  That list of devices is what
- * is traversed by pci_bus_add_devices().
- */
 int visorbus_register_visor_driver(struct visor_driver *drv)
 {
 	int rc = 0;
-- 
1.9.1

[PATCH v2 24/27] staging: unisys: Move vbushelper.h to visorbus directory

[David Kershner]

Only visorbus needs this header file so move it to visorbus
directory.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/{include => visorbus}/vbushelper.h | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename drivers/staging/unisys/{include => visorbus}/vbushelper.h (100%)

diff --git a/drivers/staging/unisys/include/vbushelper.h b/drivers/staging/unisys/visorbus/vbushelper.h
similarity index 100%
rename from drivers/staging/unisys/include/vbushelper.h
rename to drivers/staging/unisys/visorbus/vbushelper.h
-- 
1.9.1

[PATCH v2 25/27] include: linux: visorbus: Add visorbus to include/linux directory

[David Kershner]

Update include/linux to include the s-Par associated common include
header files needed for the s-Par visorbus.

Since we have now moved the include directories over to
include/linux/visorbus this patch makes all of the visor
drivers visorbus, visorinput, visornic, and visorhba use
the new include folders.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/MAINTAINERS                                | 2 +-
 drivers/staging/unisys/visorbus/Makefile                          | 2 --
 drivers/staging/unisys/visorbus/controlvmchannel.h                | 2 +-
 drivers/staging/unisys/visorbus/vbuschannel.h                     | 3 ++-
 drivers/staging/unisys/visorbus/visorbus_main.c                   | 6 +++---
 drivers/staging/unisys/visorbus/visorchannel.c                    | 4 ++--
 drivers/staging/unisys/visorbus/visorchipset.c                    | 8 ++++----
 drivers/staging/unisys/visorbus/vmcallinterface.h                 | 5 ++---
 drivers/staging/unisys/visorhba/Makefile                          | 2 --
 drivers/staging/unisys/visorhba/visorhba_main.c                   | 5 ++---
 drivers/staging/unisys/visorinput/Makefile                        | 2 --
 drivers/staging/unisys/visorinput/visorinput.c                    | 6 +++---
 drivers/staging/unisys/visornic/Makefile                          | 2 --
 drivers/staging/unisys/visornic/visornic_main.c                   | 5 ++---
 .../staging/unisys/include => include/linux/visorbus}/channel.h   | 0
 .../unisys/include => include/linux/visorbus}/channel_guid.h      | 0
 .../unisys/include => include/linux/visorbus}/diagchannel.h       | 0
 .../unisys/include => include/linux/visorbus}/guestlinuxdebug.h   | 0
 .../staging/unisys/include => include/linux/visorbus}/iochannel.h | 0
 .../staging/unisys/include => include/linux/visorbus}/version.h   | 0
 .../staging/unisys/include => include/linux/visorbus}/visorbus.h  | 0
 21 files changed, 22 insertions(+), 32 deletions(-)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/channel.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/channel_guid.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/diagchannel.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/guestlinuxdebug.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/iochannel.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/version.h (100%)
 rename {drivers/staging/unisys/include => include/linux/visorbus}/visorbus.h (100%)

diff --git a/drivers/staging/unisys/MAINTAINERS b/drivers/staging/unisys/MAINTAINERS
index 1f0425b..146a8c3 100644
--- a/drivers/staging/unisys/MAINTAINERS
+++ b/drivers/staging/unisys/MAINTAINERS
@@ -1,5 +1,5 @@
 Unisys s-Par drivers
 M:	David Kershner <sparmaintainer@unisys.com>
 S:	Maintained
-F:	Documentation/s-Par/overview.txt
+F:	Documentation/visorbus.txt
 F:	drivers/staging/unisys/
diff --git a/drivers/staging/unisys/visorbus/Makefile b/drivers/staging/unisys/visorbus/Makefile
index f3730d8..7f328cc 100644
--- a/drivers/staging/unisys/visorbus/Makefile
+++ b/drivers/staging/unisys/visorbus/Makefile
@@ -7,5 +7,3 @@ obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus.o
 visorbus-y := visorbus_main.o
 visorbus-y += visorchannel.o
 visorbus-y += visorchipset.o
-
-ccflags-y += -Idrivers/staging/unisys/include
diff --git a/drivers/staging/unisys/visorbus/controlvmchannel.h b/drivers/staging/unisys/visorbus/controlvmchannel.h
index 03e36fb..0a0e221 100644
--- a/drivers/staging/unisys/visorbus/controlvmchannel.h
+++ b/drivers/staging/unisys/visorbus/controlvmchannel.h
@@ -16,7 +16,7 @@
 #define __CONTROLVMCHANNEL_H__
 
 #include <linux/uuid.h>
-#include "channel.h"
+#include <linux/visorbus/channel.h>
 
 /* {2B3C2D10-7EF5-4ad8-B966-3448B7386B3D} */
 #define SPAR_CONTROLVM_CHANNEL_PROTOCOL_UUID	\
diff --git a/drivers/staging/unisys/visorbus/vbuschannel.h b/drivers/staging/unisys/visorbus/vbuschannel.h
index 90fa12e..3e0388d 100644
--- a/drivers/staging/unisys/visorbus/vbuschannel.h
+++ b/drivers/staging/unisys/visorbus/vbuschannel.h
@@ -23,8 +23,9 @@
  *  the client devices and client drivers for the server end to see.
  */
 #include <linux/uuid.h>
+#include <linux/visorbus/channel.h>
+
 #include "vbusdeviceinfo.h"
-#include "channel.h"
 
 /* {193b331b-c58f-11da-95a9-00e08161165f} */
 #define SPAR_VBUS_CHANNEL_PROTOCOL_UUID \
diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 0a537c7..ac480fb 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -16,11 +16,11 @@
 
 #include <linux/uuid.h>
 
-#include "visorbus.h"
+#include <linux/visorbus/visorbus.h>
+#include <linux/visorbus/version.h>
+#include <linux/visorbus/guestlinuxdebug.h>
 #include "visorbus_private.h"
-#include "version.h"
 #include "vbuschannel.h"
-#include "guestlinuxdebug.h"
 #include "vmcallinterface.h"
 
 #define MYDRVNAME "visorbus"
diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
index 1b743d7..40c484a 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -22,8 +22,8 @@
 #include <linux/uuid.h>
 #include <linux/io.h>
 
-#include "version.h"
-#include "visorbus.h"
+#include <linux/visorbus/version.h>
+#include <linux/visorbus/visorbus.h>
 #include "controlvmchannel.h"
 #include "visorbus_private.h"
 
diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 189ba40..c0b7e6c 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -24,13 +24,13 @@
 #include <linux/platform_device.h>
 #include <linux/uuid.h>
 #include <linux/crash_dump.h>
+#include <linux/visorbus/channel_guid.h>
+#include <linux/visorbus/guestlinuxdebug.h>
+#include <linux/visorbus/version.h>
+#include <linux/visorbus/visorbus.h>
 
-#include "channel_guid.h"
 #include "controlvmchannel.h"
 #include "controlvmcompletionstatus.h"
-#include "guestlinuxdebug.h"
-#include "version.h"
-#include "visorbus.h"
 #include "visorbus_private.h"
 #include "vmcallinterface.h"
 
diff --git a/drivers/staging/unisys/visorbus/vmcallinterface.h b/drivers/staging/unisys/visorbus/vmcallinterface.h
index c043fa4..aac7000 100644
--- a/drivers/staging/unisys/visorbus/vmcallinterface.h
+++ b/drivers/staging/unisys/visorbus/vmcallinterface.h
@@ -21,10 +21,9 @@
 * running on IO Partitions.
 */
 
-#ifdef __GNUC__
+#include <linux/visorbus/diagchannel.h>
+
 #include "iovmcall_gnuc.h"
-#endif	/*  */
-#include "diagchannel.h"
 
 #ifdef VMCALL_IO_CONTROLVM_ADDR
 #undef VMCALL_IO_CONTROLVM_ADDR
diff --git a/drivers/staging/unisys/visorhba/Makefile b/drivers/staging/unisys/visorhba/Makefile
index a8a8e0e..e65b2be 100644
--- a/drivers/staging/unisys/visorhba/Makefile
+++ b/drivers/staging/unisys/visorhba/Makefile
@@ -6,5 +6,3 @@ obj-$(CONFIG_UNISYS_VISORHBA)	+= visorhba.o
 
 visorhba-y := visorhba_main.o
 
-ccflags-y += -Idrivers/staging/unisys/include
-
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 6a4570d..6e280e0 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -20,9 +20,8 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
-
-#include "visorbus.h"
-#include "iochannel.h"
+#include <linux/visorbus/visorbus.h>
+#include <linux/visorbus/iochannel.h>
 
 /* The Send and Receive Buffers of the IO Queue may both be full */
 
diff --git a/drivers/staging/unisys/visorinput/Makefile b/drivers/staging/unisys/visorinput/Makefile
index beedca7..87426a0 100644
--- a/drivers/staging/unisys/visorinput/Makefile
+++ b/drivers/staging/unisys/visorinput/Makefile
@@ -3,5 +3,3 @@
 #
 
 obj-$(CONFIG_UNISYS_VISORINPUT)	+= visorinput.o
-
-ccflags-y += -Idrivers/staging/unisys/include
diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
index 9c00710..2b12d23 100644
--- a/drivers/staging/unisys/visorinput/visorinput.c
+++ b/drivers/staging/unisys/visorinput/visorinput.c
@@ -27,11 +27,11 @@
 #include <linux/input.h>
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
+#include <linux/visorbus/version.h>
+#include <linux/visorbus/visorbus.h>
+#include <linux/visorbus/channel.h>
 #include <linux/uuid.h>
 
-#include "version.h"
-#include "visorbus.h"
-#include "channel.h"
 #include "ultrainputreport.h"
 
 /* Keyboard channel {c73416d0-b0b8-44af-b304-9d2ae99f1b3d} */
diff --git a/drivers/staging/unisys/visornic/Makefile b/drivers/staging/unisys/visornic/Makefile
index 439e95e..43985bb 100644
--- a/drivers/staging/unisys/visornic/Makefile
+++ b/drivers/staging/unisys/visornic/Makefile
@@ -6,5 +6,3 @@ obj-$(CONFIG_UNISYS_VISORNIC)	+= visornic.o
 
 visornic-y := visornic_main.o
 
-ccflags-y += -Idrivers/staging/unisys/include
-
diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 9e5b258..b971be3 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -24,9 +24,8 @@
 #include <linux/kthread.h>
 #include <linux/skbuff.h>
 #include <linux/rtnetlink.h>
-
-#include "visorbus.h"
-#include "iochannel.h"
+#include <linux/visorbus/visorbus.h>
+#include <linux/visorbus/iochannel.h>
 
 #define VISORNIC_INFINITE_RSP_WAIT 0
 #define VISORNICSOPENMAX 32
diff --git a/drivers/staging/unisys/include/channel.h b/include/linux/visorbus/channel.h
similarity index 100%
rename from drivers/staging/unisys/include/channel.h
rename to include/linux/visorbus/channel.h
diff --git a/drivers/staging/unisys/include/channel_guid.h b/include/linux/visorbus/channel_guid.h
similarity index 100%
rename from drivers/staging/unisys/include/channel_guid.h
rename to include/linux/visorbus/channel_guid.h
diff --git a/drivers/staging/unisys/include/diagchannel.h b/include/linux/visorbus/diagchannel.h
similarity index 100%
rename from drivers/staging/unisys/include/diagchannel.h
rename to include/linux/visorbus/diagchannel.h
diff --git a/drivers/staging/unisys/include/guestlinuxdebug.h b/include/linux/visorbus/guestlinuxdebug.h
similarity index 100%
rename from drivers/staging/unisys/include/guestlinuxdebug.h
rename to include/linux/visorbus/guestlinuxdebug.h
diff --git a/drivers/staging/unisys/include/iochannel.h b/include/linux/visorbus/iochannel.h
similarity index 100%
rename from drivers/staging/unisys/include/iochannel.h
rename to include/linux/visorbus/iochannel.h
diff --git a/drivers/staging/unisys/include/version.h b/include/linux/visorbus/version.h
similarity index 100%
rename from drivers/staging/unisys/include/version.h
rename to include/linux/visorbus/version.h
diff --git a/drivers/staging/unisys/include/visorbus.h b/include/linux/visorbus/visorbus.h
similarity index 100%
rename from drivers/staging/unisys/include/visorbus.h
rename to include/linux/visorbus/visorbus.h
-- 
1.9.1

[PATCH v2 26/27] Documentation: Move visorbus documentation from staging to Documentation/

[David Kershner]

This patch simple does a git mv of the
drivers/staging/unisys/Documentation directory to Documentation. Renames
overview.txt to visorbus.txt and renames sysfs-platform-visorchipset to
the correct name sysfs-bus-visorbus.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 .../ABI/stable/sysfs-bus-visorbus                                         | 0
 .../unisys/Documentation/overview.txt => Documentation/visorbus.txt       | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset => Documentation/ABI/stable/sysfs-bus-visorbus (100%)
 rename drivers/staging/unisys/Documentation/overview.txt => Documentation/visorbus.txt (100%)

diff --git a/drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset b/Documentation/ABI/stable/sysfs-bus-visorbus
similarity index 100%
rename from drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset
rename to Documentation/ABI/stable/sysfs-bus-visorbus
diff --git a/drivers/staging/unisys/Documentation/overview.txt b/Documentation/visorbus.txt
similarity index 100%
rename from drivers/staging/unisys/Documentation/overview.txt
rename to Documentation/visorbus.txt
-- 
1.9.1

[PATCH v2 27/27] drivers: Add visorbus to the drivers directory

[David Kershner]

visorbus is currently located at drivers/staging/visorbus,
this patch moves it to drivers/virt.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
---
 drivers/staging/unisys/Kconfig                                        | 3 +--
 drivers/staging/unisys/Makefile                                       | 1 -
 drivers/virt/Kconfig                                                  | 2 ++
 drivers/virt/Makefile                                                 | 1 +
 drivers/{staging/unisys => virt}/visorbus/Kconfig                     | 0
 drivers/{staging/unisys => virt}/visorbus/Makefile                    | 0
 drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h          | 0
 drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h | 0
 drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h             | 0
 drivers/{staging/unisys => virt}/visorbus/vbuschannel.h               | 0
 drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h            | 0
 drivers/{staging/unisys => virt}/visorbus/vbushelper.h                | 0
 drivers/{staging/unisys => virt}/visorbus/visorbus_main.c             | 0
 drivers/{staging/unisys => virt}/visorbus/visorbus_private.h          | 0
 drivers/{staging/unisys => virt}/visorbus/visorchannel.c              | 0
 drivers/{staging/unisys => virt}/visorbus/visorchipset.c              | 0
 drivers/{staging/unisys => virt}/visorbus/vmcallinterface.h           | 0
 17 files changed, 4 insertions(+), 3 deletions(-)
 rename drivers/{staging/unisys => virt}/visorbus/Kconfig (100%)
 rename drivers/{staging/unisys => virt}/visorbus/Makefile (100%)
 rename drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vbuschannel.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vbushelper.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorbus_main.c (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorbus_private.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorchannel.c (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorchipset.c (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vmcallinterface.h (100%)

diff --git a/drivers/staging/unisys/Kconfig b/drivers/staging/unisys/Kconfig
index 4f1f5e6..dab09a9 100644
--- a/drivers/staging/unisys/Kconfig
+++ b/drivers/staging/unisys/Kconfig
@@ -3,7 +3,7 @@
 #
 menuconfig UNISYSSPAR
 	bool "Unisys SPAR driver support"
-	depends on X86_64 && !UML
+	depends on X86_64 && !UML && VIRT_DRIVERS
 	select PCI
 	select ACPI
 	---help---
@@ -11,7 +11,6 @@ menuconfig UNISYSSPAR
 
 if UNISYSSPAR
 
-source "drivers/staging/unisys/visorbus/Kconfig"
 source "drivers/staging/unisys/visornic/Kconfig"
 source "drivers/staging/unisys/visorinput/Kconfig"
 source "drivers/staging/unisys/visorhba/Kconfig"
diff --git a/drivers/staging/unisys/Makefile b/drivers/staging/unisys/Makefile
index 20eb098..e45f44b 100644
--- a/drivers/staging/unisys/Makefile
+++ b/drivers/staging/unisys/Makefile
@@ -1,7 +1,6 @@
 #
 # Makefile for Unisys SPAR drivers
 #
-obj-$(CONFIG_UNISYS_VISORBUS)		+= visorbus/
 obj-$(CONFIG_UNISYS_VISORNIC)		+= visornic/
 obj-$(CONFIG_UNISYS_VISORINPUT)		+= visorinput/
 obj-$(CONFIG_UNISYS_VISORHBA)		+= visorhba/
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 99ebdde..0c60896 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -30,4 +30,6 @@ config FSL_HV_MANAGER
           4) A kernel interface for receiving callbacks when a managed
 	     partition shuts down.
 
+source "drivers/virt/visorbus/Kconfig"
 endif
+
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index c47f04d..44aebd2 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_FSL_HV_MANAGER)	+= fsl_hypervisor.o
+obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus/
diff --git a/drivers/staging/unisys/visorbus/Kconfig b/drivers/virt/visorbus/Kconfig
similarity index 100%
rename from drivers/staging/unisys/visorbus/Kconfig
rename to drivers/virt/visorbus/Kconfig
diff --git a/drivers/staging/unisys/visorbus/Makefile b/drivers/virt/visorbus/Makefile
similarity index 100%
rename from drivers/staging/unisys/visorbus/Makefile
rename to drivers/virt/visorbus/Makefile
diff --git a/drivers/staging/unisys/visorbus/controlvmchannel.h b/drivers/virt/visorbus/controlvmchannel.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/controlvmchannel.h
rename to drivers/virt/visorbus/controlvmchannel.h
diff --git a/drivers/staging/unisys/visorbus/controlvmcompletionstatus.h b/drivers/virt/visorbus/controlvmcompletionstatus.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/controlvmcompletionstatus.h
rename to drivers/virt/visorbus/controlvmcompletionstatus.h
diff --git a/drivers/staging/unisys/visorbus/iovmcall_gnuc.h b/drivers/virt/visorbus/iovmcall_gnuc.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/iovmcall_gnuc.h
rename to drivers/virt/visorbus/iovmcall_gnuc.h
diff --git a/drivers/staging/unisys/visorbus/vbuschannel.h b/drivers/virt/visorbus/vbuschannel.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/vbuschannel.h
rename to drivers/virt/visorbus/vbuschannel.h
diff --git a/drivers/staging/unisys/visorbus/vbusdeviceinfo.h b/drivers/virt/visorbus/vbusdeviceinfo.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/vbusdeviceinfo.h
rename to drivers/virt/visorbus/vbusdeviceinfo.h
diff --git a/drivers/staging/unisys/visorbus/vbushelper.h b/drivers/virt/visorbus/vbushelper.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/vbushelper.h
rename to drivers/virt/visorbus/vbushelper.h
diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/virt/visorbus/visorbus_main.c
similarity index 100%
rename from drivers/staging/unisys/visorbus/visorbus_main.c
rename to drivers/virt/visorbus/visorbus_main.c
diff --git a/drivers/staging/unisys/visorbus/visorbus_private.h b/drivers/virt/visorbus/visorbus_private.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/visorbus_private.h
rename to drivers/virt/visorbus/visorbus_private.h
diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/virt/visorbus/visorchannel.c
similarity index 100%
rename from drivers/staging/unisys/visorbus/visorchannel.c
rename to drivers/virt/visorbus/visorchannel.c
diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/virt/visorbus/visorchipset.c
similarity index 100%
rename from drivers/staging/unisys/visorbus/visorchipset.c
rename to drivers/virt/visorbus/visorchipset.c
diff --git a/drivers/staging/unisys/visorbus/vmcallinterface.h b/drivers/virt/visorbus/vmcallinterface.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/vmcallinterface.h
rename to drivers/virt/visorbus/vmcallinterface.h
-- 
1.9.1

Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Thomas Gleixner]

On Tue, 31 May 2016, David Kershner wrote:
> +	/*
> +	 * If we're not paused, really enable interrupts.
> +	 * Regardless of whether we are paused, set a flag indicating
> +	 * interrupts should be enabled so when we resume, interrupts
> +	 * will really be enabled.
> +	 */
> +	down_write(&devdata->lock_visor_dev);

Why is this a rw_semaphore? It's only ever taken with down_write() and it's
always the same context. Should be a mutex, right?

While at it, please convert the notifier_lock to a mutex as well.

Thanks,

	tglx

Re: [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel function descriptions more kerneldoc-like

[Thomas Gleixner]

On Tue, 31 May 2016, David Kershner wrote:

> From: David Binder <david.binder@unisys.com>
> 
> Per audit feedback from Thomas Gleixner, function descriptions in
> visorchannel.c now utilize a more kerneldoc-like formatting. The affected
> comments do not implement other kerneldoc requirements.

That was not my feedback. If you start a comment with '/**' then it should be
a kernel-doc function/struct documentation. If not then it starts with '/*'.
 
Thanks,

	tglx

Re: [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock a mutex

[Thomas Gleixner]

On Tue, 31 May 2016, David Kershner wrote:

> From: Bryan Thompson <bryan.thompson@unisys.com>
> 
> visordriver_callback_lock is just a binary semaphore that logically
> makes more sense as a mutex.
> 
> Signed-off-by: Bryan Thompson <bryan.thompson@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
> ---
>  drivers/staging/unisys/include/visorbus.h       |  3 ++-
>  drivers/staging/unisys/visorbus/visorbus_main.c | 10 +++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
> index 9bb88bb..9da25c0 100644
> --- a/drivers/staging/unisys/include/visorbus.h
> +++ b/drivers/staging/unisys/include/visorbus.h
> @@ -161,7 +161,8 @@ struct visor_device {
>  	struct timer_list timer;
>  	bool timer_active;
>  	bool being_removed;
> -	struct semaphore visordriver_callback_lock;
> +	/* mutex to serialize visor_driver function callbacks */

TBH. I hate these kind of comments. The mutex name is self explaining, right?
I rather wish you would have spent time documenting the non obvious parts of
the code.

Thanks,

	tglx

Re: [PATCH v2 02/27] staging: unisys: visorchipset change -1 return value

[Neil Horman]

On Tue, May 31, 2016 at 10:26:28PM -0400, David Kershner wrote:
> From: Erik Arfvidson <erik.arfvidson@unisys.com>
> 
> This patch changes the vague -1 return value to -EINVAL
> 
> Signed-off-by: Erik Arfvidson <erik.arfvidson@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
> index 5ba5936..d248c94 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1613,7 +1613,7 @@ parahotplug_request_complete(int id, u16 active)
>  	}
>  
>  	spin_unlock(&parahotplug_request_list_lock);
> -	return -1;
> +	return -EINVAL;
>  }
>  
>  /*
> -- 
> 1.9.1
> 
Why return anything here, every caller of this function ignores the return code
entirely.  Alternatively, passing the EINVAL back from the caller seems
reasonable.

Neil

Re: [PATCH v2 01/27] staging: unisys: visorbus change -1 return values

[Neil Horman]

On Tue, May 31, 2016 at 10:26:27PM -0400, David Kershner wrote:
> From: Erik Arfvidson <erik.arfvidson@unisys.com>
> 
> This patch changes the vague -1 return values to -EFAULT since
> it would be the most appropriate, given that this error
> would only occur in an unexpected bad offset field.
> Resulting in a bad address.
> 
> Signed-off-by: Erik Arfvidson <erik.arfvidson@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 3a147db..d32b898 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -876,10 +876,10 @@ write_vbus_chp_info(struct visorchannel *chan,
>  	int off = sizeof(struct channel_header) + hdr_info->chp_info_offset;
>  
>  	if (hdr_info->chp_info_offset == 0)
> -		return -1;
> +		return -EFAULT;
>  
>  	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
> -		return -1;
> +		return -EFAULT;
>  	return 0;
>  }
>  
> @@ -895,10 +895,10 @@ write_vbus_bus_info(struct visorchannel *chan,
>  	int off = sizeof(struct channel_header) + hdr_info->bus_info_offset;
>  
>  	if (hdr_info->bus_info_offset == 0)
> -		return -1;
> +		return -EFAULT;
>  
>  	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
> -		return -1;
> +		return -EFAULT;
>  	return 0;
>  }
>  
> @@ -915,10 +915,10 @@ write_vbus_dev_info(struct visorchannel *chan,
>  	    (hdr_info->device_info_struct_bytes * devix);
>  
>  	if (hdr_info->dev_info_offset == 0)
> -		return -1;
> +		return -EFAULT;
>  
>  	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
> -		return -1;
> +		return -EFAULT;
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 

This seems fine, but why are you bothering to return anything at all, since the
return code is ignored at all the call sites.  Or more directly, why aren't you
checking these return codes, and acting appropriately if a fault is returned?

Neil

Re: [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1 return values

[Neil Horman]

On Tue, May 31, 2016 at 10:26:29PM -0400, David Kershner wrote:
> From: Erik Arfvidson <erik.arfvidson@unisys.com>
> 
> This patch changes the vague -1 return values to -EPERM.
> This operation is not supported is a good alternative
> to -1 because the return is basically telling the caller
> that the processor doesn't support vmcall operations.
> 
> Signed-off-by: Erik Arfvidson <erik.arfvidson@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/iovmcall_gnuc.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/iovmcall_gnuc.h b/drivers/staging/unisys/visorbus/iovmcall_gnuc.h
> index b08b6ec..98ea7f3 100644
> --- a/drivers/staging/unisys/visorbus/iovmcall_gnuc.h
> +++ b/drivers/staging/unisys/visorbus/iovmcall_gnuc.h
> @@ -22,7 +22,7 @@ __unisys_vmcall_gnuc(unsigned long tuple, unsigned long reg_ebx,
>  
>  	cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
>  	if (!(cpuid_ecx & 0x80000000))
> -		return -1;
> +		return -EPERM;
>  
>  	__asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
>  		"a"(tuple), "b"(reg_ebx), "c"(reg_ecx));
> @@ -40,7 +40,7 @@ __unisys_extended_vmcall_gnuc(unsigned long long tuple,
>  
>  	cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
>  	if (!(cpuid_ecx & 0x80000000))
> -		return -1;
> +		return -EPERM;
>  
>  	__asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
>  		"a"(tuple), "b"(reg_ebx), "c"(reg_ecx), "d"(reg_edx));
> -- 
> 1.9.1
> 

This gets properly checked as far as return codes go for the most part, which is
good, but in the case of an issuing of the
VMCALL_QUERY_GUEST_VIRTUAL_TIME_OFFSET ioctl, the return code is simply copied
into a user space buffer.  That might be expected if -1 is used as an error
condition marker, but I would check before you assume that a user space caller
will handle -EINVAL properly there.

Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Neil Horman]

On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> From: Tim Sell <Timothy.Sell@unisys.com>
> 
> Locking in the _interrupt() function is NOT necessary so long as we ensure
> that interrupts have been stopped whenever we need to pause or resume the
> device, which we now do.
> 
> While a device is paused, we ensure that interrupts stay disabled, i.e.
> that the _interrupt() function will NOT be called, yet remember the desired
> state in devdata->interrupts_enabled if open() or close() are called are
> called while the device is paused.  Then when the device is resumed, we
> restore the actual state of interrupts (i.e., whether _interrupt() is going
> to be called or not) to the desired state in devdata->interrupts_enabled.
> 
> Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> ---
>  drivers/staging/unisys/visorinput/visorinput.c | 57 +++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c
> index 12a3570..9c00710 100644
> --- a/drivers/staging/unisys/visorinput/visorinput.c
> +++ b/drivers/staging/unisys/visorinput/visorinput.c
> @@ -66,6 +66,7 @@ struct visorinput_devdata {
>  	struct rw_semaphore lock_visor_dev; /* lock for dev */
>  	struct input_dev *visorinput_dev;
>  	bool paused;
> +	bool interrupts_enabled;
>  	unsigned int keycode_table_bytes; /* size of following array */
>  	/* for keyboard devices: visorkbd_keycode[] + visorkbd_ext_keycode[] */
>  	unsigned char keycode_table[0];
> @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev *visorinput_dev)
>  		return -EINVAL;
>  	}
>  	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
> +
> +	/*
> +	 * If we're not paused, really enable interrupts.
> +	 * Regardless of whether we are paused, set a flag indicating
> +	 * interrupts should be enabled so when we resume, interrupts
> +	 * will really be enabled.
> +	 */
> +	down_write(&devdata->lock_visor_dev);
> +	devdata->interrupts_enabled = true;
> +	if (devdata->paused)
> +		goto out_unlock;
Don't you want to wait until you actually enable interrupts here to set
interrupts_enabled to true?  Otherwise, if devdata->paused is true, you will be
out of sync.

>  	visorbus_enable_channel_interrupts(devdata->dev);
> +
> +out_unlock:
> +	up_write(&devdata->lock_visor_dev);
>  	return 0;
>  }
>  
> @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev *visorinput_dev)
>  		return;
>  	}
>  	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
> +
> +	/*
> +	 * If we're not paused, really disable interrupts.
> +	 * Regardless of whether we are paused, set a flag indicating
> +	 * interrupts should be disabled so when we resume we will
> +	 * not re-enable them.
> +	 */
> +
> +	down_write(&devdata->lock_visor_dev);
> +	devdata->interrupts_enabled = false;
> +	if (devdata->paused)
> +		goto out_unlock;
Ditto to my above comment

>  	visorbus_disable_channel_interrupts(devdata->dev);
> +
> +out_unlock:
> +	up_write(&devdata->lock_visor_dev);
>  }
>  
>  /*
> @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
>  	 * in visorinput_channel_interrupt()
>  	 */
>  
> -	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);
>  }
>  
> @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct visor_device *dev)
>  	if (!devdata)
>  		return;
>  
> -	down_write(&devdata->lock_visor_dev);
> -	if (devdata->paused) /* don't touch device/channel when paused */
> -		goto out_locked;
> -
>  	visorinput_dev = devdata->visorinput_dev;
> -	if (!visorinput_dev)
> -		goto out_locked;
>  
>  	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
>  		scancode = r.activity.arg1;
> @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct visor_device *dev)
>  			break;
>  		}
>  	}
> -out_locked:
> -	up_write(&devdata->lock_visor_dev);
>  }
>  
>  static int
> @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
>  		rc = -EBUSY;
>  		goto out_locked;
>  	}
> +	if (devdata->interrupts_enabled)
> +		visorbus_disable_channel_interrupts(dev);
> +
> +	/*
> +	 * due to above, at this time no thread of execution will be
> +	 * in visorinput_channel_interrupt()
> +	 */
> +
>  	devdata->paused = true;
>  	complete_func(dev, 0);
>  	rc = 0;
> @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
>  	}
>  	devdata->paused = false;
>  	complete_func(dev, 0);
> +
> +	/*
> +	 * Re-establish calls to visorinput_channel_interrupt() if that is
> +	 * the desired state that we've kept track of in interrupts_enabled
> +	 * while the device was paused.
> +	 */
> +	if (devdata->interrupts_enabled)
> +		visorbus_enable_channel_interrupts(dev);
> +

Unless I'm mistaken, it seems that visorinput_pause and visorinput_open or close
can be called in parallel on different cpus.  As such the state of
interrupts_enabled may change during the execution of this function, which would
lead to interrupts not getting properly enabled.

>  	rc = 0;
>  out_locked:
>  	up_write(&devdata->lock_visor_dev);
> -- 
> 1.9.1
> 

RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Sell, Timothy C]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@redhat.com]
> Sent: Wednesday, June 01, 2016 10:18 AM
> To: Kershner, David A
> Cc: corbet@lwn.net; tglx@linutronix.de; mingo@redhat.com;
> hpa@zytor.com; gregkh@linuxfoundation.org; Arfvidson, Erik; Sell, Timothy
> C; hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com;
> Curtin, Alexander Paul; janani.rvchndrn@gmail.com;
> sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David Anthony;
> dan.j.williams@intel.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer
> Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> > From: Tim Sell <Timothy.Sell@unisys.com>
> >
> > Locking in the _interrupt() function is NOT necessary so long as we ensure
> > that interrupts have been stopped whenever we need to pause or resume
> the
> > device, which we now do.
> >
> > While a device is paused, we ensure that interrupts stay disabled, i.e.
> > that the _interrupt() function will NOT be called, yet remember the
> desired
> > state in devdata->interrupts_enabled if open() or close() are called are
> > called while the device is paused.  Then when the device is resumed, we
> > restore the actual state of interrupts (i.e., whether _interrupt() is going
> > to be called or not) to the desired state in devdata->interrupts_enabled.
> >
> > Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > ---
> >  drivers/staging/unisys/visorinput/visorinput.c | 57
> +++++++++++++++++++++-----
> >  1 file changed, 47 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/unisys/visorinput/visorinput.c
> b/drivers/staging/unisys/visorinput/visorinput.c
> > index 12a3570..9c00710 100644
> > --- a/drivers/staging/unisys/visorinput/visorinput.c
> > +++ b/drivers/staging/unisys/visorinput/visorinput.c
> > @@ -66,6 +66,7 @@ struct visorinput_devdata {
> >  	struct rw_semaphore lock_visor_dev; /* lock for dev */
> >  	struct input_dev *visorinput_dev;
> >  	bool paused;
> > +	bool interrupts_enabled;
> >  	unsigned int keycode_table_bytes; /* size of following array */
> >  	/* for keyboard devices: visorkbd_keycode[] +
> visorkbd_ext_keycode[] */
> >  	unsigned char keycode_table[0];
> > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev
> *visorinput_dev)
> >  		return -EINVAL;
> >  	}
> >  	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
> > +
> > +	/*
> > +	 * If we're not paused, really enable interrupts.
> > +	 * Regardless of whether we are paused, set a flag indicating
> > +	 * interrupts should be enabled so when we resume, interrupts
> > +	 * will really be enabled.
> > +	 */
> > +	down_write(&devdata->lock_visor_dev);
> > +	devdata->interrupts_enabled = true;
> > +	if (devdata->paused)
> > +		goto out_unlock;
> Don't you want to wait until you actually enable interrupts here to set
> interrupts_enabled to true?  Otherwise, if devdata->paused is true, you will
> be
> out of sync.

No.  That's the intent of this code, to remember what the 
state of interrupts SHOULD be (via devdata->interrupts_enabled), at
a point in time when interrupts can NOT be enabled, e.g., when
the device is paused (devdata->paused).  After the device is resumed,
the real interrupt state (visorbus_enable_channel_interrupts())
will be synchronized with the remembered state.

> 
> >  	visorbus_enable_channel_interrupts(devdata->dev);
> > +
> > +out_unlock:
> > +	up_write(&devdata->lock_visor_dev);
> >  	return 0;
> >  }
> >
> > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev
> *visorinput_dev)
> >  		return;
> >  	}
> >  	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
> > +
> > +	/*
> > +	 * If we're not paused, really disable interrupts.
> > +	 * Regardless of whether we are paused, set a flag indicating
> > +	 * interrupts should be disabled so when we resume we will
> > +	 * not re-enable them.
> > +	 */
> > +
> > +	down_write(&devdata->lock_visor_dev);
> > +	devdata->interrupts_enabled = false;
> > +	if (devdata->paused)
> > +		goto out_unlock;
> Ditto to my above comment

Ditto my response above.

> 
> >  	visorbus_disable_channel_interrupts(devdata->dev);
> > +
> > +out_unlock:
> > +	up_write(&devdata->lock_visor_dev);
> >  }
> >
> >  /*
> > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
> >  	 * in visorinput_channel_interrupt()
> >  	 */
> >
> > -	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);
> >  }
> >
> > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct visor_device
> *dev)
> >  	if (!devdata)
> >  		return;
> >
> > -	down_write(&devdata->lock_visor_dev);
> > -	if (devdata->paused) /* don't touch device/channel when paused */
> > -		goto out_locked;
> > -
> >  	visorinput_dev = devdata->visorinput_dev;
> > -	if (!visorinput_dev)
> > -		goto out_locked;
> >
> >  	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
> >  		scancode = r.activity.arg1;
> > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct visor_device
> *dev)
> >  			break;
> >  		}
> >  	}
> > -out_locked:
> > -	up_write(&devdata->lock_visor_dev);
> >  }
> >
> >  static int
> > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
> >  		rc = -EBUSY;
> >  		goto out_locked;
> >  	}
> > +	if (devdata->interrupts_enabled)
> > +		visorbus_disable_channel_interrupts(dev);
> > +
> > +	/*
> > +	 * due to above, at this time no thread of execution will be
> > +	 * in visorinput_channel_interrupt()
> > +	 */
> > +
> >  	devdata->paused = true;
> >  	complete_func(dev, 0);
> >  	rc = 0;
> > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
> >  	}
> >  	devdata->paused = false;
> >  	complete_func(dev, 0);
> > +
> > +	/*
> > +	 * Re-establish calls to visorinput_channel_interrupt() if that is
> > +	 * the desired state that we've kept track of in interrupts_enabled
> > +	 * while the device was paused.
> > +	 */
> > +	if (devdata->interrupts_enabled)
> > +		visorbus_enable_channel_interrupts(dev);
> > +
> 
> Unless I'm mistaken, it seems that visorinput_pause and visorinput_open or
> close
> can be called in parallel on different cpus.  As such the state of
> interrupts_enabled may change during the execution of this function, which
> would
> lead to interrupts not getting properly enabled.
> 


You are correct that visorinput_pause and visorinput_open/close
can be called in parallel.  However, as I alluded to in my comment
above, the intent of this code is to just restore the actual interrupt
state with the desired state (remembered in
devdata->interrupts_enabled).  It's ok if interrupts don't get
enabled, because that would be our intent if there are no longer
any users of the device.  (In this case visorinput_close() would have
been called and devdata->interrupts_enabled would have got set
false while the device was paused.)

Tim Sell

> >  	rc = 0;
> >  out_locked:
> >  	up_write(&devdata->lock_visor_dev);
> > --
> > 1.9.1
> >

Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Neil Horman]

On Wed, Jun 01, 2016 at 03:09:13PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@redhat.com]
> > Sent: Wednesday, June 01, 2016 10:18 AM
> > To: Kershner, David A
> > Cc: corbet@lwn.net; tglx@linutronix.de; mingo@redhat.com;
> > hpa@zytor.com; gregkh@linuxfoundation.org; Arfvidson, Erik; Sell, Timothy
> > C; hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com;
> > Curtin, Alexander Paul; janani.rvchndrn@gmail.com;
> > sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David Anthony;
> > dan.j.williams@intel.com; linux-kernel@vger.kernel.org; linux-
> > doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> > Maintainer
> > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> > unnecessary locking
> > 
> > On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> > > From: Tim Sell <Timothy.Sell@unisys.com>
> > >
> > > Locking in the _interrupt() function is NOT necessary so long as we ensure
> > > that interrupts have been stopped whenever we need to pause or resume
> > the
> > > device, which we now do.
> > >
> > > While a device is paused, we ensure that interrupts stay disabled, i.e.
> > > that the _interrupt() function will NOT be called, yet remember the
> > desired
> > > state in devdata->interrupts_enabled if open() or close() are called are
> > > called while the device is paused.  Then when the device is resumed, we
> > > restore the actual state of interrupts (i.e., whether _interrupt() is going
> > > to be called or not) to the desired state in devdata->interrupts_enabled.
> > >
> > > Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> > > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > > ---
> > >  drivers/staging/unisys/visorinput/visorinput.c | 57
> > +++++++++++++++++++++-----
> > >  1 file changed, 47 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c
> > b/drivers/staging/unisys/visorinput/visorinput.c
> > > index 12a3570..9c00710 100644
> > > --- a/drivers/staging/unisys/visorinput/visorinput.c
> > > +++ b/drivers/staging/unisys/visorinput/visorinput.c
> > > @@ -66,6 +66,7 @@ struct visorinput_devdata {
> > >  	struct rw_semaphore lock_visor_dev; /* lock for dev */
> > >  	struct input_dev *visorinput_dev;
> > >  	bool paused;
> > > +	bool interrupts_enabled;
> > >  	unsigned int keycode_table_bytes; /* size of following array */
> > >  	/* for keyboard devices: visorkbd_keycode[] +
> > visorkbd_ext_keycode[] */
> > >  	unsigned char keycode_table[0];
> > > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev
> > *visorinput_dev)
> > >  		return -EINVAL;
> > >  	}
> > >  	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
> > > +
> > > +	/*
> > > +	 * If we're not paused, really enable interrupts.
> > > +	 * Regardless of whether we are paused, set a flag indicating
> > > +	 * interrupts should be enabled so when we resume, interrupts
> > > +	 * will really be enabled.
> > > +	 */
> > > +	down_write(&devdata->lock_visor_dev);
> > > +	devdata->interrupts_enabled = true;
> > > +	if (devdata->paused)
> > > +		goto out_unlock;
> > Don't you want to wait until you actually enable interrupts here to set
> > interrupts_enabled to true?  Otherwise, if devdata->paused is true, you will
> > be
> > out of sync.
> 
> No.  That's the intent of this code, to remember what the 
> state of interrupts SHOULD be (via devdata->interrupts_enabled), at
> a point in time when interrupts can NOT be enabled, e.g., when
> the device is paused (devdata->paused).  After the device is resumed,
> the real interrupt state (visorbus_enable_channel_interrupts())
> will be synchronized with the remembered state.
> 

Ok, I'll buy that, but it still looks rather racy to me.  It appears to me that
the code path in which the paused state is toggled (visorinput_pause|resume), is
called from a path that originates in visorchipset, specifically in the work
queue function controlvm_periodic_work.  Given that, its entirely possible for
the paused state of the virutal hardware to change while the device is being
opened.  That is to say devdata->paused can become true immediately after its
checked in visorinput_open above, and so we can enable interrupts on hardware
that is paused, which seems to be what this code is trying to avoid.

> > 
> > >  	visorbus_enable_channel_interrupts(devdata->dev);
> > > +
> > > +out_unlock:
> > > +	up_write(&devdata->lock_visor_dev);
> > >  	return 0;
> > >  }
> > >
> > > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev
> > *visorinput_dev)
> > >  		return;
> > >  	}
> > >  	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
> > > +
> > > +	/*
> > > +	 * If we're not paused, really disable interrupts.
> > > +	 * Regardless of whether we are paused, set a flag indicating
> > > +	 * interrupts should be disabled so when we resume we will
> > > +	 * not re-enable them.
> > > +	 */
> > > +
> > > +	down_write(&devdata->lock_visor_dev);
> > > +	devdata->interrupts_enabled = false;
> > > +	if (devdata->paused)
> > > +		goto out_unlock;
> > Ditto to my above comment
> 
> Ditto my response above.
> 
Same comment regarding racyness.

> > 
> > >  	visorbus_disable_channel_interrupts(devdata->dev);
> > > +
> > > +out_unlock:
> > > +	up_write(&devdata->lock_visor_dev);
> > >  }
> > >
> > >  /*
> > > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
> > >  	 * in visorinput_channel_interrupt()
> > >  	 */
> > >
> > > -	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);
> > >  }
> > >
> > > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct visor_device
> > *dev)
> > >  	if (!devdata)
> > >  		return;
> > >
> > > -	down_write(&devdata->lock_visor_dev);
> > > -	if (devdata->paused) /* don't touch device/channel when paused */
> > > -		goto out_locked;
> > > -
> > >  	visorinput_dev = devdata->visorinput_dev;
> > > -	if (!visorinput_dev)
> > > -		goto out_locked;
> > >
> > >  	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
> > >  		scancode = r.activity.arg1;
> > > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct visor_device
> > *dev)
> > >  			break;
> > >  		}
> > >  	}
> > > -out_locked:
> > > -	up_write(&devdata->lock_visor_dev);
> > >  }
> > >
> > >  static int
> > > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
> > >  		rc = -EBUSY;
> > >  		goto out_locked;
> > >  	}
> > > +	if (devdata->interrupts_enabled)
> > > +		visorbus_disable_channel_interrupts(dev);
> > > +
> > > +	/*
> > > +	 * due to above, at this time no thread of execution will be
> > > +	 * in visorinput_channel_interrupt()
> > > +	 */
> > > +
> > >  	devdata->paused = true;
> > >  	complete_func(dev, 0);
> > >  	rc = 0;
> > > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
> > >  	}
> > >  	devdata->paused = false;
> > >  	complete_func(dev, 0);
> > > +
> > > +	/*
> > > +	 * Re-establish calls to visorinput_channel_interrupt() if that is
> > > +	 * the desired state that we've kept track of in interrupts_enabled
> > > +	 * while the device was paused.
> > > +	 */
> > > +	if (devdata->interrupts_enabled)
> > > +		visorbus_enable_channel_interrupts(dev);
> > > +
> > 
> > Unless I'm mistaken, it seems that visorinput_pause and visorinput_open or
> > close
> > can be called in parallel on different cpus.  As such the state of
> > interrupts_enabled may change during the execution of this function, which
> > would
> > lead to interrupts not getting properly enabled.
> > 
> 
> 
> You are correct that visorinput_pause and visorinput_open/close
> can be called in parallel.  However, as I alluded to in my comment
> above, the intent of this code is to just restore the actual interrupt
> state with the desired state (remembered in
> devdata->interrupts_enabled).  It's ok if interrupts don't get
> enabled, because that would be our intent if there are no longer
> any users of the device.  (In this case visorinput_close() would have
> been called and devdata->interrupts_enabled would have got set
> false while the device was paused.)
> 


Heres an illustration of my concern.  Assume the visorinput device is currently
paused, and someone has called open on it while at the same time resuming it

CPU0				CPU1
				visoinput_resume
visorinput_open
 <handle random smi>		check ->interrupts_enabled (false)
 <return from smi>		<handle random smi>
 set interrupts_enabled=true	
 check ->paused (true)		<return from smi>	
				set ->paused = true
 return 0

In the above scenario visorinput_open and visorinput_resume will both return
without having enabled interrupts, rendering the device non-responsive.

A simmmilar scenario can be seen on close/pause, in which interrupts are left
enabled on a device that is paused.

It seems you can't remove all level of serialization here (though you can remove
some).  I would recommend that, instead of keeping your own mutex, you instead
augment visorinput_pause/resume, to extract the input_device structure from the
driver private data and hold the input device mutex when pausing/resuming the
device.  That will ensure that neither the paused or interrupts_enabled state
will change during the execution of visorinput_open/close

Neil

RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Sell, Timothy C]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@redhat.com]
> Sent: Wednesday, June 01, 2016 2:43 PM
> To: Sell, Timothy C
> Cc: Kershner, David A; corbet@lwn.net; tglx@linutronix.de;
> mingo@redhat.com; hpa@zytor.com; gregkh@linuxfoundation.org;
> Arfvidson, Erik; hofrat@osadl.org; dzickus@redhat.com;
> jes.sorensen@redhat.com; Curtin, Alexander Paul;
> janani.rvchndrn@gmail.com; sudipm.mukherjee@gmail.com;
> prarit@redhat.com; Binder, David Anthony; dan.j.williams@intel.com;
> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; *S-Par-Maintainer
> Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Wed, Jun 01, 2016 at 03:09:13PM +0000, Sell, Timothy C wrote:
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@redhat.com]
> > > Sent: Wednesday, June 01, 2016 10:18 AM
> > > To: Kershner, David A
> > > Cc: corbet@lwn.net; tglx@linutronix.de; mingo@redhat.com;
> > > hpa@zytor.com; gregkh@linuxfoundation.org; Arfvidson, Erik; Sell,
> Timothy
> > > C; hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com;
> > > Curtin, Alexander Paul; janani.rvchndrn@gmail.com;
> > > sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David
> Anthony;
> > > dan.j.williams@intel.com; linux-kernel@vger.kernel.org; linux-
> > > doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> > > Maintainer
> > > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> > > unnecessary locking
> > >
> > > On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> > > > From: Tim Sell <Timothy.Sell@unisys.com>
> > > >
> > > > Locking in the _interrupt() function is NOT necessary so long as we
> ensure
> > > > that interrupts have been stopped whenever we need to pause or
> resume
> > > the
> > > > device, which we now do.
> > > >
> > > > While a device is paused, we ensure that interrupts stay disabled, i.e.
> > > > that the _interrupt() function will NOT be called, yet remember the
> > > desired
> > > > state in devdata->interrupts_enabled if open() or close() are called are
> > > > called while the device is paused.  Then when the device is resumed,
> we
> > > > restore the actual state of interrupts (i.e., whether _interrupt() is going
> > > > to be called or not) to the desired state in devdata-
> >interrupts_enabled.
> > > >
> > > > Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> > > > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > > > ---
> > > >  drivers/staging/unisys/visorinput/visorinput.c | 57
> > > +++++++++++++++++++++-----
> > > >  1 file changed, 47 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c
> > > b/drivers/staging/unisys/visorinput/visorinput.c
> > > > index 12a3570..9c00710 100644
> > > > --- a/drivers/staging/unisys/visorinput/visorinput.c
> > > > +++ b/drivers/staging/unisys/visorinput/visorinput.c
> > > > @@ -66,6 +66,7 @@ struct visorinput_devdata {
> > > >  	struct rw_semaphore lock_visor_dev; /* lock for dev */
> > > >  	struct input_dev *visorinput_dev;
> > > >  	bool paused;
> > > > +	bool interrupts_enabled;
> > > >  	unsigned int keycode_table_bytes; /* size of following array */
> > > >  	/* for keyboard devices: visorkbd_keycode[] +
> > > visorkbd_ext_keycode[] */
> > > >  	unsigned char keycode_table[0];
> > > > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev
> > > *visorinput_dev)
> > > >  		return -EINVAL;
> > > >  	}
> > > >  	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
> > > > +
> > > > +	/*
> > > > +	 * If we're not paused, really enable interrupts.
> > > > +	 * Regardless of whether we are paused, set a flag indicating
> > > > +	 * interrupts should be enabled so when we resume, interrupts
> > > > +	 * will really be enabled.
> > > > +	 */
> > > > +	down_write(&devdata->lock_visor_dev);
> > > > +	devdata->interrupts_enabled = true;
> > > > +	if (devdata->paused)
> > > > +		goto out_unlock;
> > > Don't you want to wait until you actually enable interrupts here to set
> > > interrupts_enabled to true?  Otherwise, if devdata->paused is true, you
> will
> > > be
> > > out of sync.
> >
> > No.  That's the intent of this code, to remember what the
> > state of interrupts SHOULD be (via devdata->interrupts_enabled), at
> > a point in time when interrupts can NOT be enabled, e.g., when
> > the device is paused (devdata->paused).  After the device is resumed,
> > the real interrupt state (visorbus_enable_channel_interrupts())
> > will be synchronized with the remembered state.
> >
> 
> Ok, I'll buy that, but it still looks rather racy to me.  It appears to me that
> the code path in which the paused state is toggled
> (visorinput_pause|resume), is
> called from a path that originates in visorchipset, specifically in the work
> queue function controlvm_periodic_work.  Given that, its entirely possible
> for
> the paused state of the virutal hardware to change while the device is being
> opened.  That is to say devdata->paused can become true immediately after
> its
> checked in visorinput_open above, and so we can enable interrupts on
> hardware
> that is paused, which seems to be what this code is trying to avoid.
> 

You are absolutely correct about the 2 different threads of execution
where these functions can be called.

But in this code, we hold devdata->lock_visor_dev in order to prevent
the scenario you describe.  I.e., the code in all of the paths involved:
* never changes dev->paused or dev->interrupts_enabled without
holding devdata->lock_visor_dev
* never makes any decisions based on dev->paused or
dev->interrupts_enabled without holding devdata->lock_visor_dev

> > >
> > > >  	visorbus_enable_channel_interrupts(devdata->dev);
> > > > +
> > > > +out_unlock:
> > > > +	up_write(&devdata->lock_visor_dev);
> > > >  	return 0;
> > > >  }
> > > >
> > > > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev
> > > *visorinput_dev)
> > > >  		return;
> > > >  	}
> > > >  	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
> > > > +
> > > > +	/*
> > > > +	 * If we're not paused, really disable interrupts.
> > > > +	 * Regardless of whether we are paused, set a flag indicating
> > > > +	 * interrupts should be disabled so when we resume we will
> > > > +	 * not re-enable them.
> > > > +	 */
> > > > +
> > > > +	down_write(&devdata->lock_visor_dev);
> > > > +	devdata->interrupts_enabled = false;
> > > > +	if (devdata->paused)
> > > > +		goto out_unlock;
> > > Ditto to my above comment
> >
> > Ditto my response above.
> >
> Same comment regarding racyness.
> 
> > >
> > > >  	visorbus_disable_channel_interrupts(devdata->dev);
> > > > +
> > > > +out_unlock:
> > > > +	up_write(&devdata->lock_visor_dev);
> > > >  }
> > > >
> > > >  /*
> > > > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
> > > >  	 * in visorinput_channel_interrupt()
> > > >  	 */
> > > >
> > > > -	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);
> > > >  }
> > > >
> > > > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct
> visor_device
> > > *dev)
> > > >  	if (!devdata)
> > > >  		return;
> > > >
> > > > -	down_write(&devdata->lock_visor_dev);
> > > > -	if (devdata->paused) /* don't touch device/channel when paused */
> > > > -		goto out_locked;
> > > > -
> > > >  	visorinput_dev = devdata->visorinput_dev;
> > > > -	if (!visorinput_dev)
> > > > -		goto out_locked;
> > > >
> > > >  	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
> > > >  		scancode = r.activity.arg1;
> > > > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct
> visor_device
> > > *dev)
> > > >  			break;
> > > >  		}
> > > >  	}
> > > > -out_locked:
> > > > -	up_write(&devdata->lock_visor_dev);
> > > >  }
> > > >
> > > >  static int
> > > > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
> > > >  		rc = -EBUSY;
> > > >  		goto out_locked;
> > > >  	}
> > > > +	if (devdata->interrupts_enabled)
> > > > +		visorbus_disable_channel_interrupts(dev);
> > > > +
> > > > +	/*
> > > > +	 * due to above, at this time no thread of execution will be
> > > > +	 * in visorinput_channel_interrupt()
> > > > +	 */
> > > > +
> > > >  	devdata->paused = true;
> > > >  	complete_func(dev, 0);
> > > >  	rc = 0;
> > > > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
> > > >  	}
> > > >  	devdata->paused = false;
> > > >  	complete_func(dev, 0);
> > > > +
> > > > +	/*
> > > > +	 * Re-establish calls to visorinput_channel_interrupt() if that is
> > > > +	 * the desired state that we've kept track of in interrupts_enabled
> > > > +	 * while the device was paused.
> > > > +	 */
> > > > +	if (devdata->interrupts_enabled)
> > > > +		visorbus_enable_channel_interrupts(dev);
> > > > +
> > >
> > > Unless I'm mistaken, it seems that visorinput_pause and
> visorinput_open or
> > > close
> > > can be called in parallel on different cpus.  As such the state of
> > > interrupts_enabled may change during the execution of this function,
> which
> > > would
> > > lead to interrupts not getting properly enabled.
> > >
> >
> >
> > You are correct that visorinput_pause and visorinput_open/close
> > can be called in parallel.  However, as I alluded to in my comment
> > above, the intent of this code is to just restore the actual interrupt
> > state with the desired state (remembered in
> > devdata->interrupts_enabled).  It's ok if interrupts don't get
> > enabled, because that would be our intent if there are no longer
> > any users of the device.  (In this case visorinput_close() would have
> > been called and devdata->interrupts_enabled would have got set
> > false while the device was paused.)
> >
> 
> 
> Heres an illustration of my concern.  Assume the visorinput device is
> currently
> paused, and someone has called open on it while at the same time
> resuming it
> 
> CPU0				CPU1
> 				visoinput_resume
> visorinput_open
>  <handle random smi>		check ->interrupts_enabled (false)
>  <return from smi>		<handle random smi>
>  set interrupts_enabled=true
>  check ->paused (true)		<return from smi>
> 				set ->paused = true
>  return 0
> 
> In the above scenario visorinput_open and visorinput_resume will both
> return
> without having enabled interrupts, rendering the device non-responsive.
> 
> A simmmilar scenario can be seen on close/pause, in which interrupts are
> left
> enabled on a device that is paused.
> 
> It seems you can't remove all level of serialization here (though you can
> remove
> some).  I would recommend that, instead of keeping your own mutex, you
> instead
> augment visorinput_pause/resume, to extract the input_device structure
> from the
> driver private data and hold the input device mutex when
> pausing/resuming the
> device.  That will ensure that neither the paused or interrupts_enabled
> state
> will change during the execution of visorinput_open/close
> 
> Neil

Nice illustration.  That would usually be enough to drill something thru
my thick skull, but I'm still missing something in this case.  ;-(

I'm still missing how this scenario could happen given our usage of 
devdata->lock_visor_dev.  We hold that lock for the entire execution of
visorinput_open(), visorinput_close(), visorinput_pause(), and
visorinput_resume(), where we are dealing with the checks and state
transitions of devdata->paused, devdata->interrupts_enabled, and
the actual state of channel interrupts.  So even if the circumstance
presented itself where we were ready to run thru 2 of those functions
for the same device on mutiple cpus at the same exact time, the
execution would be serialized due to devdata->lock_visor_dev.

Tim Sell

Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Neil Horman]

On Thu, Jun 02, 2016 at 05:02:11AM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@redhat.com]
> > Sent: Wednesday, June 01, 2016 2:43 PM
> > To: Sell, Timothy C
> > Cc: Kershner, David A; corbet@lwn.net; tglx@linutronix.de;
> > mingo@redhat.com; hpa@zytor.com; gregkh@linuxfoundation.org;
> > Arfvidson, Erik; hofrat@osadl.org; dzickus@redhat.com;
> > jes.sorensen@redhat.com; Curtin, Alexander Paul;
> > janani.rvchndrn@gmail.com; sudipm.mukherjee@gmail.com;
> > prarit@redhat.com; Binder, David Anthony; dan.j.williams@intel.com;
> > linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org; driverdev-
> > devel@linuxdriverproject.org; *S-Par-Maintainer
> > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> > unnecessary locking
> > 
> > On Wed, Jun 01, 2016 at 03:09:13PM +0000, Sell, Timothy C wrote:
> > > > -----Original Message-----
> > > > From: Neil Horman [mailto:nhorman@redhat.com]
> > > > Sent: Wednesday, June 01, 2016 10:18 AM
> > > > To: Kershner, David A
> > > > Cc: corbet@lwn.net; tglx@linutronix.de; mingo@redhat.com;
> > > > hpa@zytor.com; gregkh@linuxfoundation.org; Arfvidson, Erik; Sell,
> > Timothy
> > > > C; hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com;
> > > > Curtin, Alexander Paul; janani.rvchndrn@gmail.com;
> > > > sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David
> > Anthony;
> > > > dan.j.williams@intel.com; linux-kernel@vger.kernel.org; linux-
> > > > doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> > > > Maintainer
> > > > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> > > > unnecessary locking
> > > >
> > > > On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> > > > > From: Tim Sell <Timothy.Sell@unisys.com>
> > > > >
> > > > > Locking in the _interrupt() function is NOT necessary so long as we
> > ensure
> > > > > that interrupts have been stopped whenever we need to pause or
> > resume
> > > > the
> > > > > device, which we now do.
> > > > >
> > > > > While a device is paused, we ensure that interrupts stay disabled, i.e.
> > > > > that the _interrupt() function will NOT be called, yet remember the
> > > > desired
> > > > > state in devdata->interrupts_enabled if open() or close() are called are
> > > > > called while the device is paused.  Then when the device is resumed,
> > we
> > > > > restore the actual state of interrupts (i.e., whether _interrupt() is going
> > > > > to be called or not) to the desired state in devdata-
> > >interrupts_enabled.
> > > > >
> > > > > Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> > > > > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > > > > ---
> > > > >  drivers/staging/unisys/visorinput/visorinput.c | 57
> > > > +++++++++++++++++++++-----
> > > > >  1 file changed, 47 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c
> > > > b/drivers/staging/unisys/visorinput/visorinput.c
> > > > > index 12a3570..9c00710 100644
> > > > > --- a/drivers/staging/unisys/visorinput/visorinput.c
> > > > > +++ b/drivers/staging/unisys/visorinput/visorinput.c
> > > > > @@ -66,6 +66,7 @@ struct visorinput_devdata {
> > > > >  	struct rw_semaphore lock_visor_dev; /* lock for dev */
> > > > >  	struct input_dev *visorinput_dev;
> > > > >  	bool paused;
> > > > > +	bool interrupts_enabled;
> > > > >  	unsigned int keycode_table_bytes; /* size of following array */
> > > > >  	/* for keyboard devices: visorkbd_keycode[] +
> > > > visorkbd_ext_keycode[] */
> > > > >  	unsigned char keycode_table[0];
> > > > > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev
> > > > *visorinput_dev)
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  	dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__);
> > > > > +
> > > > > +	/*
> > > > > +	 * If we're not paused, really enable interrupts.
> > > > > +	 * Regardless of whether we are paused, set a flag indicating
> > > > > +	 * interrupts should be enabled so when we resume, interrupts
> > > > > +	 * will really be enabled.
> > > > > +	 */
> > > > > +	down_write(&devdata->lock_visor_dev);
> > > > > +	devdata->interrupts_enabled = true;
> > > > > +	if (devdata->paused)
> > > > > +		goto out_unlock;
> > > > Don't you want to wait until you actually enable interrupts here to set
> > > > interrupts_enabled to true?  Otherwise, if devdata->paused is true, you
> > will
> > > > be
> > > > out of sync.
> > >
> > > No.  That's the intent of this code, to remember what the
> > > state of interrupts SHOULD be (via devdata->interrupts_enabled), at
> > > a point in time when interrupts can NOT be enabled, e.g., when
> > > the device is paused (devdata->paused).  After the device is resumed,
> > > the real interrupt state (visorbus_enable_channel_interrupts())
> > > will be synchronized with the remembered state.
> > >
> > 
> > Ok, I'll buy that, but it still looks rather racy to me.  It appears to me that
> > the code path in which the paused state is toggled
> > (visorinput_pause|resume), is
> > called from a path that originates in visorchipset, specifically in the work
> > queue function controlvm_periodic_work.  Given that, its entirely possible
> > for
> > the paused state of the virutal hardware to change while the device is being
> > opened.  That is to say devdata->paused can become true immediately after
> > its
> > checked in visorinput_open above, and so we can enable interrupts on
> > hardware
> > that is paused, which seems to be what this code is trying to avoid.
> > 
> 
> You are absolutely correct about the 2 different threads of execution
> where these functions can be called.
> 
> But in this code, we hold devdata->lock_visor_dev in order to prevent
> the scenario you describe.  I.e., the code in all of the paths involved:
> * never changes dev->paused or dev->interrupts_enabled without
> holding devdata->lock_visor_dev
> * never makes any decisions based on dev->paused or
> dev->interrupts_enabled without holding devdata->lock_visor_dev
> 
> > > >
> > > > >  	visorbus_enable_channel_interrupts(devdata->dev);
> > > > > +
> > > > > +out_unlock:
> > > > > +	up_write(&devdata->lock_visor_dev);
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev
> > > > *visorinput_dev)
> > > > >  		return;
> > > > >  	}
> > > > >  	dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__);
> > > > > +
> > > > > +	/*
> > > > > +	 * If we're not paused, really disable interrupts.
> > > > > +	 * Regardless of whether we are paused, set a flag indicating
> > > > > +	 * interrupts should be disabled so when we resume we will
> > > > > +	 * not re-enable them.
> > > > > +	 */
> > > > > +
> > > > > +	down_write(&devdata->lock_visor_dev);
> > > > > +	devdata->interrupts_enabled = false;
> > > > > +	if (devdata->paused)
> > > > > +		goto out_unlock;
> > > > Ditto to my above comment
> > >
> > > Ditto my response above.
> > >
> > Same comment regarding racyness.
> > 
> > > >
> > > > >  	visorbus_disable_channel_interrupts(devdata->dev);
> > > > > +
> > > > > +out_unlock:
> > > > > +	up_write(&devdata->lock_visor_dev);
> > > > >  }
> > > > >
> > > > >  /*
> > > > > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev)
> > > > >  	 * in visorinput_channel_interrupt()
> > > > >  	 */
> > > > >
> > > > > -	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);
> > > > >  }
> > > > >
> > > > > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct
> > visor_device
> > > > *dev)
> > > > >  	if (!devdata)
> > > > >  		return;
> > > > >
> > > > > -	down_write(&devdata->lock_visor_dev);
> > > > > -	if (devdata->paused) /* don't touch device/channel when paused */
> > > > > -		goto out_locked;
> > > > > -
> > > > >  	visorinput_dev = devdata->visorinput_dev;
> > > > > -	if (!visorinput_dev)
> > > > > -		goto out_locked;
> > > > >
> > > > >  	while (visorchannel_signalremove(dev->visorchannel, 0, &r)) {
> > > > >  		scancode = r.activity.arg1;
> > > > > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct
> > visor_device
> > > > *dev)
> > > > >  			break;
> > > > >  		}
> > > > >  	}
> > > > > -out_locked:
> > > > > -	up_write(&devdata->lock_visor_dev);
> > > > >  }
> > > > >
> > > > >  static int
> > > > > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev,
> > > > >  		rc = -EBUSY;
> > > > >  		goto out_locked;
> > > > >  	}
> > > > > +	if (devdata->interrupts_enabled)
> > > > > +		visorbus_disable_channel_interrupts(dev);
> > > > > +
> > > > > +	/*
> > > > > +	 * due to above, at this time no thread of execution will be
> > > > > +	 * in visorinput_channel_interrupt()
> > > > > +	 */
> > > > > +
> > > > >  	devdata->paused = true;
> > > > >  	complete_func(dev, 0);
> > > > >  	rc = 0;
> > > > > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev,
> > > > >  	}
> > > > >  	devdata->paused = false;
> > > > >  	complete_func(dev, 0);
> > > > > +
> > > > > +	/*
> > > > > +	 * Re-establish calls to visorinput_channel_interrupt() if that is
> > > > > +	 * the desired state that we've kept track of in interrupts_enabled
> > > > > +	 * while the device was paused.
> > > > > +	 */
> > > > > +	if (devdata->interrupts_enabled)
> > > > > +		visorbus_enable_channel_interrupts(dev);
> > > > > +
> > > >
> > > > Unless I'm mistaken, it seems that visorinput_pause and
> > visorinput_open or
> > > > close
> > > > can be called in parallel on different cpus.  As such the state of
> > > > interrupts_enabled may change during the execution of this function,
> > which
> > > > would
> > > > lead to interrupts not getting properly enabled.
> > > >
> > >
> > >
> > > You are correct that visorinput_pause and visorinput_open/close
> > > can be called in parallel.  However, as I alluded to in my comment
> > > above, the intent of this code is to just restore the actual interrupt
> > > state with the desired state (remembered in
> > > devdata->interrupts_enabled).  It's ok if interrupts don't get
> > > enabled, because that would be our intent if there are no longer
> > > any users of the device.  (In this case visorinput_close() would have
> > > been called and devdata->interrupts_enabled would have got set
> > > false while the device was paused.)
> > >
> > 
> > 
> > Heres an illustration of my concern.  Assume the visorinput device is
> > currently
> > paused, and someone has called open on it while at the same time
> > resuming it
> > 
> > CPU0				CPU1
> > 				visoinput_resume
> > visorinput_open
> >  <handle random smi>		check ->interrupts_enabled (false)
> >  <return from smi>		<handle random smi>
> >  set interrupts_enabled=true
> >  check ->paused (true)		<return from smi>
> > 				set ->paused = true
> >  return 0
> > 
> > In the above scenario visorinput_open and visorinput_resume will both
> > return
> > without having enabled interrupts, rendering the device non-responsive.
> > 
> > A simmmilar scenario can be seen on close/pause, in which interrupts are
> > left
> > enabled on a device that is paused.
> > 
> > It seems you can't remove all level of serialization here (though you can
> > remove
> > some).  I would recommend that, instead of keeping your own mutex, you
> > instead
> > augment visorinput_pause/resume, to extract the input_device structure
> > from the
> > driver private data and hold the input device mutex when
> > pausing/resuming the
> > device.  That will ensure that neither the paused or interrupts_enabled
> > state
> > will change during the execution of visorinput_open/close
> > 
> > Neil
> 
> Nice illustration.  That would usually be enough to drill something thru
> my thick skull, but I'm still missing something in this case.  ;-(
> 
> I'm still missing how this scenario could happen given our usage of 
> devdata->lock_visor_dev.  We hold that lock for the entire execution of
> visorinput_open(), visorinput_close(), visorinput_pause(), and
> visorinput_resume(), where we are dealing with the checks and state
> transitions of devdata->paused, devdata->interrupts_enabled, and
> the actual state of channel interrupts.  So even if the circumstance
> presented itself where we were ready to run thru 2 of those functions
> for the same device on mutiple cpus at the same exact time, the
> execution would be serialized due to devdata->lock_visor_dev.
> 
Ok, there it is, that works then, thanks for the clarification

Neil

> Tim Sell
> 

RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

[Sell, Timothy C]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, June 01, 2016 2:41 AM
> To: Kershner, David A
> Cc: corbet@lwn.net; mingo@redhat.com; hpa@zytor.com;
> gregkh@linuxfoundation.org; Arfvidson, Erik; Sell, Timothy C;
> hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com; Curtin,
> Alexander Paul; janani.rvchndrn@gmail.com;
> sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David Anthony;
> nhorman@redhat.com; dan.j.williams@intel.com; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; *S-Par-Maintainer
> Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Tue, 31 May 2016, David Kershner wrote:
> > +	/*
> > +	 * If we're not paused, really enable interrupts.
> > +	 * Regardless of whether we are paused, set a flag indicating
> > +	 * interrupts should be enabled so when we resume, interrupts
> > +	 * will really be enabled.
> > +	 */
> > +	down_write(&devdata->lock_visor_dev);
> 
> Why is this a rw_semaphore? It's only ever taken with down_write() and it's
> always the same context. Should be a mutex, right?
> 

Correct.  We have a local patch that addresses this, but would like
to submit this via a follow-on patchset if possible.  I'll explain.

Rationale: our intent for this patchset was to focus on the visorbus
driver ONLY.  The only reason visorinput got involved in the first place
was due to the visorbus change that necessitated that we remove the locking
from visorinput_channel_interrupt(), due to that now being called from atomic
context.

If the semaphore --> mutex change would have been as simple as it sounds,
we would have had NO problem including it with the next version (v3) of this
patchset.  But unfortunately, this change uncovered a latent defect, which
necessitated yet another patch.  (I know... hard to believe that something
this simple would do that, but it did.)  Rather than further complicating this
patchset, we thought it would be better to address the visorinput issues via a
separate follow-on patchset.

Is that acceptable for you?

> While at it, please convert the notifier_lock to a mutex as well.

Thanks.  Since this is visorbus-specific, we DO plan to address this in v3 of
this patchset, which will most-likely just be REMOVING notifier_lock altogether.

Tim Sell

> 
> Thanks,
> 
> 	tglx