Re: [PATCH 02/10] staging: unisys: add toolaction to sysfs

2014-07-16 Thread Romer, Benjamin M 
On Tue, 2014-07-15 at 21:50 -0700, Greg KH wrote:
 On Tue, Jul 15, 2014 at 01:30:42PM -0400, Benjamin Romer wrote:
 All sysfs files need a Documentation/ABI/ entry.  As this isn't in the
 real part of the kernel yet, just create the entries in the unisys/
 subdir and then when it moves out, we can move them to the right place
 in the tree.
 
 Can you redo this patch with that entry?
 

Of course. I'll add an ABI-documentation file to our subdir with all of
the documentation as the first patch in my resubmission.

 How can this not be true?  I tried to follow the code here, but it's
 horrid, why is this assigned in a work queue and not when the module
 starts up?

I'm not sure of the history or original purpose for the delayed
initialization, and it also looks like there are two different ways
defined in separate files for the channel to be assigned an address, so
what's there now is definitely overcomplicated. In addition to the
initialization fix, I'll move the channel address function into this
file and remove the other files to simplify things further.

  +   U8 toolAction;
  +
  +   visorchannel_read(ControlVm_channel,
  +   offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
  +  ToolAction), toolAction, sizeof(U8));
  +   return scnprintf(buf, PAGE_SIZE, %u\n, toolAction);
  +   } else
  +   return -ENODEV;
  +}
 
 Why would this sysfs file be created if there is not a device?  That's
 not how sysfs files work, it should only be present if the file has a
 value, and if not, it shouldn't be there.

I'll clean this up so that it works that way, and remove the extraneous
checks for the channel address. 

 Same goes for your other sysfs files in this patchset, you should never
 be return -ENODEV, you just shouldn't have created the file in the first
 place.  That makes your kernel code simpler, and hopefully, your
 userspace code easier too.

It will. :) I'll do a cleanup patch set first and then redo the proc
changeover set after that.

 thanks,
 
 greg k-h

-- 
Ben Romer | Software Engineer |
Virtual Systems Development 

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



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

[PATCH 02/10] staging: unisys: add toolaction to sysfs

2014-07-15 Thread Benjamin Romer 
Move the proc entry for controlling the toolaction field to sysfs. The field
appears in /sys/devices/platform/visorchipset/install/toolaction.

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.

Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com
---
 .../unisys/visorchipset/visorchipset_main.c| 157 -
 1 file changed, 60 insertions(+), 97 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c 
b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index ecbeaec..d30c365 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -150,11 +150,6 @@ static ssize_t proc_read_installer(struct file *file, char 
__user *buf,
 static ssize_t proc_write_installer(struct file *file,
const char __user *buffer,
size_t count, loff_t *ppos);
-static ssize_t proc_read_toolaction(struct file *file, char __user *buf,
-   size_t len, loff_t *offset);
-static ssize_t proc_write_toolaction(struct file *file,
-const char __user *buffer,
-size_t count, loff_t *ppos);
 static ssize_t proc_read_bootToTool(struct file *file, char __user *buf,
size_t len, loff_t *offset);
 static ssize_t proc_write_bootToTool(struct file *file,
@@ -165,11 +160,6 @@ static const struct file_operations proc_installer_fops = {
.write = proc_write_installer,
 };
 
-static const struct file_operations proc_toolaction_fops = {
-   .read = proc_read_toolaction,
-   .write = proc_write_toolaction,
-};
-
 static const struct file_operations proc_bootToTool_fops = {
.read = proc_read_bootToTool,
.write = proc_write_bootToTool,
@@ -322,10 +312,36 @@ static VISORCHIPSET_BUSDEV_RESPONDERS BusDev_Responders = 
{
 /* info for /dev/visorchipset */
 static dev_t MajorDev = -1; /** indicates major num for device */
 
+/* prototypes for attributes */
+static ssize_t show_toolaction(struct device *dev,
+   struct device_attribute *attr, char *buf);
+
+static ssize_t store_toolaction(struct device *dev,
+   struct device_attribute *attr, const char *buf, size_t count);
+
+static DEVICE_ATTR(toolaction, S_IRUSR | S_IWUSR, show_toolaction,
+   store_toolaction);
+
+static struct attribute *visorchipset_install_attrs[] = {
+   dev_attr_toolaction.attr,
+   NULL
+};
+
+static struct attribute_group visorchipset_install_group = {
+   .name = install,
+   .attrs = visorchipset_install_attrs
+};
+
+static const struct attribute_group *visorchipset_dev_groups[] = {
+   visorchipset_install_group,
+   NULL
+};
+
 /* /sys/devices/platform/visorchipset */
 static struct platform_device Visorchipset_platform_device = {
.name = visorchipset,
.id = -1,
+   .dev.groups = visorchipset_dev_groups,
 };
 
 /* Function prototypes */
@@ -337,6 +353,40 @@ static void 
controlvm_respond_physdev_changestate(CONTROLVM_MESSAGE_HEADER *
  msgHdr, int response,
  ULTRA_SEGMENT_STATE state);
 
+ssize_t show_toolaction(struct device *dev, struct device_attribute *attr,
+   char *buf)
+{
+   if (ControlVm_channel) {
+   U8 toolAction;
+
+   visorchannel_read(ControlVm_channel,
+   offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
+  ToolAction), toolAction, sizeof(U8));
+   return scnprintf(buf, PAGE_SIZE, %u\n, toolAction);
+   } else
+   return -ENODEV;
+}
+
+ssize_t store_toolaction(struct device *dev, struct device_attribute *attr,
+   const char *buf, size_t count)
+{
+   if (ControlVm_channel) {
+   U8 toolAction;
+
+   if (sscanf(buf, %hhu\n, toolAction) == 1) {
+   if (visorchannel_write(ControlVm_channel,
+   offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
+   ToolAction),
+   toolAction, sizeof(U8))  0)
+   return -EFAULT;
+   else
+   return count;
+   } else
+   return -EIO;
+   } else
+   return -ENODEV;
+}
+
 static void
 show_partition_property(struct seq_file *f, void *ctx, int property)
 {
@@ -2423,89 +2473,6 @@ proc_write_installer(struct file *file,
 }
 
 /**
- * Reads the ToolAction field of ControlVMChannel.
- */
-static ssize_t
-proc_read_toolaction(struct file *file, char __user *buf,
-

Re: [PATCH 02/10] staging: unisys: add toolaction to sysfs

2014-07-15 Thread Greg KH 
On Tue, Jul 15, 2014 at 01:30:42PM -0400, Benjamin Romer wrote:
 Move the proc entry for controlling the toolaction field to sysfs. The field
 appears in /sys/devices/platform/visorchipset/install/toolaction.
 
 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.
 
 Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com
 ---
  .../unisys/visorchipset/visorchipset_main.c| 157 
 -
  1 file changed, 60 insertions(+), 97 deletions(-)

All sysfs files need a Documentation/ABI/ entry.  As this isn't in the
real part of the kernel yet, just create the entries in the unisys/
subdir and then when it moves out, we can move them to the right place
in the tree.

Can you redo this patch with that entry?

And of course, there are comments below:


 --- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
 +++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
 @@ -150,11 +150,6 @@ static ssize_t proc_read_installer(struct file *file, 
 char __user *buf,
  static ssize_t proc_write_installer(struct file *file,
   const char __user *buffer,
   size_t count, loff_t *ppos);
 -static ssize_t proc_read_toolaction(struct file *file, char __user *buf,
 - size_t len, loff_t *offset);
 -static ssize_t proc_write_toolaction(struct file *file,
 -  const char __user *buffer,
 -  size_t count, loff_t *ppos);
  static ssize_t proc_read_bootToTool(struct file *file, char __user *buf,
   size_t len, loff_t *offset);
  static ssize_t proc_write_bootToTool(struct file *file,
 @@ -165,11 +160,6 @@ static const struct file_operations proc_installer_fops 
 = {
   .write = proc_write_installer,
  };
  
 -static const struct file_operations proc_toolaction_fops = {
 - .read = proc_read_toolaction,
 - .write = proc_write_toolaction,
 -};
 -
  static const struct file_operations proc_bootToTool_fops = {
   .read = proc_read_bootToTool,
   .write = proc_write_bootToTool,
 @@ -322,10 +312,36 @@ static VISORCHIPSET_BUSDEV_RESPONDERS BusDev_Responders 
 = {
  /* info for /dev/visorchipset */
  static dev_t MajorDev = -1; /** indicates major num for device */
  
 +/* prototypes for attributes */
 +static ssize_t show_toolaction(struct device *dev,
 + struct device_attribute *attr, char *buf);
 +
 +static ssize_t store_toolaction(struct device *dev,
 + struct device_attribute *attr, const char *buf, size_t count);
 +
 +static DEVICE_ATTR(toolaction, S_IRUSR | S_IWUSR, show_toolaction,
 + store_toolaction);
 +
 +static struct attribute *visorchipset_install_attrs[] = {
 + dev_attr_toolaction.attr,
 + NULL
 +};
 +
 +static struct attribute_group visorchipset_install_group = {
 + .name = install,
 + .attrs = visorchipset_install_attrs
 +};
 +
 +static const struct attribute_group *visorchipset_dev_groups[] = {
 + visorchipset_install_group,
 + NULL
 +};
 +
  /* /sys/devices/platform/visorchipset */
  static struct platform_device Visorchipset_platform_device = {
   .name = visorchipset,
   .id = -1,
 + .dev.groups = visorchipset_dev_groups,
  };

Why is this a platform device in the first place?  Isn't it a real
device in the system on some type of bus?

  /* Function prototypes */
 @@ -337,6 +353,40 @@ static void 
 controlvm_respond_physdev_changestate(CONTROLVM_MESSAGE_HEADER *
 msgHdr, int response,
 ULTRA_SEGMENT_STATE state);
  
 +ssize_t show_toolaction(struct device *dev, struct device_attribute *attr,
 + char *buf)
 +{
 + if (ControlVm_channel) {

How can this not be true?  I tried to follow the code here, but it's
horrid, why is this assigned in a work queue and not when the module
starts up?

 + U8 toolAction;
 +
 + visorchannel_read(ControlVm_channel,
 + offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
 +ToolAction), toolAction, sizeof(U8));
 + return scnprintf(buf, PAGE_SIZE, %u\n, toolAction);
 + } else
 + return -ENODEV;
 +}

Why would this sysfs file be created if there is not a device?  That's
not how sysfs files work, it should only be present if the file has a
value, and if not, it shouldn't be there.

Same goes for your other sysfs files in this patchset, you should never
be return -ENODEV, you just shouldn't have created the file in the first
place.  That makes your kernel code simpler, and hopefully, your
userspace code easier too.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org