On Thu, Nov 03, 2016 at 11:44:16AM -0400, David Kershner wrote:
> From: Tim Sell <Timothy.Sell@xxxxxxxxxx>
>
> Previously, the sysfs entry (assuming traditional sysfs mountpoint):
>
> /sys/bus/visorbus/devices/visorbus<n>/client_bus_info
>
> violated kernel conventions by printing more than one item. This along
> with the fact that the data emitted was diagnostic data (intended to
> shadow the client driver info provided via s-Par livedumps) made it a
> logical candidate for debugfs. So this patch moves this sysfs entry to
> debugfs as (assuming traditional debugfs mountpoint):
>
> /sys/kernel/debug/visorbus/visorbus<n>/client_bus_info
>
> Data for this debugfs is emitted using the preferred seq_file interface,
> which allowed a vastly-simplified version of vbuschannel_print_devinfo()
> to format the individual output components.
>
> Functionality was verified as follows:
>
> [root@sparguest visorbus]# mount | grep debug
> debugfs on /sys/kernel/debug type debugfs (rw)
> [root@sparguest visorbus]# pwd
> /sys/kernel/debug/visorbus
> [root@sparguest visorbus]# l visorbus1/
> total 0
> drwxr-xr-x 2 root root 0 Sep 28 16:36 .
> drwxr-xr-x 4 root root 0 Sep 28 16:36 ..
> -r--r----- 1 root root 0 Sep 28 16:36 client_bus_info
> [root@sparguest visorbus]# l visorbus2
> total 0
> drwxr-xr-x 2 root root 0 Sep 28 16:36 .
> drwxr-xr-x 4 root root 0 Sep 28 16:36 ..
> -r--r----- 1 root root 0 Sep 28 16:36 client_bus_info
> [root@sparguest visorbus]# cat visorbus1/client_bus_info
> Client device / client driver info for s-Par Console partition (vbus #1):
> chipset visorchipset kernel ver. 4.8.0-rc6-ARCH+
> clientbus visorbus kernel ver. 4.8.0-rc6-ARCH+
> [2]keyboard visorinput kernel ver. 4.8.0-rc6-ARCH+
> [3]mouse visorinput kernel ver. 4.8.0-rc6-ARCH+
> [root@sparguest visorbus]# cat visorbus2/client_bus_info
> Client device / client driver info for s-Par IOVM partition (vbus #2):
> chipset visorchipset kernel ver. 4.8.0-rc6-ARCH+
> clientbus visorbus kernel ver. 4.8.0-rc6-ARCH+
> [0]ultravnic visornic kernel ver. 4.8.0-rc6-ARCH+
> [1]ultravnic visornic kernel ver. 4.8.0-rc6-ARCH+
> [2]sparvhba visorhba kernel ver. 4.8.0-rc6-ARCH+
>
> Signed-off-by: Tim Sell <Timothy.Sell@xxxxxxxxxx>
> Reported-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: David Kershner <david.kershner@xxxxxxxxxx>
> ---
> drivers/staging/unisys/include/visorbus.h | 2 +
> drivers/staging/unisys/visorbus/vbuschannel.h | 214 +++---------------------
> drivers/staging/unisys/visorbus/visorbus_main.c | 165 ++++++++++--------
> 3 files changed, 127 insertions(+), 254 deletions(-)
Minor review comments, you can fix these up in future patches.
>
> diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
> index 677627c..03d56f8 100644
> --- a/drivers/staging/unisys/include/visorbus.h
> +++ b/drivers/staging/unisys/include/visorbus.h
> @@ -166,6 +166,8 @@ struct visor_device {
> struct controlvm_message_header *pending_msg_hdr;
> void *vbus_hdr_info;
> uuid_le partition_uuid;
> + struct dentry *debugfs_dir;
> + struct dentry *debugfs_client_bus_info;
No need to keep the dentry for the bus info, right?
> @@ -151,6 +153,8 @@ struct bus_type visorbus_type = {
> {
> struct visor_device *dev = dev_get_drvdata(xdev);
>
> + debugfs_remove(dev->debugfs_client_bus_info);
> + debugfs_remove_recursive(dev->debugfs_dir);
Yup, removing the directory will remove the file, so no need to keep it
around.
> + dev->debugfs_dir = debugfs_create_dir(dev_name(&dev->device),
> + visorbus_debugfs_dir);
> + if (!dev->debugfs_dir) {
> + err = -ENOMEM;
> + goto err_hdr_info;
> + }
There's never any need to check the return value of a debugfs call,
unless you really want to for some really odd reason. It shouldn't keep
your main logic from doing anything different, as you should not be
relying on it in any way.
So just drop the check here.
> + dev->debugfs_client_bus_info =
> + debugfs_create_file("client_bus_info", S_IRUSR | S_IRGRP,
> + dev->debugfs_dir, dev,
> + &client_bus_info_debugfs_fops);
> + if (!dev->debugfs_client_bus_info) {
> + err = -ENOMEM;
> + goto err_debugfs_dir;
> + }
Same here. Also, no need to keep the dentry at all.
> + visorbus_debugfs_dir = debugfs_create_dir("visorbus", NULL);
> + if (!visorbus_debugfs_dir)
> + return -ENOMEM;
And here, no need to check.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
