[PATCH 00/10] staging: unisys: additional error handling and channel cleanups.

[PATCH 00/10] staging: unisys: additional error handling and channel cleanups.

[David Kershner]

This series fixes a problem where we were using a Linux specific data
structure in our s-Par channel. These channels are shared across multiple
OS types and should not use OS specific structures unless absolutely
needed. It also adds some additional error handling and other cleanup.

This patchset also introduces some changes being made to the functions that
invoke the uevents that occur on chipset_ready/selftest/notready messages
from the s-Par firmware. This simplifies the functions so that we can
tackle more challenging changes in later patches.

David Binder (6):
  staging: unisys: visorbus: Remove unused struct in visorchannel.c
  staging: unisys: visorbus: Check controlvm message payload size
  staging: unisys: visorbus: Consolidate kobject functions
  staging: unisys: visorbus: Clarify reason for pointer check in bus_destroy()
  staging: unisys: visorbus: Clarify reason for bus pointer checks
  staging: unisys: visorbus: Clarify reason for device pointer checks

David Kershner (2):
  staging: unisys: visorbus: remove putfile data structures
  staging: unisys: visorbus: get rid of unused payload info

Steven Matthews (1):
  staging: unisys: include: fix improper use of dma_data_direction

Tim Sell (1):
  staging: unisys: visornic: prevent hang doing 'modprobe -r visornic'

 drivers/staging/unisys/include/iochannel.h      |  11 +-
 drivers/staging/unisys/visorbus/visorbus_main.c |   1 +-
 drivers/staging/unisys/visorbus/visorchannel.c  |   6 +-
 drivers/staging/unisys/visorbus/visorchipset.c  | 240 +++--------------
 drivers/staging/unisys/visorhba/visorhba_main.c |  22 +-
 drivers/staging/unisys/visornic/visornic_main.c |   5 +-
 6 files changed, 75 insertions(+), 210 deletions(-)

base-commit: 203804300820c2c1ed28f9868c0fbd67b816cef3
-- 
git-series 0.9.1

[PATCH 01/10] staging: unisys: include: fix improper use of dma_data_direction

[David Kershner]

From: Steven Matthews <steven.matthews@unisys.com>

Replace use of standard Linux dma_data_direction with a Unisys-
specific uis_dma_data_direction and provide a function to convert
from the latter to the former.  This is necessary because Unisys
s-Par depends on the exact format of this field in multiple OSs
and languages, and so using the standard version creates an
unnecessary dependency between the kernel and s-Par.

Signed-off-by: Steven Matthews <steven.matthews@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/include/iochannel.h      | 11 +++++++--
 drivers/staging/unisys/visorhba/visorhba_main.c | 22 +++++++++++++++++-
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/unisys/include/iochannel.h b/drivers/staging/unisys/include/iochannel.h
index 54f4900..6e80462 100644
--- a/drivers/staging/unisys/include/iochannel.h
+++ b/drivers/staging/unisys/include/iochannel.h
@@ -31,7 +31,6 @@
 
 #include <linux/uuid.h>
 
-#include <linux/dma-direction.h>
 #include "channel.h"
 
 #define ULTRA_VHBA_CHANNEL_PROTOCOL_SIGNATURE ULTRA_CHANNEL_PROTOCOL_SIGNATURE
@@ -80,6 +79,14 @@
 /* Size of cdb - i.e., SCSI cmnd */
 #define MAX_CMND_SIZE 16
 
+/* Unisys-specific DMA direction values */
+enum uis_dma_data_direction {
+	UIS_DMA_BIDIRECTIONAL = 0,
+	UIS_DMA_TO_DEVICE,
+	UIS_DMA_FROM_DEVICE,
+	UIS_DMA_NONE
+};
+
 #define MAX_SENSE_SIZE 64
 
 #define MAX_PHYS_INFO 64
@@ -192,7 +199,7 @@ struct uiscmdrsp_scsi {
 							 * information for each
 							 * fragment
 							 */
-	enum dma_data_direction data_dir; /* direction of the data, if any */
+	enum uis_dma_data_direction data_dir; /* direction of the data */
 	struct uisscsi_dest vdest;	/* identifies the virtual hba, id, */
 					/* channel, lun to which cmd was sent */
 
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 0ce92c8..8765f67 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -462,6 +462,25 @@ static const char *visorhba_get_info(struct Scsi_Host *shp)
 	return "visorhba";
 }
 
+/*
+ *	dma_data_dir_linux_to_spar - convert dma_data_direction value to
+ *				     Unisys-specific equivalent
+ *	@d: dma direction value to convert
+ *
+ *	Returns the Unisys-specific dma direction value corresponding to @d
+ */
+static enum
+uis_dma_data_direction dma_data_dir_linux_to_spar(enum dma_data_direction d)
+{
+	switch (d) {
+	case DMA_BIDIRECTIONAL: return UIS_DMA_BIDIRECTIONAL;
+	case DMA_TO_DEVICE: return UIS_DMA_TO_DEVICE;
+	case DMA_FROM_DEVICE: return UIS_DMA_FROM_DEVICE;
+	case DMA_NONE: return UIS_DMA_NONE;
+	default: return UIS_DMA_NONE;
+	}
+}
+
 /**
  *	visorhba_queue_command_lck -- queues command to the Service Partition
  *	@scsicmd: Command to be queued
@@ -512,7 +531,8 @@ visorhba_queue_command_lck(struct scsi_cmnd *scsicmd,
 	cmdrsp->scsi.vdest.id = scsidev->id;
 	cmdrsp->scsi.vdest.lun = scsidev->lun;
 	/* save datadir */
-	cmdrsp->scsi.data_dir = scsicmd->sc_data_direction;
+	cmdrsp->scsi.data_dir =
+		dma_data_dir_linux_to_spar(scsicmd->sc_data_direction);
 	memcpy(cmdrsp->scsi.cmnd, cdb, MAX_CMND_SIZE);
 
 	cmdrsp->scsi.bufflen = scsi_bufflen(scsicmd);
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/10] staging: unisys: visorbus: Remove unused struct in visorchannel.c

[David Kershner]

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

Removes struct safe_uis_queue, which is within struct visorchannel. The
struct is not used anywhere in the s-Par drivers.

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

diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
index f51a725..e91febc 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -45,12 +45,6 @@ struct visorchannel {
 	spinlock_t insert_lock; /* protect head writes in chan_hdr */
 	spinlock_t remove_lock;	/* protect tail writes in chan_hdr */
 
-	struct {
-		struct signal_queue_header req_queue;
-		struct signal_queue_header rsp_queue;
-		struct signal_queue_header event_queue;
-		struct signal_queue_header ack_queue;
-	} safe_uis_queue;
 	uuid_le type;
 	uuid_le inst;
 };
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 03/10] staging: unisys: visorbus: Check controlvm message payload size

[David Kershner]

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

Checks the controlvm message's payload size before copying it into a
parser_context struct's name region.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 4e630ea..df2dfeb 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -399,6 +399,10 @@ parser_name_get(struct parser_context *ctx)
 	struct spar_controlvm_parameters_header *phdr = NULL;
 
 	phdr = (struct spar_controlvm_parameters_header *)(ctx->data);
+
+	if (phdr->name_offset + phdr->name_length > ctx->param_bytes)
+		return NULL;
+
 	ctx->curr = ctx->data + phdr->name_offset;
 	ctx->bytes_remaining = phdr->name_length;
 	return parser_string_get(ctx);
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 04/10] staging: unisys: visorbus: Consolidate kobject functions

[David Kershner]

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

Simplifies kobject usage in visorchipset.c by combining pairs of functions
that are better expressed combined.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 76 +++++++------------
 1 file changed, 31 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index df2dfeb..c90ea6a 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -1435,22 +1435,33 @@ parahotplug_process_message(struct controlvm_message *inmsg)
 	}
 }
 
-/**
- * visorchipset_chipset_ready() - sends chipset_ready action
+/*
+ * chipset_ready_uevent() - sends chipset_ready action
  *
  * Send ACTION=online for DEVPATH=/sys/devices/platform/visorchipset.
  *
- * Return: CONTROLVM_RESP_SUCCESS
+ * Return: 0 on success, negative on failure
  */
 static int
-visorchipset_chipset_ready(void)
+chipset_ready_uevent(struct controlvm_message_header *msg_hdr)
 {
 	kobject_uevent(&visorchipset_platform_device.dev.kobj, KOBJ_ONLINE);
-	return CONTROLVM_RESP_SUCCESS;
+
+	if (msg_hdr->flags.response_expected)
+		return controlvm_respond(msg_hdr, CONTROLVM_RESP_SUCCESS);
+
+	return 0;
 }
 
+/*
+ * chipset_selftest_uevent() - sends chipset_selftest action
+ *
+ * Send ACTION=online for DEVPATH=/sys/devices/platform/visorchipset.
+ *
+ * Return: 0 on success, negative on failure
+ */
 static int
-visorchipset_chipset_selftest(void)
+chipset_selftest_uevent(struct controlvm_message_header *msg_hdr)
 {
 	char env_selftest[20];
 	char *envp[] = { env_selftest, NULL };
@@ -1458,54 +1469,29 @@ visorchipset_chipset_selftest(void)
 	sprintf(env_selftest, "SPARSP_SELFTEST=%d", 1);
 	kobject_uevent_env(&visorchipset_platform_device.dev.kobj, KOBJ_CHANGE,
 			   envp);
-	return CONTROLVM_RESP_SUCCESS;
+
+	if (msg_hdr->flags.response_expected)
+		return controlvm_respond(msg_hdr, CONTROLVM_RESP_SUCCESS);
+
+	return 0;
 }
 
-/**
- * visorchipset_chipset_notready() - sends chipset_notready action
+/*
+ * chipset_notready_uevent() - sends chipset_notready action
  *
  * Send ACTION=offline for DEVPATH=/sys/devices/platform/visorchipset.
  *
- * Return: CONTROLVM_RESP_SUCCESS
+ * Return: 0 on success, negative on failure
  */
 static int
-visorchipset_chipset_notready(void)
+chipset_notready_uevent(struct controlvm_message_header *msg_hdr)
 {
 	kobject_uevent(&visorchipset_platform_device.dev.kobj, KOBJ_OFFLINE);
-	return CONTROLVM_RESP_SUCCESS;
-}
-
-static void
-chipset_ready(struct controlvm_message_header *msg_hdr)
-{
-	int rc = visorchipset_chipset_ready();
-
-	if (rc != CONTROLVM_RESP_SUCCESS)
-		rc = -rc;
-	if (msg_hdr->flags.response_expected)
-		controlvm_respond(msg_hdr, rc);
-}
 
-static void
-chipset_selftest(struct controlvm_message_header *msg_hdr)
-{
-	int rc = visorchipset_chipset_selftest();
-
-	if (rc != CONTROLVM_RESP_SUCCESS)
-		rc = -rc;
 	if (msg_hdr->flags.response_expected)
-		controlvm_respond(msg_hdr, rc);
-}
+		return controlvm_respond(msg_hdr, CONTROLVM_RESP_SUCCESS);
 
-static void
-chipset_notready(struct controlvm_message_header *msg_hdr)
-{
-	int rc = visorchipset_chipset_notready();
-
-	if (rc != CONTROLVM_RESP_SUCCESS)
-		rc = -rc;
-	if (msg_hdr->flags.response_expected)
-		controlvm_respond(msg_hdr, rc);
+	return 0;
 }
 
 static inline unsigned int
@@ -1967,13 +1953,13 @@ handle_command(struct controlvm_message inmsg, u64 channel_addr)
 			controlvm_respond(&inmsg.hdr, CONTROLVM_RESP_SUCCESS);
 		break;
 	case CONTROLVM_CHIPSET_READY:
-		chipset_ready(&inmsg.hdr);
+		chipset_ready_uevent(&inmsg.hdr);
 		break;
 	case CONTROLVM_CHIPSET_SELFTEST:
-		chipset_selftest(&inmsg.hdr);
+		chipset_selftest_uevent(&inmsg.hdr);
 		break;
 	case CONTROLVM_CHIPSET_STOP:
-		chipset_notready(&inmsg.hdr);
+		chipset_notready_uevent(&inmsg.hdr);
 		break;
 	default:
 		if (inmsg.hdr.flags.response_expected)
-- 
git-series 0.9.1

[PATCH 05/10] staging: unisys: visorbus: Clarify reason for pointer check in bus_destroy()

[David Kershner]

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

Clarifies why the pointer returned from visorbus_get_device_by_id() in
bus_destroy() is validated. The check is performed in order to be extra
careful, for the sake of added security, that the s-Par backend is
providing us with a valid bus/device pair.

Signed-off-by: David Binder <david.binder@unisys.com>
Signed-off-by: David Kershner <david.kershner@unisys.com>
Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index c90ea6a..2d1b226 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -755,6 +755,7 @@ bus_destroy(struct controlvm_message *inmsg)
 	int err;
 
 	bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
+	/* Validate that s-Par backend gave a good bus */
 	if (!bus_info) {
 		err = -ENODEV;
 		goto err_respond;
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 06/10] staging: unisys: visornic: prevent hang doing 'modprobe -r visornic'

[David Kershner]

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

A stray+extraneous 'netif_napi_add()' that we were doing in
visornic_probe() was causing havoc when we got into visornic_remove(),
called during 'modprobe -r visornic'. The symptom was a processor busy-wait
loop on the modprobe process, which '/proc/<pid>/stack' would show looping
doing napi things.

Presumably the stray line got there as a result of some merging snafoo, and
has been deleted to fix the problem. With this patch 'modprobe -r visornic'
and a subsequent 'modprobe visornic' both complete successfully, and result
in an operational network.

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

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index c44c430..d8f5eca 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1802,7 +1802,7 @@ static int visornic_probe(struct visor_device *dev)
 
 	/* TODO: Setup Interrupt information */
 	/* Let's start our threads to get responses */
-	netif_napi_add(netdev, &devdata->napi, visornic_poll, 64);
+	netif_napi_add(netdev, &devdata->napi, visornic_poll, NAPI_WEIGHT);
 
 	setup_timer(&devdata->irq_poll_timer, poll_for_irq,
 		    (unsigned long)devdata);
@@ -1832,9 +1832,6 @@ static int visornic_probe(struct visor_device *dev)
 		goto cleanup_napi_add;
 	}
 
-	/* Let's start our threads to get responses */
-	netif_napi_add(netdev, &devdata->napi, visornic_poll, NAPI_WEIGHT);
-
 	/* Note: Interrupts have to be enable before the while
 	 * loop below because the napi routine is responsible for
 	 * setting enab_dis_acked
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 07/10] staging: unisys: visorbus: remove putfile data structures

[David Kershner]

There were several unused data structures dealing with putfile that are no
longer being used. So get rid of them.

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

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 2d1b226..74e3349 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -114,60 +114,6 @@ static unsigned long controlvm_payload_bytes_buffered;
 static struct controlvm_message controlvm_pending_msg;
 static bool controlvm_pending_msg_valid;
 
-/*
- * This describes a buffer and its current state of transfer (e.g., how many
- * bytes have already been supplied as putfile data, and how many bytes are
- * remaining) for a putfile_request.
- */
-struct putfile_active_buffer {
-	/* a payload from a controlvm message, containing a file data buffer */
-	struct parser_context *parser_ctx;
-	/* points within data area of parser_ctx to next byte of data */
-	size_t bytes_remaining;
-};
-
-#define PUTFILE_REQUEST_SIG 0x0906101302281211
-/*
- * This identifies a single remote --> local CONTROLVM_TRANSMIT_FILE
- * conversation. Structs of this type are dynamically linked into
- * <Putfile_request_list>.
- */
-struct putfile_request {
-	u64 sig;		/* PUTFILE_REQUEST_SIG */
-
-	/* header from original TransmitFile request */
-	struct controlvm_message_header controlvm_header;
-
-	/* link to next struct putfile_request */
-	struct list_head next_putfile_request;
-
-	/*
-	 * head of putfile_buffer_entry list, which describes the data to be
-	 * supplied as putfile data;
-	 * - this list is added to when controlvm messages come in that supply
-	 * file data
-	 * - this list is removed from via the hotplug program that is actually
-	 * consuming these buffers to write as file data
-	 */
-	struct list_head input_buffer_list;
-	spinlock_t req_list_lock;	/* lock for input_buffer_list */
-
-	/* waiters for input_buffer_list to go non-empty */
-	wait_queue_head_t input_buffer_wq;
-
-	/* data not yet read within current putfile_buffer_entry */
-	struct putfile_active_buffer active_buf;
-
-	/*
-	 * <0 = failed, 0 = in-progress, >0 = successful;
-	 * note that this must be set with req_list_lock, and if you set <0,
-	 * it is your responsibility to also free up all of the other objects
-	 * in this struct (like input_buffer_list, active_buf.parser_ctx)
-	 * before releasing the lock
-	 */
-	int completion_status;
-};
-
 struct parahotplug_request {
 	struct list_head list;
 	int id;
-- 
git-series 0.9.1

[PATCH 08/10] staging: unisys: visorbus: get rid of unused payload info

[David Kershner]

We no longer send payloads back to the s-Par firmware, we can get rid of
the initialize and destroy functions since they weren't actually being
used just created and destroyed.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Reviewed-by: David Binder <david.binder@unisys.com>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 101 +------------------
 1 file changed, 3 insertions(+), 98 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 74e3349..96086f5 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -91,18 +91,6 @@ static struct cdev file_cdev;
 static struct visorchannel **file_controlvm_channel;
 
 static struct visorchannel *controlvm_channel;
-
-/* Manages the request payload in the controlvm channel */
-struct visor_controlvm_payload_info {
-	u8 *ptr;		/* pointer to base address of payload pool */
-	u64 offset;		/*
-				 * offset from beginning of controlvm
-				 * channel to beginning of payload * pool
-				 */
-	u32 bytes;		/* number of bytes in payload pool */
-};
-
-static struct visor_controlvm_payload_info controlvm_payload_info;
 static unsigned long controlvm_payload_bytes_buffered;
 
 /*
@@ -1002,82 +990,6 @@ my_device_destroy(struct controlvm_message *inmsg)
 	return err;
 }
 
-/**
- * initialize_controlvm_payload_info() - init controlvm_payload_info struct
- * @phys_addr: the physical address of controlvm channel
- * @offset:    the offset to payload
- * @bytes:     the size of the payload in bytes
- * @info:      the returning valid struct
- *
- * When provided with the physical address of the controlvm channel
- * (phys_addr), the offset to the payload area we need to manage
- * (offset), and the size of this payload area (bytes), fills in the
- * controlvm_payload_info struct.
- *
- * Return: CONTROLVM_RESP_SUCCESS for success or a negative for failure
- */
-static int
-initialize_controlvm_payload_info(u64 phys_addr, u64 offset, u32 bytes,
-				  struct visor_controlvm_payload_info *info)
-{
-	u8 *payload = NULL;
-
-	if (!info)
-		return -CONTROLVM_RESP_PAYLOAD_INVALID;
-
-	if ((offset == 0) || (bytes == 0))
-		return -CONTROLVM_RESP_PAYLOAD_INVALID;
-
-	payload = memremap(phys_addr + offset, bytes, MEMREMAP_WB);
-	if (!payload)
-		return -CONTROLVM_RESP_IOREMAP_FAILED;
-
-	memset(info, 0, sizeof(struct visor_controlvm_payload_info));
-	info->offset = offset;
-	info->bytes = bytes;
-	info->ptr = payload;
-
-	return CONTROLVM_RESP_SUCCESS;
-}
-
-static void
-destroy_controlvm_payload_info(struct visor_controlvm_payload_info *info)
-{
-	if (info->ptr) {
-		memunmap(info->ptr);
-		info->ptr = NULL;
-	}
-	memset(info, 0, sizeof(struct visor_controlvm_payload_info));
-}
-
-static void
-initialize_controlvm_payload(void)
-{
-	u64 phys_addr = visorchannel_get_physaddr(controlvm_channel);
-	u64 payload_offset = 0;
-	u32 payload_bytes = 0;
-
-	if (visorchannel_read(controlvm_channel,
-			      offsetof(struct spar_controlvm_channel_protocol,
-				       request_payload_offset),
-			      &payload_offset, sizeof(payload_offset)) < 0) {
-		POSTCODE_LINUX(CONTROLVM_INIT_FAILURE_PC, 0, 0,
-			       DIAG_SEVERITY_ERR);
-		return;
-	}
-	if (visorchannel_read(controlvm_channel,
-			      offsetof(struct spar_controlvm_channel_protocol,
-				       request_payload_bytes),
-			      &payload_bytes, sizeof(payload_bytes)) < 0) {
-		POSTCODE_LINUX(CONTROLVM_INIT_FAILURE_PC, 0, 0,
-			       DIAG_SEVERITY_ERR);
-		return;
-	}
-	initialize_controlvm_payload_info(phys_addr,
-					  payload_offset, payload_bytes,
-					  &controlvm_payload_info);
-}
-
 /*
  * The general parahotplug flow works as follows. The visorchipset receives
  * a DEVICE_CHANGESTATE message from Command specifying a physical device
@@ -2057,17 +1969,14 @@ visorchipset_init(struct acpi_device *acpi_device)
 	if (!controlvm_channel)
 		goto error;
 
-	if (SPAR_CONTROLVM_CHANNEL_OK_CLIENT(
-		    visorchannel_get_header(controlvm_channel))) {
-		initialize_controlvm_payload();
-	} else {
+	if (!SPAR_CONTROLVM_CHANNEL_OK_CLIENT(
+				visorchannel_get_header(controlvm_channel)))
 		goto error_destroy_channel;
-	}
 
 	major_dev = MKDEV(visorchipset_major, 0);
 	err = visorchipset_file_init(major_dev, &controlvm_channel);
 	if (err < 0)
-		goto error_destroy_payload;
+		goto error_destroy_channel;
 
 	/* if booting in a crash kernel */
 	if (is_kdump_kernel())
@@ -2103,9 +2012,6 @@ visorchipset_init(struct acpi_device *acpi_device)
 	cancel_delayed_work_sync(&periodic_controlvm_work);
 	visorchipset_file_cleanup(major_dev);
 
-error_destroy_payload:
-	destroy_controlvm_payload_info(&controlvm_payload_info);
-
 error_destroy_channel:
 	visorchannel_destroy(controlvm_channel);
 
@@ -2122,7 +2028,6 @@ visorchipset_exit(struct acpi_device *acpi_device)
 	visorbus_exit();
 
 	cancel_delayed_work_sync(&periodic_controlvm_work);
-	destroy_controlvm_payload_info(&controlvm_payload_info);
 
 	visorchannel_destroy(controlvm_channel);
 
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 09/10] staging: unisys: visorbus: Clarify reason for bus pointer checks

[David Kershner]

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

Clarifies why the pointers returned from visorbus_get_device_by_id() in
visorbus are validated. The check is performed in order to be extra
careful, for the sake of added security, that the s-Par backend is
providing us with a valid bus.

Signed-off-by: David Binder <david.binder@unisys.com>
Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 1 +
 drivers/staging/unisys/visorbus/visorchipset.c  | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index aea1aa2..4a11c89 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -800,6 +800,7 @@ fix_vbus_dev_info(struct visor_device *visordev)
 		return;
 
 	bdev = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
+	/* Validate that s-Par backend gave a good bus */
 	if (!bdev)
 		return;
 	hdr_info = (struct spar_vbus_headerinfo *)bdev->vbus_hdr_info;
diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 96086f5..188e224 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -742,6 +742,7 @@ bus_configure(struct controlvm_message *inmsg,
 		       DIAG_SEVERITY_PRINT);
 
 	bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
+	/* Validate that s-Par backend gave a good bus */
 	if (!bus_info) {
 		POSTCODE_LINUX(BUS_CONFIGURE_FAILURE_PC, 0, bus_no,
 			       DIAG_SEVERITY_ERR);
@@ -796,6 +797,7 @@ my_device_create(struct controlvm_message *inmsg)
 	int err;
 
 	bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
+	/* Validate that s-Par backend gave a good bus */
 	if (!bus_info) {
 		POSTCODE_LINUX(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
 			       DIAG_SEVERITY_ERR);
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 10/10] staging: unisys: visorbus: Clarify reason for device pointer checks

[David Kershner]

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

Clarifies why the pointers returned from visorbus_get_device_by_id() in
visorbus are validated. The check is performed in order to be extra
careful, for the sake of added security, that the s-Par backend is
providing us with a valid device.

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

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index 188e224..07f8d05 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -897,6 +897,7 @@ my_device_changestate(struct controlvm_message *inmsg)
 	int err;
 
 	dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
+	/* Validate that s-Par backend gave a good device */
 	if (!dev_info) {
 		POSTCODE_LINUX(DEVICE_CHANGESTATE_FAILURE_PC, dev_no, bus_no,
 			       DIAG_SEVERITY_ERR);
@@ -957,6 +958,7 @@ my_device_destroy(struct controlvm_message *inmsg)
 	int err;
 
 	dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
+	/* Validate that s-Par backend gave a good device */
 	if (!dev_info) {
 		err = -ENODEV;
 		goto err_respond;
-- 
git-series 0.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/10] staging: unisys: include: fix improper use of dma_data_direction

[Greg KH]

On Wed, Feb 01, 2017 at 05:38:53PM -0500, David Kershner wrote:
> From: Steven Matthews <steven.matthews@unisys.com>
> 
> Replace use of standard Linux dma_data_direction with a Unisys-
> specific uis_dma_data_direction and provide a function to convert
> from the latter to the former.  This is necessary because Unisys
> s-Par depends on the exact format of this field in multiple OSs
> and languages, and so using the standard version creates an
> unnecessary dependency between the kernel and s-Par.
> 
> Signed-off-by: Steven Matthews <steven.matthews@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> ---
>  drivers/staging/unisys/include/iochannel.h      | 11 +++++++--
>  drivers/staging/unisys/visorhba/visorhba_main.c | 22 +++++++++++++++++-
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/unisys/include/iochannel.h b/drivers/staging/unisys/include/iochannel.h
> index 54f4900..6e80462 100644
> --- a/drivers/staging/unisys/include/iochannel.h
> +++ b/drivers/staging/unisys/include/iochannel.h
> @@ -31,7 +31,6 @@
>  
>  #include <linux/uuid.h>
>  
> -#include <linux/dma-direction.h>
>  #include "channel.h"
>  
>  #define ULTRA_VHBA_CHANNEL_PROTOCOL_SIGNATURE ULTRA_CHANNEL_PROTOCOL_SIGNATURE
> @@ -80,6 +79,14 @@
>  /* Size of cdb - i.e., SCSI cmnd */
>  #define MAX_CMND_SIZE 16
>  
> +/* Unisys-specific DMA direction values */
> +enum uis_dma_data_direction {
> +	UIS_DMA_BIDIRECTIONAL = 0,
> +	UIS_DMA_TO_DEVICE,
> +	UIS_DMA_FROM_DEVICE,
> +	UIS_DMA_NONE

An enumerated type that is shared across platforms yet you don't
specifically set what each type is?  That's just begging for nasty bugs
in the future.

Please set all of them so you _know_ what the values are, never trust a
C compiler you haven't written yourself :)

thanks,

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

Re: [PATCH 05/10] staging: unisys: visorbus: Clarify reason for pointer check in bus_destroy()

[Greg KH]

On Wed, Feb 01, 2017 at 05:38:57PM -0500, David Kershner wrote:
> From: David Binder <david.binder@unisys.com>
> 
> Clarifies why the pointer returned from visorbus_get_device_by_id() in
> bus_destroy() is validated. The check is performed in order to be extra
> careful, for the sake of added security, that the s-Par backend is
> providing us with a valid bus/device pair.
> 
> Signed-off-by: David Binder <david.binder@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
> index c90ea6a..2d1b226 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -755,6 +755,7 @@ bus_destroy(struct controlvm_message *inmsg)
>  	int err;
>  
>  	bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
> +	/* Validate that s-Par backend gave a good bus */

I don't remember what I said in my review, but this comment is pretty
useless.

I guess my point is, how could BUS_ROOT_DEVICE ever NOT be a valid
device on the bus?  What would have made it go away?

Comments like this don't make much sense, maybe my review didn't either
:)

thanks,

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

Re: [PATCH 10/10] staging: unisys: visorbus: Clarify reason for device pointer checks

[Greg KH]

On Wed, Feb 01, 2017 at 05:39:02PM -0500, David Kershner wrote:
> From: David Binder <david.binder@unisys.com>
> 
> Clarifies why the pointers returned from visorbus_get_device_by_id() in
> visorbus are validated. The check is performed in order to be extra
> careful, for the sake of added security, that the s-Par backend is
> providing us with a valid device.
> 
> Signed-off-by: David Binder <david.binder@unisys.com>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
> index 188e224..07f8d05 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -897,6 +897,7 @@ my_device_changestate(struct controlvm_message *inmsg)
>  	int err;
>  
>  	dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
> +	/* Validate that s-Par backend gave a good device */

Same comment here as the previous review, this comment really isn't
needed, right?

>  	if (!dev_info) {
>  		POSTCODE_LINUX(DEVICE_CHANGESTATE_FAILURE_PC, dev_no, bus_no,
>  			       DIAG_SEVERITY_ERR);
> @@ -957,6 +958,7 @@ my_device_destroy(struct controlvm_message *inmsg)
>  	int err;
>  
>  	dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
> +	/* Validate that s-Par backend gave a good device */

Or here.

thanks,

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

Re: [PATCH 09/10] staging: unisys: visorbus: Clarify reason for bus pointer checks

[Greg KH]

On Wed, Feb 01, 2017 at 05:39:01PM -0500, David Kershner wrote:
> From: David Binder <david.binder@unisys.com>
> 
> Clarifies why the pointers returned from visorbus_get_device_by_id() in
> visorbus are validated. The check is performed in order to be extra
> careful, for the sake of added security, that the s-Par backend is
> providing us with a valid bus.
> 
> Signed-off-by: David Binder <david.binder@unisys.com>
> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 1 +
>  drivers/staging/unisys/visorbus/visorchipset.c  | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index aea1aa2..4a11c89 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -800,6 +800,7 @@ fix_vbus_dev_info(struct visor_device *visordev)
>  		return;
>  
>  	bdev = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
> +	/* Validate that s-Par backend gave a good bus */

Same as the other reviews...

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

Re: [PATCH 00/10] staging: unisys: additional error handling and channel cleanups.

[Greg KH]

On Wed, Feb 01, 2017 at 05:38:52PM -0500, David Kershner wrote:
> This series fixes a problem where we were using a Linux specific data
> structure in our s-Par channel. These channels are shared across multiple
> OS types and should not use OS specific structures unless absolutely
> needed. It also adds some additional error handling and other cleanup.
> 
> This patchset also introduces some changes being made to the functions that
> invoke the uevents that occur on chipset_ready/selftest/notready messages
> from the s-Par firmware. This simplifies the functions so that we can
> tackle more challenging changes in later patches.

I've applied most of these.

thanks,

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

RE: [PATCH 05/10] staging: unisys: visorbus: Clarify reason for pointer check in bus_destroy()

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, February 02, 2017 7:14 AM
> To: Kershner, David A <David.Kershner@unisys.com>
> Cc: driverdev-devel@linuxdriverproject.org; *S-Par-Maintainer
> <SParMaintainer@unisys.com>; jes.sorensen@gmail.com; Binder, David
> Anthony <David.Binder@unisys.com>
> Subject: Re: [PATCH 05/10] staging: unisys: visorbus: Clarify reason for pointer
> check in bus_destroy()
> 
> On Wed, Feb 01, 2017 at 05:38:57PM -0500, David Kershner wrote:
> > From: David Binder <david.binder@unisys.com>
> >
> > Clarifies why the pointer returned from visorbus_get_device_by_id() in
> > bus_destroy() is validated. The check is performed in order to be extra
> > careful, for the sake of added security, that the s-Par backend is
> > providing us with a valid bus/device pair.
> >
> > Signed-off-by: David Binder <david.binder@unisys.com>
> > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/staging/unisys/visorbus/visorchipset.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> > index c90ea6a..2d1b226 100644
> > --- a/drivers/staging/unisys/visorbus/visorchipset.c
> > +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> > @@ -755,6 +755,7 @@ bus_destroy(struct controlvm_message *inmsg)
> >  	int err;
> >
> >  	bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE,
> NULL);
> > +	/* Validate that s-Par backend gave a good bus */
> 
> I don't remember what I said in my review, but this comment is pretty
> useless.
> 
> I guess my point is, how could BUS_ROOT_DEVICE ever NOT be a valid
> device on the bus?  What would have made it go away?

We are essentially validating "bus_no" here, whose value we just received
from the s-Par backend via a message.  If bus_no is invalid, i.e., we have
no record of a bus with that number ever being created, then
visorbus_get_device_by_id() will return NULL.
 
> 
> Comments like this don't make much sense, maybe my review didn't either
> :)
> 
> thanks,
> 
> greg k-h