Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1410748 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/27] Fixed issues raised by  tglx, then move visorbus to drivers/virt David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock a mutex David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
      Re: [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock  a mutex Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
    [PATCH v2 27/27] drivers: Add visorbus to the drivers directory David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    [PATCH v2 17/27] staging: unisys: visorbus: vbusdeviceinfo function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    [PATCH v2 12/27] staging: unisys: visorbus: remove periodic_work.h/.c David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    [PATCH v2 06/27] staging: unisys: visorbus: modify format string to match argument David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    [PATCH v2 16/27] staging: unisys: Remove reference to unused STANDALONE_CLIENT David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    [PATCH v2 05/27] staging: unisys: visorbus: remove unused struct David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 07/27] staging: unisys: visornic: Correct comment spelling mistake David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 24/27] staging: unisys: Move vbushelper.h to visorbus directory David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
      Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
        RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-03 06:40 +0200
      Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-01 16:20 +0200
        RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-01 17:10 +0200
          Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-01 20:50 +0200
            RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-02 07:10 +0200
              Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-02 14:50 +0200
    [PATCH v2 15/27] staging: unisys: visorbus: Remove unused functions David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
      Re: [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel  function descriptions more kerneldoc-like Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
    [PATCH v2 23/27] staging: unisys: visorbus: Add kerneldoc-style comments for visorbus API David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 04/27] staging: unisys: visorbus: remove unused module parameters David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 18/27] staging: unisys: visorbus: make function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 14/27] staging: unisys: visorbus: Remove unnecessary EXPORT_SYMBOL statements David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 02/27] staging: unisys: visorchipset change -1 return value David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
      Re: [PATCH v2 02/27] staging: unisys: visorchipset change -1 return  value Neil Horman <nhorman@redhat.com> - 2016-06-01 15:20 +0200
    [PATCH v2 08/27] staging: unisys: include: Remove thread-related enum members David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 22/27] staging: unisys: visorbus: Move visorbus-unique functions to private header David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1 return values David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
      Re: [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1  return values Neil Horman <nhorman@redhat.com> - 2016-06-01 15:40 +0200
    [PATCH v2 21/27] staging: unisys: visorbus: make visorchipset function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 01/27] staging: unisys: visorbus change -1 return values David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
      Re: [PATCH v2 01/27] staging: unisys: visorbus change -1 return  values Neil Horman <nhorman@redhat.com> - 2016-06-01 15:30 +0200
    [PATCH v2 19/27] staging: unisys: visorbus: make visorbus_private.h function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 11/27] staging: unisys: visorbus: use kernel timer instead of workqueue David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    [PATCH v2 09/27] staging: unisys: visorbus: removed unused periodic_test_workqueue David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1410851 — 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?

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

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

#1411266 — 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.

>  	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
> 

[toc] | [prev] | [next] | [standalone]

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

> -----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
> >

[toc] | [prev] | [next] | [standalone]

#1411464 — 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.

> > 
> > >  	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

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web