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.romer@xxxxxxxxxx>
> ---
> .../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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
