All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6 v3]  staging: unisys: visorchipset proc fixes
@ 2014-07-24 18:08 Benjamin Romer
  2014-07-24 18:08 ` [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields Benjamin Romer
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: driverdev-devel, sparmaintainer, Benjamin Romer

This patch set moves several interfaces in the visorchipset module from procfs
to sysfs, and removes the remaining proc interfaces from the module. It includes
documentation for the new interfaces in Documentation/ABI, and some code cleanup
in the new sysfs handler functions.

In version 3 of the set, an unused second parameter was removed from the
chipsetready attribute, and the parahotplug interface was split into two
separate attributes. Additional documentation has been added to the ABI doc
file to describe the use of these interfaces. Formatting mistakes were corrected
and sysfs attribute functions were made static to match their prototypes.

In version 2 of the set, extraneous checks for controlvm_channel pointer 
validity were removed, all newly created attributes use DEVICE_ATTR_RW or 
DEVICE_ATTR_WO, and logging was removed from the show and store functions to 
prevent creating a DOS attack possibility. 


Benjamin Romer (6):
  staging: unisys: move installer to sysfs and split fields
  staging: unisys: move chipsetready to sysfs
  staging: unisys: clean up diagdump proc entry code
  staging: unisys: remove partition information from proc
  staging: unisys: move parahotplug to sysfs
  staging: unisys: ABI documentation for new sysfs entries

 Makefile                                           |   2 +-
 .../Documentation/ABI/sysfs-platform-visorchipset  | 101 +++++
 .../unisys/visorchipset/visorchipset_main.c        | 487 +++++++--------------
 3 files changed, 258 insertions(+), 332 deletions(-)
 create mode 100644 drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset

-- 
1.9.1

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
@ 2014-07-24 18:08 ` Benjamin Romer
  2014-07-25 11:05   ` Dan Carpenter
  2014-07-24 18:08 ` [PATCH 2/6 v3] staging: unisys: move chipsetready to sysfs Benjamin Romer
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: driverdev-devel, sparmaintainer, Benjamin Romer

The installer entry in /proc/visorchipset/installer was composed of three
separate fields as one entry. This patch removes the proc entry and associated
functions, and creates new fields with distinct entries under sysfs in the
visorchipset/install directory. The fields are:

	textid: used to send the ID of a string that should be displayed on
		s-Par's automatic installation progress screen. Setting this
		field when not in installation mode (boottotool was set on
		the previous guest boot) has no effect.

	remaining_steps: used to set the value of the progress bar on the
		s-Par automatic installation progress screen. This field has
		no effect if not in installation mode.

	error: used to send the ID of a string that should be displayed on
		s-Par's automatic installation progress screen when an error
		is encountered during installation. This field has no effect
		if not in installation mode.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
v3: an errant tab was removed and the static keyword was added to the definition
of each sysfs attribute function.
v2: attribute creation was fixed and checks for controlvm_channel pointer were
removed.
 .../unisys/visorchipset/visorchipset_main.c        | 233 ++++++++++-----------
 1 file changed, 106 insertions(+), 127 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 48db6ee..14eb0ab 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -144,16 +144,6 @@ static VISORCHANNEL *ControlVm_channel;
 static ssize_t visorchipset_proc_read_writeonly(struct file *file,
 						char __user *buf,
 						size_t len, loff_t *offset);
-static ssize_t proc_read_installer(struct file *file, char __user *buf,
-				   size_t len, loff_t *offset);
-static ssize_t proc_write_installer(struct file *file,
-				    const char __user *buffer,
-				    size_t count, loff_t *ppos);
-
-static const struct file_operations proc_installer_fops = {
-	.read = proc_read_installer,
-	.write = proc_write_installer,
-};
 
 typedef struct {
 	U8 __iomem *ptr;	/* pointer to base address of payload pool */
@@ -315,9 +305,30 @@ static ssize_t boottotool_store(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count);
 static DEVICE_ATTR_RW(boottotool);
 
+static ssize_t error_show(struct device *dev, struct device_attribute *attr,
+	char *buf);
+static ssize_t error_store(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count);
+static DEVICE_ATTR_RW(error);
+
+static ssize_t textid_show(struct device *dev, struct device_attribute *attr,
+	char *buf);
+static ssize_t textid_store(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count);
+static DEVICE_ATTR_RW(textid);
+
+static ssize_t remaining_steps_show(struct device *dev,
+	struct device_attribute *attr, char *buf);
+static ssize_t remaining_steps_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count);
+static DEVICE_ATTR_RW(remaining_steps);
+
 static struct attribute *visorchipset_install_attrs[] = {
 	&dev_attr_toolaction.attr,
 	&dev_attr_boottotool.attr,
+	&dev_attr_error.attr,
+	&dev_attr_textid.attr,
+	&dev_attr_remaining_steps.attr,
 	NULL
 };
 
@@ -407,6 +418,91 @@ ssize_t boottotool_store(struct device *dev, struct device_attribute *attr,
 	} else
 		return -EIO;
 }
+
+static ssize_t error_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	u32 error;
+
+	visorchannel_read(ControlVm_channel, offsetof(
+		ULTRA_CONTROLVM_CHANNEL_PROTOCOL, InstallationError),
+		&error, sizeof(u32));
+	return scnprintf(buf, PAGE_SIZE, "%i\n", error);
+}
+
+static ssize_t error_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	u32 error;
+
+	if (sscanf(buf, "%i\n", &error) == 1) {
+		if (visorchannel_write(ControlVm_channel,
+			offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
+				InstallationError),
+			&error, sizeof(u32)) == 1) {
+				return count;
+		}
+	}
+	return -EIO;
+}
+
+static ssize_t textid_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	u32 textId;
+
+	visorchannel_read(ControlVm_channel, offsetof(
+		ULTRA_CONTROLVM_CHANNEL_PROTOCOL, InstallationTextId),
+		&textId, sizeof(u32));
+	return scnprintf(buf, PAGE_SIZE, "%i\n", textId);
+}
+
+static ssize_t textid_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	u32 textId;
+
+	if (sscanf(buf, "%i\n", &textId) == 1) {
+		if (visorchannel_write(ControlVm_channel,
+			offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
+				InstallationTextId),
+			&textId, sizeof(u32)) == 1) {
+				return count;
+		}
+	}
+	return -EIO;
+}
+
+
+static ssize_t remaining_steps_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	u16 remainingSteps;
+
+	visorchannel_read(ControlVm_channel,
+		offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
+			InstallationRemainingSteps),
+		&remainingSteps,
+		sizeof(u16));
+	return scnprintf(buf, PAGE_SIZE, "%hu\n", remainingSteps);
+}
+
+static ssize_t remaining_steps_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	u16 remainingSteps;
+
+	if (sscanf(buf, "%hu\n", &remainingSteps) == 1) {
+		if (visorchannel_write(ControlVm_channel,
+			offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
+				InstallationRemainingSteps),
+			&remainingSteps, sizeof(u16)) == 1) {
+				return count;
+		}
+	}
+	return -EIO;
+}
+
 static void
 show_partition_property(struct seq_file *f, void *ctx, int property)
 {
@@ -2326,118 +2422,6 @@ visorchipset_proc_read_writeonly(struct file *file, char __user *buf,
 	return 0;
 }
 
-/**
- * Reads the InstallationError, InstallationTextId,
- * InstallationRemainingSteps fields of ControlVMChannel.
- */
-static ssize_t
-proc_read_installer(struct file *file, char __user *buf,
-		    size_t len, loff_t *offset)
-{
-	int length = 0;
-	U16 remainingSteps;
-	U32 error, textId;
-	char *vbuf;
-	loff_t pos = *offset;
-
-	if (pos < 0)
-		return -EINVAL;
-
-	if (pos > 0 || !len)
-		return 0;
-
-	vbuf = kzalloc(len, GFP_KERNEL);
-	if (!vbuf)
-		return -ENOMEM;
-
-	visorchannel_read(ControlVm_channel,
-			  offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
-				   InstallationRemainingSteps), &remainingSteps,
-			  sizeof(U16));
-	visorchannel_read(ControlVm_channel,
-			  offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
-				   InstallationError), &error, sizeof(U32));
-	visorchannel_read(ControlVm_channel,
-			  offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
-				   InstallationTextId), &textId, sizeof(U32));
-
-	length = sprintf(vbuf, "%u %u %u\n", remainingSteps, error, textId);
-	if (copy_to_user(buf, vbuf, length)) {
-		kfree(vbuf);
-		return -EFAULT;
-	}
-
-	kfree(vbuf);
-	*offset += length;
-	return length;
-}
-
-/**
- * Writes to the InstallationError, InstallationTextId,
- * InstallationRemainingSteps fields of
- * ControlVMChannel.
- * Input: RemainingSteps Error TextId
- * Limit 32 characters input
- */
-#define UINT16_MAX		(65535U)
-#define UINT32_MAX		(4294967295U)
-static ssize_t
-proc_write_installer(struct file *file,
-		     const char __user *buffer, size_t count, loff_t *ppos)
-{
-	char buf[32];
-	U16 remainingSteps;
-	U32 error, textId;
-
-	/* Check to make sure there is no buffer overflow */
-	if (count > (sizeof(buf) - 1))
-		return -EINVAL;
-
-	if (copy_from_user(buf, buffer, count)) {
-		WARN(1, "Error copying from user space\n");
-		return -EFAULT;
-	}
-
-	if (sscanf(buf, "%hu %i %i", &remainingSteps, &error, &textId) != 3) {
-		remainingSteps = UINT16_MAX;
-		error = UINT32_MAX;
-		textId = UINT32_MAX;
-	}
-
-	if (remainingSteps != UINT16_MAX) {
-		if (visorchannel_write
-		    (ControlVm_channel,
-		     offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
-			      InstallationRemainingSteps), &remainingSteps,
-		     sizeof(U16)) < 0)
-			WARN(1, "Installation Status Write Failed - Write function error - RemainingSteps = %d\n",
-			     remainingSteps);
-	}
-
-	if (error != UINT32_MAX) {
-		if (visorchannel_write
-		    (ControlVm_channel,
-		     offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
-			      InstallationError), &error, sizeof(U32)) < 0)
-			WARN(1, "Installation Status Write Failed - Write function error - Error = %d\n",
-			     error);
-	}
-
-	if (textId != UINT32_MAX) {
-		if (visorchannel_write
-		    (ControlVm_channel,
-		     offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
-			      InstallationTextId), &textId, sizeof(U32)) < 0)
-			WARN(1, "Installation Status Write Failed - Write function error - TextId = %d\n",
-			     textId);
-	}
-
-	/* So this function isn't called multiple times, must return
-	 * size of buffer
-	 */
-	return count;
-}
-
 static const struct file_operations chipset_proc_fops = {
 	.owner = THIS_MODULE,
 	.read = visorchipset_proc_read_writeonly,
@@ -2449,7 +2433,6 @@ visorchipset_init(void)
 {
 	int rc = 0, x = 0;
 	char s[64];
-	struct proc_dir_entry *installer_file;
 	HOSTADDRESS addr;
 
 	if (!unisys_spar_platform)
@@ -2523,10 +2506,6 @@ visorchipset_init(void)
 					      PartitionPropertyNames,
 					      &show_partition_property);
 
-	/* Setup Installation fields */
-	installer_file = proc_create("installer", 0644, ProcDir,
-				     &proc_installer_fops);
-
 	memset(&g_DiagMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	chipset_proc_dir = proc_create(VISORCHIPSET_CHIPSET_PROC_ENTRY_FN,
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/6 v3] staging: unisys: move chipsetready to sysfs
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
  2014-07-24 18:08 ` [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields Benjamin Romer
@ 2014-07-24 18:08 ` Benjamin Romer
  2014-07-24 18:08 ` [PATCH 3/6 v3] staging: unisys: clean up diagdump proc entry code Benjamin Romer
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: jkc, driverdev-devel, sparmaintainer, Benjamin Romer

Move the chipsetready proc entry to sysfs under a new directory guest. This
entry is used by Unisys application software on the guest to acknowledge
completion of specific events for integration purposes, but these
acknowledgements are not required for the guest to operate correctly.

The store function is simplified as well, to use scanf() instead of copying
the buffer and using strsep().

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
v3: The second parameter being passed in to the store function was found to be
unnecessary, and it was removed.
v2: attribute creation was fixed and checks for controlvm_channel pointer were
removed. The off-by-one error in the sscanf() was fixed. Error -1 that was 
being returned was changed to -EINVAL.

 .../unisys/visorchipset/visorchipset_main.c        | 76 +++++++---------------
 1 file changed, 24 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 14eb0ab..4211731 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -129,9 +129,6 @@ static MYPROCTYPE *PartitionType;
 #define VISORCHIPSET_DIAG_PROC_ENTRY_FN "diagdump"
 static struct proc_dir_entry *diag_proc_dir;
 
-#define VISORCHIPSET_CHIPSET_PROC_ENTRY_FN "chipsetready"
-static struct proc_dir_entry *chipset_proc_dir;
-
 #define VISORCHIPSET_PARAHOTPLUG_PROC_ENTRY_FN "parahotplug"
 static struct proc_dir_entry *parahotplug_proc_dir;
 
@@ -323,6 +320,10 @@ static ssize_t remaining_steps_store(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count);
 static DEVICE_ATTR_RW(remaining_steps);
 
+static ssize_t chipsetready_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count);
+static DEVICE_ATTR_WO(chipsetready);
+
 static struct attribute *visorchipset_install_attrs[] = {
 	&dev_attr_toolaction.attr,
 	&dev_attr_boottotool.attr,
@@ -337,8 +338,19 @@ static struct attribute_group visorchipset_install_group = {
 	.attrs = visorchipset_install_attrs
 };
 
+static struct attribute *visorchipset_guest_attrs[] = {
+	&dev_attr_chipsetready.attr,
+	NULL
+};
+
+static struct attribute_group visorchipset_guest_group = {
+	.name = "guest",
+	.attrs = visorchipset_guest_attrs
+};
+
 static const struct attribute_group *visorchipset_dev_groups[] = {
 	&visorchipset_install_group,
+	&visorchipset_guest_group,
 	NULL
 };
 
@@ -2369,49 +2381,21 @@ visorchipset_cache_free(struct kmem_cache *pool, void *p, char *fn, int ln)
 	kmem_cache_free(pool, p);
 }
 
-#define gettoken(bufp) strsep(bufp, " -\t\n")
-
-static ssize_t
-chipset_proc_write(struct file *file, const char __user *buffer,
-		   size_t count, loff_t *ppos)
+static ssize_t chipsetready_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
 {
-	char buf[512];
-	char *token, *p;
+	char msgtype[64];
 
-	if (count > sizeof(buf) - 1) {
-		LOGERR("chipset_proc_write: count (%d) exceeds size of buffer (%d)",
-		     (int) count, (int) sizeof(buffer));
-		return -EINVAL;
-	}
-	if (copy_from_user(buf, buffer, count)) {
-		LOGERR("chipset_proc_write: copy_from_user failed");
-		return -EFAULT;
-	}
-	buf[count] = '\0';
-
-	p = buf;
-	token = gettoken(&p);
-
-	if (strcmp(token, "CALLHOMEDISK_MOUNTED") == 0) {
-		token = gettoken(&p);
-		/* The Call Home Disk has been mounted */
-		if (strcmp(token, "0") == 0)
+	if (sscanf(buf, "%63s", msgtype) == 1) {
+		if (strcmp(msgtype, "CALLHOMEDISK_MOUNTED") == 0)
 			chipset_events[0] = 1;
-	} else if (strcmp(token, "MODULES_LOADED") == 0) {
-		token = gettoken(&p);
-		/* All modules for the partition have been loaded */
-		if (strcmp(token, "0") == 0)
+		else if (strcmp(msgtype, "MODULES_LOADED") == 0)
 			chipset_events[1] = 1;
-	} else if (token == NULL) {
-		/* No event specified */
-		LOGERR("No event was specified to send CHIPSET_READY response");
-		return -1;
+		else
+			return -EINVAL;
 	} else {
-		/* Unsupported event specified */
-		LOGERR("%s is an invalid event for sending CHIPSET_READY response",		     token);
-		return -1;
+		return -EINVAL;
 	}
-
 	return count;
 }
 
@@ -2422,12 +2406,6 @@ visorchipset_proc_read_writeonly(struct file *file, char __user *buf,
 	return 0;
 }
 
-static const struct file_operations chipset_proc_fops = {
-	.owner = THIS_MODULE,
-	.read = visorchipset_proc_read_writeonly,
-	.write = chipset_proc_write,
-};
-
 static int __init
 visorchipset_init(void)
 {
@@ -2508,8 +2486,6 @@ visorchipset_init(void)
 
 	memset(&g_DiagMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
-	chipset_proc_dir = proc_create(VISORCHIPSET_CHIPSET_PROC_ENTRY_FN,
-				       0644, ProcDir, &chipset_proc_fops);
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	parahotplug_proc_dir =
@@ -2613,10 +2589,6 @@ visorchipset_exit(void)
 	}
 	memset(&g_DiagMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
-	if (chipset_proc_dir) {
-		remove_proc_entry(VISORCHIPSET_CHIPSET_PROC_ENTRY_FN, ProcDir);
-		chipset_proc_dir = NULL;
-	}
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	if (parahotplug_proc_dir) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/6 v3] staging: unisys: clean up diagdump proc entry code
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
  2014-07-24 18:08 ` [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields Benjamin Romer
  2014-07-24 18:08 ` [PATCH 2/6 v3] staging: unisys: move chipsetready to sysfs Benjamin Romer
@ 2014-07-24 18:08 ` Benjamin Romer
  2014-07-24 18:08 ` [PATCH 4/6 v3] staging: unisys: remove partition information from proc Benjamin Romer
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: jkc, driverdev-devel, sparmaintainer, Benjamin Romer

Remove remnant code left over from the diagdump proc entry.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
v3: patch locations changed due to prior patches being revised.
v2: patch locations changed due to prior patches being revised.

 drivers/staging/unisys/visorchipset/visorchipset_main.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 4211731..58a441d 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -126,9 +126,6 @@ InitPartitionProperties(void)
 
 static MYPROCTYPE *PartitionType;
 
-#define VISORCHIPSET_DIAG_PROC_ENTRY_FN "diagdump"
-static struct proc_dir_entry *diag_proc_dir;
-
 #define VISORCHIPSET_PARAHOTPLUG_PROC_ENTRY_FN "parahotplug"
 static struct proc_dir_entry *parahotplug_proc_dir;
 
@@ -2583,10 +2580,7 @@ visorchipset_exit(void)
 		visor_proc_DestroyType(PartitionType);
 		PartitionType = NULL;
 	}
-	if (diag_proc_dir) {
-		remove_proc_entry(VISORCHIPSET_DIAG_PROC_ENTRY_FN, ProcDir);
-		diag_proc_dir = NULL;
-	}
+
 	memset(&g_DiagMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/6 v3] staging: unisys: remove partition information from proc
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
                   ` (2 preceding siblings ...)
  2014-07-24 18:08 ` [PATCH 3/6 v3] staging: unisys: clean up diagdump proc entry code Benjamin Romer
@ 2014-07-24 18:08 ` Benjamin Romer
  2014-07-24 22:07   ` Greg KH
  2014-07-24 18:08 ` [PATCH 5/6 v3] staging: unisys: move parahotplug to sysfs Benjamin Romer
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: driverdev-devel, sparmaintainer, Benjamin Romer

Debugging information for the guest's channels was being exposed in proc.
Remove the code that creates these entries, which are no longer needed.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
v3: patch location changed due to prior patches being revised.
v2: patch location changed due to prior patches being revised.

 Makefile                                           |  2 +-
 .../unisys/visorchipset/visorchipset_main.c        | 99 ----------------------
 2 files changed, 1 insertion(+), 100 deletions(-)

diff --git a/Makefile b/Makefile
index f3c543d..a1c224f 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 VERSION = 3
 PATCHLEVEL = 16
 SUBLEVEL = 0
-EXTRAVERSION = -rc5
+EXTRAVERSION = -bmr4
 NAME = Shuffling Zombie Juror
 
 # *DOCUMENTATION*
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 58a441d..13f4da8 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -98,34 +98,6 @@ static CONTROLVM_MESSAGE_PACKET g_DeviceChangeStatePacket;
 #define is_diagpool_channel(channel_type_guid) \
 	 (uuid_le_cmp(channel_type_guid, UltraDiagPoolChannelProtocolGuid) == 0)
 
-typedef enum {
-	PARTPROP_invalid,
-	PARTPROP_name,
-	PARTPROP_description,
-	PARTPROP_handle,
-	PARTPROP_busNumber,
-	/* add new properties above, but don't forget to change
-	 * InitPartitionProperties() and show_partition_property() also...
-	 */
-	PARTPROP_last
-} PARTITION_property;
-static const char *PartitionTypeNames[] = { "partition", NULL };
-
-static char *PartitionPropertyNames[PARTPROP_last + 1];
-static void
-InitPartitionProperties(void)
-{
-	char **p = PartitionPropertyNames;
-	p[PARTPROP_invalid] = "";
-	p[PARTPROP_name] = "name";
-	p[PARTPROP_description] = "description";
-	p[PARTPROP_handle] = "handle";
-	p[PARTPROP_busNumber] = "busNumber";
-	p[PARTPROP_last] = NULL;
-}
-
-static MYPROCTYPE *PartitionType;
-
 #define VISORCHIPSET_PARAHOTPLUG_PROC_ENTRY_FN "parahotplug"
 static struct proc_dir_entry *parahotplug_proc_dir;
 
@@ -512,52 +484,6 @@ static ssize_t remaining_steps_store(struct device *dev,
 	return -EIO;
 }
 
-static void
-show_partition_property(struct seq_file *f, void *ctx, int property)
-{
-	VISORCHIPSET_BUS_INFO *info = (VISORCHIPSET_BUS_INFO *) (ctx);
-
-	switch (property) {
-	case PARTPROP_name:
-		seq_printf(f, "%s\n", NONULLSTR(info->name));
-		break;
-	case PARTPROP_description:
-		seq_printf(f, "%s\n", NONULLSTR(info->description));
-		break;
-	case PARTPROP_handle:
-		seq_printf(f, "0x%-16.16Lx\n", info->partitionHandle);
-		break;
-	case PARTPROP_busNumber:
-		seq_printf(f, "%d\n", info->busNo);
-		break;
-	default:
-		seq_printf(f, "(%d??)\n", property);
-		break;
-	}
-}
-
-static void
-proc_Init(void)
-{
-	if (ProcDir == NULL) {
-		ProcDir = proc_mkdir(MYDRVNAME, NULL);
-		if (ProcDir == NULL) {
-			LOGERR("failed to create /proc directory %s",
-			       MYDRVNAME);
-			POSTCODE_LINUX_2(CHIPSET_INIT_FAILURE_PC,
-					 POSTCODE_SEVERITY_ERR);
-		}
-	}
-}
-
-static void
-proc_DeInit(void)
-{
-	if (ProcDir != NULL)
-		remove_proc_entry(MYDRVNAME, NULL);
-	ProcDir = NULL;
-}
-
 #if 0
 static void
 testUnicode(void)
@@ -1260,16 +1186,6 @@ bus_configure(CONTROLVM_MESSAGE *inmsg, PARSER_CONTEXT *parser_ctx)
 	pBusInfo->name = parser_string_get(parser_ctx);
 
 	visorchannel_uuid_id(&pBusInfo->partitionGuid, s);
-	pBusInfo->procObject =
-	    visor_proc_CreateObject(PartitionType, s, (void *) (pBusInfo));
-	if (pBusInfo->procObject == NULL) {
-		LOGERR("CONTROLVM_BUS_CONFIGURE Failed: busNo=%lu failed to create /proc entry",
-		     busNo);
-		POSTCODE_LINUX_3(BUS_CONFIGURE_FAILURE_PC, busNo,
-				 POSTCODE_SEVERITY_ERR);
-		rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
-		goto Away;
-	}
 	POSTCODE_LINUX_3(BUS_CONFIGURE_EXIT_PC, busNo, POSTCODE_SEVERITY_INFO);
 Away:
 	bus_epilog(busNo, CONTROLVM_BUS_CONFIGURE, &inmsg->hdr,
@@ -2472,15 +2388,6 @@ visorchipset_init(void)
 		goto Away;
 	}
 
-	proc_Init();
-	memset(PartitionPropertyNames, 0, sizeof(PartitionPropertyNames));
-	InitPartitionProperties();
-
-	PartitionType = visor_proc_CreateType(ProcDir, PartitionTypeNames,
-					      (const char **)
-					      PartitionPropertyNames,
-					      &show_partition_property);
-
 	memset(&g_DiagMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
@@ -2576,11 +2483,6 @@ visorchipset_exit(void)
 
 	cleanup_controlvm_structures();
 
-	if (PartitionType) {
-		visor_proc_DestroyType(PartitionType);
-		PartitionType = NULL;
-	}
-
 	memset(&g_DiagMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
@@ -2593,7 +2495,6 @@ visorchipset_exit(void)
 
 	memset(&g_DelDumpMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
-	proc_DeInit();
 	LOGINF("Channel %s (ControlVm) disconnected",
 	       visorchannel_id(ControlVm_channel, s));
 	visorchannel_destroy(ControlVm_channel);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/6 v3] staging: unisys: move parahotplug to sysfs
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
                   ` (3 preceding siblings ...)
  2014-07-24 18:08 ` [PATCH 4/6 v3] staging: unisys: remove partition information from proc Benjamin Romer
@ 2014-07-24 18:08 ` Benjamin Romer
  2014-07-25 11:27   ` Dan Carpenter
  2014-07-24 18:08 ` Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries Benjamin Romer
  2014-07-24 22:08 ` [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Greg KH
  6 siblings, 1 reply; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: driverdev-devel, sparmaintainer, Benjamin Romer

Move the /proc/visorchipset/parahotplug interface to sysfs under
/sys/devices/platform/visorchipset/parahotplug/deviceenabled and
/sys/devices/platform/visorchipset/parahotplug/devicedisabled.

The parahotplug interface is used to deal with SR-IOV recovery situations on
s-Par guest partitions. The command service partition will send a message to a
guest when an SR-IOV device that guest is using needs to be temporarily removed.
The message triggers a udev event that will cause a recovery script to run.
When that script has completed its work, it will write to one of the
parahotplug interfaces to send a message back to Command, indicating that the
recovery action has completed.

When a guest that is sharing an SR-IOV device is restarted, that guest will
take down the PF driver on the device, but any guests with VFs will not know
that their device needs to be reset as well. The recovery script makes it so
the device will be shut down fully and then restarted after the sharing guest
comes back up, and ensures that the timing is correct.

Moving this interface to sysfs orphans the visorchipset_proc_read_writeonly()
function, so it is also removed.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
v3: The interface was split in two, so only the ID need be passed as a parameter
to use the interfaces. Additional documentation was added.
v2: attribute creation was fixed and checks for controlvm_channel pointer were
removed.
 .../unisys/visorchipset/visorchipset_main.c        | 99 +++++++++-------------
 1 file changed, 39 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 13f4da8..7a7f515 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -98,19 +98,11 @@ static CONTROLVM_MESSAGE_PACKET g_DeviceChangeStatePacket;
 #define is_diagpool_channel(channel_type_guid) \
 	 (uuid_le_cmp(channel_type_guid, UltraDiagPoolChannelProtocolGuid) == 0)
 
-#define VISORCHIPSET_PARAHOTPLUG_PROC_ENTRY_FN "parahotplug"
-static struct proc_dir_entry *parahotplug_proc_dir;
-
 static LIST_HEAD(BusInfoList);
 static LIST_HEAD(DevInfoList);
 
-static struct proc_dir_entry *ProcDir;
 static VISORCHANNEL *ControlVm_channel;
 
-static ssize_t visorchipset_proc_read_writeonly(struct file *file,
-						char __user *buf,
-						size_t len, loff_t *offset);
-
 typedef struct {
 	U8 __iomem *ptr;	/* pointer to base address of payload pool */
 	U64 offset;		/* offset from beginning of controlvm
@@ -293,6 +285,14 @@ static ssize_t chipsetready_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count);
 static DEVICE_ATTR_WO(chipsetready);
 
+static ssize_t devicedisabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count);
+static DEVICE_ATTR_WO(devicedisabled);
+
+static ssize_t deviceenabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count);
+static DEVICE_ATTR_WO(deviceenabled);
+
 static struct attribute *visorchipset_install_attrs[] = {
 	&dev_attr_toolaction.attr,
 	&dev_attr_boottotool.attr,
@@ -317,9 +317,21 @@ static struct attribute_group visorchipset_guest_group = {
 	.attrs = visorchipset_guest_attrs
 };
 
+static struct attribute *visorchipset_parahotplug_attrs[] = {
+	&dev_attr_devicedisabled.attr,
+	&dev_attr_deviceenabled.attr,
+	NULL
+};
+
+static struct attribute_group visorchipset_parahotplug_group = {
+	.name = "parahotplug",
+	.attrs = visorchipset_parahotplug_attrs
+};
+
 static const struct attribute_group *visorchipset_dev_groups[] = {
 	&visorchipset_install_group,
 	&visorchipset_guest_group,
+	&visorchipset_parahotplug_group,
 	NULL
 };
 
@@ -1723,53 +1735,36 @@ parahotplug_process_message(CONTROLVM_MESSAGE *inmsg)
 	}
 }
 
-/*
- * Gets called when the udev script writes to
- * /proc/visorchipset/parahotplug.  Expects input in the form of "<id>
- * <active>" where <id> is the identifier passed to the script that
- * matches a request on the request list, and <active> is 0 or 1
- * indicating whether the device is now enabled or not.
+/* The parahotplug/devicedisabled interface gets called by our support script
+ * when an SR-IOV device has been shut down. The ID is passed to the script
+ * and then passed back when the device has been removed.
  */
-static ssize_t
-parahotplug_proc_write(struct file *file, const char __user *buffer,
-		       size_t count, loff_t *ppos)
+ssize_t devicedisabled_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
 {
-	char buf[64];
 	uint id;
-	ushort active;
 
-	if (count > sizeof(buf) - 1) {
-		LOGERR("parahotplug_proc_write: count (%d) exceeds size of buffer (%d)",
-		     (int) count, (int) sizeof(buf));
+	if (kstrtouint(buf, 10, &id) != 1)
 		return -EINVAL;
-	}
-	if (copy_from_user(buf, buffer, count)) {
-		LOGERR("parahotplug_proc_write: copy_from_user failed");
-		return -EFAULT;
-	}
-	buf[count] = '\0';
+	parahotplug_request_complete((int) id, 0);
+	return count;
+}
 
-	if (sscanf(buf, "%u %hu", &id, &active) != 2) {
-		id = 0;
-		active = 0;
-	}
+/* The parahotplug/deviceenabled interface gets called by our support script
+ * when an SR-IOV device has been recovered. The ID is passed to the script
+ * and then passed back when the device has been brought back up.
+ */
+ssize_t deviceenabled_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	uint id;
 
-	if (active != 1 && active != 0) {
-		LOGERR("parahotplug_proc_write: invalid active field");
+	if (kstrtouint(buf, 10, &id) != 1)
 		return -EINVAL;
-	}
-
-	parahotplug_request_complete((int) id, (U16) active);
-
+	parahotplug_request_complete((int) id, 1);
 	return count;
 }
 
-static const struct file_operations parahotplug_proc_fops = {
-	.owner = THIS_MODULE,
-	.read = visorchipset_proc_read_writeonly,
-	.write = parahotplug_proc_write,
-};
-
 /* Process a controlvm message.
  * Return result:
  *    FALSE - this function will return FALSE only in the case where the
@@ -2312,13 +2307,6 @@ static ssize_t chipsetready_store(struct device *dev,
 	return count;
 }
 
-static ssize_t
-visorchipset_proc_read_writeonly(struct file *file, char __user *buf,
-				 size_t len, loff_t *offset)
-{
-	return 0;
-}
-
 static int __init
 visorchipset_init(void)
 {
@@ -2392,9 +2380,6 @@ visorchipset_init(void)
 
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
-	parahotplug_proc_dir =
-	    proc_create(VISORCHIPSET_PARAHOTPLUG_PROC_ENTRY_FN, 0200,
-			ProcDir, &parahotplug_proc_fops);
 	memset(&g_DelDumpMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	Putfile_buffer_list_pool =
@@ -2487,12 +2472,6 @@ visorchipset_exit(void)
 
 	memset(&g_ChipSetMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
-	if (parahotplug_proc_dir) {
-		remove_proc_entry(VISORCHIPSET_PARAHOTPLUG_PROC_ENTRY_FN,
-				  ProcDir);
-		parahotplug_proc_dir = NULL;
-	}
-
 	memset(&g_DelDumpMsgHdr, 0, sizeof(CONTROLVM_MESSAGE_HEADER));
 
 	LOGINF("Channel %s (ControlVm) disconnected",
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
                   ` (4 preceding siblings ...)
  2014-07-24 18:08 ` [PATCH 5/6 v3] staging: unisys: move parahotplug to sysfs Benjamin Romer
@ 2014-07-24 18:08 ` Benjamin Romer
  2014-07-24 18:14   ` Romer, Benjamin M
  2014-07-24 22:08 ` [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Greg KH
  6 siblings, 1 reply; 14+ messages in thread
From: Benjamin Romer @ 2014-07-24 18:08 UTC (permalink / raw)
  To: gregkh; +Cc: jkc, driverdev-devel, sparmaintainer, Benjamin Romer

This patch adds a documentation file for all of the interfaces that were moved
to sysfs by the other patches in this set.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
v3: documentation was revised based on changes in earlier patches.
v2: whitespace errors were corrected.

 .../Documentation/ABI/sysfs-platform-visorchipset  | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset

diff --git a/drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset b/drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset
new file mode 100644
index 0000000..28f8f12
--- /dev/null
+++ b/drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset
@@ -0,0 +1,101 @@
+What:		install/error
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	used to send the ID of a string that should be displayed on
+		s-Par's automatic installation progress screen when an error
+		is encountered during installation. This field has no effect
+		if not in installation mode.
+Users:		sparmaintainer@unisys.com
+
+What:		install/remainingsteps
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	used to set the value of the progress bar on the s-Par automatic
+		installation progress screen. This field has no effect if not in
+		installation mode.
+Users:		sparmaintainer@unisys.com
+
+What:		install/textid
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	used to send the ID of a string that should be displayed on
+		s-Par's automatic installation progress screen. Setting this
+		field when not in installation mode (boottotool was set on
+		the previous guest boot) has no effect.
+Users:		sparmaintainer@unisys.com
+
+What:		install/boottotool
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	The boottotool flag controls s-Par behavior on the next boot of
+		this guest. Setting the flag will cause the guest to boot from
+		the utility and installation image, which will use the value in
+		the toolaction field to determine what operation is being
+		requested.
+Users:		sparmaintainer@unisys.com
+
+What:		install/toolaction
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	This field is used to tell s-Par which type of recovery tool
+		action to perform on the next guest boot-up. The meaning of the
+		value is dependent on the type of installation software used to
+		commission the guest.
+Users:		sparmaintainer@unisys.com
+
+What:		guest/chipsetready
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	This entry is used by Unisys application software on the guest
+		to acknowledge completion of specific events for integration
+		purposes, but these acknowledgements are not required for the
+		guest to operate correctly. The interface accepts one of two
+		strings: MODULES_LOADED to indicate that the s-Par driver
+		modules have been loaded successfully, or CALLHOMEDISK_MOUNTED,
+		which indicates that the disk used to support call home services
+		has been successfully mounted.
+Users:		sparmaintainer@unisys.com
+
+What:		parahotplug/deviceenabled
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	This entry is used by a Unisys support script installed on the
+		guest, and triggered by a udev event. The support script is
+		responsible for enabling and disabling SR-IOV devices when the
+		PF device is being recovered in another guest.
+
+		Some SR-IOV devices have problems when the PF is reset without
+		first disabling all VFs attached to that PF. s-Par handles this
+		situation by sending a message to guests using these VFs, and
+		the script will disable the device. When the PF is recovered,
+		another message is sent to the guests to re-enable the VFs.
+
+		The parahotplug/deviceenabled interface is used to acknowledge
+		the recovery message.
+Users:		sparmaintainer@unisys.com
+
+What:		parahotplug/devicedisabled
+Date:		7/18/2014
+KernelVersion: 	TBD
+Contact:	sparmaintainer@unisys.com
+Description:	This entry is used by a Unisys support script installed on the
+		guest, and triggered by a udev event. The support script is
+		responsible for enabling and disabling SR-IOV devices when the
+		PF device is being recovered in another guest.
+
+		Some SR-IOV devices have problems when the PF is reset without
+		first disabling all VFs attached to that PF. s-Par handles this
+		situation by sending a message to guests using these VFs, and
+		the script will disable the device. When the PF is recovered,
+		another message is sent to the guests to re-enable the VFs.
+
+		The parahotplug/devicedisaabled interface is used to acknowledge
+		the initial recovery message.
+Users:		sparmaintainer@unisys.com
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries
  2014-07-24 18:08 ` Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries Benjamin Romer
@ 2014-07-24 18:14   ` Romer, Benjamin M
  0 siblings, 0 replies; 14+ messages in thread
From: Romer, Benjamin M @ 2014-07-24 18:14 UTC (permalink / raw)
  To: gregkh; +Cc: driverdev-devel, *S-Par-Maintainer

Please disregard this specific email - "Subject: [PATCH 6/6 v3] staging:
unisys: ABI documentation for new sysfs entries" - the subject was
corrected and this individual patch was resent.

Sorry for any confusion.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/6 v3] staging: unisys: remove partition information from proc
  2014-07-24 18:08 ` [PATCH 4/6 v3] staging: unisys: remove partition information from proc Benjamin Romer
@ 2014-07-24 22:07   ` Greg KH
  2014-07-24 23:03     ` Ben Romer
  0 siblings, 1 reply; 14+ messages in thread
From: Greg KH @ 2014-07-24 22:07 UTC (permalink / raw)
  To: Benjamin Romer; +Cc: sparmaintainer, driverdev-devel

On Thu, Jul 24, 2014 at 02:08:45PM -0400, Benjamin Romer wrote:
> Debugging information for the guest's channels was being exposed in proc.
> Remove the code that creates these entries, which are no longer needed.
> 
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> ---
> v3: patch location changed due to prior patches being revised.
> v2: patch location changed due to prior patches being revised.
> 
>  Makefile                                           |  2 +-
>  .../unisys/visorchipset/visorchipset_main.c        | 99 ----------------------
>  2 files changed, 1 insertion(+), 100 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f3c543d..a1c224f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,7 +1,7 @@
>  VERSION = 3
>  PATCHLEVEL = 16
>  SUBLEVEL = 0
> -EXTRAVERSION = -rc5
> +EXTRAVERSION = -bmr4
>  NAME = Shuffling Zombie Juror
>  
>  # *DOCUMENTATION*


Really????


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/6 v3]  staging: unisys: visorchipset proc fixes
  2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
                   ` (5 preceding siblings ...)
  2014-07-24 18:08 ` Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries Benjamin Romer
@ 2014-07-24 22:08 ` Greg KH
  6 siblings, 0 replies; 14+ messages in thread
From: Greg KH @ 2014-07-24 22:08 UTC (permalink / raw)
  To: Benjamin Romer; +Cc: sparmaintainer, driverdev-devel

On Thu, Jul 24, 2014 at 02:08:41PM -0400, Benjamin Romer wrote:
> This patch set moves several interfaces in the visorchipset module from procfs
> to sysfs, and removes the remaining proc interfaces from the module. It includes
> documentation for the new interfaces in Documentation/ABI, and some code cleanup
> in the new sysfs handler functions.
> 
> In version 3 of the set, an unused second parameter was removed from the
> chipsetready attribute, and the parahotplug interface was split into two
> separate attributes. Additional documentation has been added to the ABI doc
> file to describe the use of these interfaces. Formatting mistakes were corrected
> and sysfs attribute functions were made static to match their prototypes.
> 
> In version 2 of the set, extraneous checks for controlvm_channel pointer 
> validity were removed, all newly created attributes use DEVICE_ATTR_RW or 
> DEVICE_ATTR_WO, and logging was removed from the show and store functions to 
> prevent creating a DOS attack possibility. 

I did not apply patches 4 and 5 for the obvious reason that 4 is a crazy
patch...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/6 v3] staging: unisys: remove partition information from proc
  2014-07-24 22:07   ` Greg KH
@ 2014-07-24 23:03     ` Ben Romer
  0 siblings, 0 replies; 14+ messages in thread
From: Ben Romer @ 2014-07-24 23:03 UTC (permalink / raw)
  To: gregkh, Benjamin.Romer; +Cc: driverdev-devel, SParMaintainer

Greg KH <gregkh@linuxfoundation.org> wrote:

>
> Really????
>

Obviously not, I'm sorry. :(
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields
  2014-07-24 18:08 ` [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields Benjamin Romer
@ 2014-07-25 11:05   ` Dan Carpenter
  2014-07-25 13:41     ` Romer, Benjamin M
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2014-07-25 11:05 UTC (permalink / raw)
  To: Benjamin Romer; +Cc: gregkh, sparmaintainer, driverdev-devel

On Thu, Jul 24, 2014 at 02:08:42PM -0400, Benjamin Romer wrote:
> +static ssize_t error_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	u32 error;
> +
> +	if (sscanf(buf, "%i\n", &error) == 1) {
> +		if (visorchannel_write(ControlVm_channel,
> +			offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
> +				InstallationError),
> +			&error, sizeof(u32)) == 1) {

visorchannel_write() returns either 0 or -EFAULT so this condition is
never true.  This bug occurs in several places.

> +				return count;
> +		}
> +	}
> +	return -EIO;
> +}

It's almost always better to have error handling instead of success
handling.  It should be a string of commands in a row and so we don't
have nested if statements.

	ret = foo();
	if (ret)
		return ret;

	ret = bar();
	if (ret)
		return ret;

	ret = baz();
	if (ret)
		return ret;

	ret = fizzbuzz();
	if (ret)
		return ret;

Look, Ma, no indenting!  Also -EIO is wrong-ish.  visorchannel_write()
should probably return -EIO instead of -EFAULT.  Do it like this:

static ssize_t error_store(struct device *dev, struct device_attribute *attr,
		const char *buf, size_t count)
{
	u32 error;
	int ret;

	if (sscanf(buf, "%u\n", &error) != 1)
		return -EINVAL;

	ret = visorchannel_write(ControlVm_channel,
				 offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
			         	  InstallationError),
				 &error, sizeof(error));
	if (ret)
		return ret;

	return count;
}

regards,
dan carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/6 v3] staging: unisys: move parahotplug to sysfs
  2014-07-24 18:08 ` [PATCH 5/6 v3] staging: unisys: move parahotplug to sysfs Benjamin Romer
@ 2014-07-25 11:27   ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-07-25 11:27 UTC (permalink / raw)
  To: Benjamin Romer; +Cc: gregkh, sparmaintainer, driverdev-devel

On Thu, Jul 24, 2014 at 02:08:46PM -0400, Benjamin Romer wrote:
> +	if (kstrtouint(buf, 10, &id) != 1)
>  		return -EINVAL;

Obviously this doesn't work.  Btw, you should update your test suite to
prevent this kind of embarassing typo in the future.  ;)

Also I was reviewing enable_ints_write() earlier today and it does:

	ret = kstrtouint(buf, 10, &new_value);
	if (ret != 0)
		return ret;

Don't do the "!= 0", it's a double negative and it doesn't contribute
anything.  Just do "if (ret) ".

regards,
dan carpenter


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields
  2014-07-25 11:05   ` Dan Carpenter
@ 2014-07-25 13:41     ` Romer, Benjamin M
  0 siblings, 0 replies; 14+ messages in thread
From: Romer, Benjamin M @ 2014-07-25 13:41 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: gregkh, *S-Par-Maintainer, driverdev-devel

On Fri, 2014-07-25 at 14:05 +0300, Dan Carpenter wrote:

> visorchannel_write() returns either 0 or -EFAULT so this condition is
> never true.  This bug occurs in several places.

Thank you for catching my mistake. I'll fix this and change
visorchannel_write() to return -EIO as you suggested.

> Look, Ma, no indenting!  Also -EIO is wrong-ish.  visorchannel_write()
> should probably return -EIO instead of -EFAULT.  Do it like this:

Will do. I'll also check for additional places where we can simplify the
code like that.

Thank you for the help!! :)

-- 
Ben Romer | Software Engineer |
Virtual Systems Development 

Unisys Corporation |  2476
Swedesford Rd |  Malvern, PA 19355
|  610-648-7140



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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2014-07-25 13:41 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-24 18:08 [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Benjamin Romer
2014-07-24 18:08 ` [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields Benjamin Romer
2014-07-25 11:05   ` Dan Carpenter
2014-07-25 13:41     ` Romer, Benjamin M
2014-07-24 18:08 ` [PATCH 2/6 v3] staging: unisys: move chipsetready to sysfs Benjamin Romer
2014-07-24 18:08 ` [PATCH 3/6 v3] staging: unisys: clean up diagdump proc entry code Benjamin Romer
2014-07-24 18:08 ` [PATCH 4/6 v3] staging: unisys: remove partition information from proc Benjamin Romer
2014-07-24 22:07   ` Greg KH
2014-07-24 23:03     ` Ben Romer
2014-07-24 18:08 ` [PATCH 5/6 v3] staging: unisys: move parahotplug to sysfs Benjamin Romer
2014-07-25 11:27   ` Dan Carpenter
2014-07-24 18:08 ` Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries Benjamin Romer
2014-07-24 18:14   ` Romer, Benjamin M
2014-07-24 22:08 ` [PATCH 0/6 v3] staging: unisys: visorchipset proc fixes Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.