[PATCH 0/2 v4] staging: unisys: visorchipset proc fixes

[PATCH 0/2 v4] staging: unisys: visorchipset proc fixes

[Benjamin Romer]

This patch set moves the parahotplug interface in the visorchipset module from
procfs to sysfs, and removes the visorchipset/partition tree from proc. It also
includes some code cleanup in the new sysfs handler functions.

In version 4 of the set, the accidental inclusion of a change to the Makefile
made for testing was fixed.

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 (2):
  staging: unisys: remove partition information from proc
  staging: unisys: move parahotplug to sysfs

 .../unisys/visorchipset/visorchipset_main.c        | 198 ++++-----------------
 1 file changed, 39 insertions(+), 159 deletions(-)

-- 
1.9.1

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

[PATCH 1/2 v4] staging: unisys: remove partition information from proc

[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>
---
v4: this patch accidentally picked up a Makefile change intended only for
testing. The change was removed.
v3: patch location changed due to prior patches being revised.
v2: patch location changed due to prior patches being revised.
 .../unisys/visorchipset/visorchipset_main.c        | 99 ----------------------
 1 file changed, 99 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 6f87e274197c..156a72fdabd9 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;
 
@@ -525,52 +497,6 @@ static ssize_t remaining_steps_store(struct device *dev,
 		return count;
 }
 
-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)
@@ -1273,16 +1199,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,
@@ -2485,15 +2401,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));
@@ -2589,11 +2496,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));
@@ -2606,7 +2508,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);

[PATCH 2/2 v4] staging: unisys: move parahotplug to sysfs

[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>
---
v4: this patch is unchanged from v3.
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

Re: [PATCH 2/2 v4] staging: unisys: move parahotplug to sysfs

[Greg KH]

On Thu, Jul 24, 2014 at 10:55:09PM -0400, Benjamin Romer wrote:
> 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>
> ---
> v4: this patch is unchanged from v3.
> 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)

static?

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