[PATCH 02/15] staging: unisys: visorbus: convert client_bus_info sysfs to debugfs

[Greg KH]

On Thu, Nov 03, 2016 at 11:44:16AM -0400, David Kershner wrote:
> From: Tim Sell <Timothy.Sell at unisys.com>
> 
> 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 at sparguest visorbus]# mount | grep debug
>   debugfs on /sys/kernel/debug type debugfs (rw)
>   [root at sparguest visorbus]# pwd
>   /sys/kernel/debug/visorbus
>   [root at 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 at 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 at 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 at 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 at unisys.com>
> Reported-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Signed-off-by: David Kershner <david.kershner at unisys.com>
> ---
>  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