On Tue, Jul 15, 2014 at 10:36:43AM -0400, Erik Arfvidson wrote:
> This patch adds the virtpci debugfs directory and the info entry
> inside of it.
>
> Signed-off-by: Erik Arfvidson <erik.arfvidson@xxxxxxxxxx>
> Signed-off-by: Benjamin Romer <benjamin.romer@xxxxxxxxxx>
> ---
> v4: Fixed comments, upper bound buffer, removed #define for virtpci and info,
> modified printvbus to work with scnprintf and added and extra element to
> print_vbus_info struct
> v3: Fixed formating and comments. Also added debufs_remove_recursive() and
> simple simple_read_from_buffer()
> v2: Fixed comments and applied Dan Carpenter suggestions
> drivers/staging/unisys/virtpci/virtpci.c | 111 ++++++++++++++++++++++++++++++-
> 1 file changed, 108 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c
> index 7d840b0..1929137 100644
> --- a/drivers/staging/unisys/virtpci/virtpci.c
> +++ b/drivers/staging/unisys/virtpci/virtpci.c
> @@ -36,6 +36,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/if_ether.h>
> #include <linux/version.h>
> +#include <linux/debugfs.h>
> #include "version.h"
> #include "guestlinuxdebug.h"
> #include "timskmodutils.h"
> @@ -57,6 +58,7 @@ struct driver_private {
> #endif
>
> #define BUS_ID(x) dev_name(x)
> +#define MAX_BUF 16384
Lovely magic number, care to explain why this is this size?
>
> #include "virtpci.h"
>
> @@ -100,6 +102,12 @@ static int virtpci_device_resume(struct device *dev);
> static int virtpci_device_probe(struct device *dev);
> static int virtpci_device_remove(struct device *dev);
>
> +static ssize_t info_debugfs_read(struct file *file, char __user *buf,
> + size_t len, loff_t *offset);
> +
> +static const struct file_operations debugfs_info_fops = {
> + .read = info_debugfs_read,
> +};
>
> /*****************************************************/
> /* Globals */
> @@ -139,7 +147,17 @@ static DEFINE_RWLOCK(VpcidevListLock);
>
> /* filled in with info about this driver, wrt it servicing client busses */
> static ULTRA_VBUS_DEVICEINFO Bus_DriverInfo;
> -
> +/*****************************************************/
> +/* debugfs entries */
> +/*****************************************************/
> +/* dentry is used to create the debugfs entry directory
> + * for virtpci
> + */
> +static struct dentry *virtpci_debugfs_dir;
> +static struct dentry *info_debugfs_entry;
> +/* info_debugfs_entry is used to tell virtpci to display current info
> + * kept in the driver
> + */
Odd to comment both above and below the variables, pick one style and
stick to it.
Also, no empty line ater the Bus_DriverInfo variable and the comment?
And again, you don't need the info_debugfs_entry variable at all. You
set it once and then never touch it again, seems like a waste to have
it, right?
>
> struct virtpci_busdev {
> struct device virtpci_bus_device;
> @@ -1375,6 +1393,89 @@ void virtpci_unregister_driver(struct virtpci_driver *drv)
> DBGINF("Leaving\n");
> }
> EXPORT_SYMBOL_GPL(virtpci_unregister_driver);
> +/*****************************************************/
> +/* debugfs filesystem functions */
> +/*****************************************************/
Again, no blank line before the comment?
> +struct print_vbus_info {
> + int *str_pos;
> + char *buf;
> + size_t *len;
> +};
> +
> +static int print_vbus(struct device *vbus, void *data)
> +{
> + struct print_vbus_info *p = (struct print_vbus_info *)data;
> +
> + *p->str_pos += scnprintf(p->buf + *p->str_pos, *p->len - *p->str_pos,
> + "bus_id:%s\n", dev_name(vbus));
> + return 0;
> +}
> +
> +static ssize_t info_debugfs_read(struct file *file, char __user *buf,
> + size_t len, loff_t *offset)
> +{
> + ssize_t bytes_read = 0;
> + int str_pos = 0;
> + struct virtpci_dev *tmpvpcidev;
> + unsigned long flags;
> + struct print_vbus_info printparam;
> + char *vbuf;
> +
> + if (len > MAX_BUF)
> + len = MAX_BUF;
> + vbuf = kzalloc(len, GFP_KERNEL);
> + if (!vbuf)
> + return -ENOMEM;
> +
> + str_pos += scnprintf(vbuf + str_pos, len - str_pos,
> + " Virtual PCI Bus devices\n");
> + printparam.str_pos = &str_pos;
> + printparam.buf = vbuf;
> + printparam.len = &len;
> + if (bus_for_each_dev(&virtpci_bus_type, NULL,
> + (void *) &printparam, print_vbus))
> + LOGERR("Failed to find bus\n");
> +
> + str_pos += scnprintf(vbuf + str_pos, len - str_pos,
> + "\n Virtual PCI devices\n");
> + read_lock_irqsave(&VpcidevListLock, flags);
> + tmpvpcidev = VpcidevListHead;
> + while (tmpvpcidev) {
> + if (tmpvpcidev->devtype == VIRTHBA_TYPE) {
> + str_pos += scnprintf(vbuf + str_pos, len - str_pos,
> + "[%d:%d] VHba:%08x:%08x max-config:%d-%d-%d-%d",
> + tmpvpcidev->busNo, tmpvpcidev->deviceNo,
> + tmpvpcidev->scsi.wwnn.wwnn1,
> + tmpvpcidev->scsi.wwnn.wwnn2,
> + tmpvpcidev->scsi.max.max_channel,
> + tmpvpcidev->scsi.max.max_id,
> + tmpvpcidev->scsi.max.max_lun,
> + tmpvpcidev->scsi.max.cmd_per_lun);
> + } else {
> + str_pos += scnprintf(vbuf + str_pos, len - str_pos,
> + "[%d:%d] VNic:%02x:%02x:%02x:%02x:%02x:%02x num_rcv_bufs:%d mtu:%d",
> + tmpvpcidev->busNo, tmpvpcidev->deviceNo,
> + tmpvpcidev->net.mac_addr[0],
> + tmpvpcidev->net.mac_addr[1],
> + tmpvpcidev->net.mac_addr[2],
> + tmpvpcidev->net.mac_addr[3],
> + tmpvpcidev->net.mac_addr[4],
> + tmpvpcidev->net.mac_addr[5],
> + tmpvpcidev->net.num_rcv_bufs,
> + tmpvpcidev->net.mtu);
> + }
> + str_pos += scnprintf(vbuf + str_pos,
> + len - str_pos, " chanptr:%p\n",
> + tmpvpcidev->queueinfo.chan);
> + tmpvpcidev = tmpvpcidev->next;
> + }
> + read_unlock_irqrestore(&VpcidevListLock, flags);
> +
> + str_pos += scnprintf(vbuf + str_pos, len - str_pos, "\n");
> + bytes_read = simple_read_from_buffer(buf, len, offset, vbuf, str_pos);
> + kfree(vbuf);
> + return bytes_read;
> +}
>
> /*****************************************************/
> /* Module Init & Exit functions */
> @@ -1426,7 +1527,10 @@ static int __init virtpci_mod_init(void)
>
> LOGINF("successfully registered virtpci_ctrlchan_func (0x%p) as callback.\n",
> (void *) &virtpci_ctrlchan_func);
> -
> + /* create debugfs directory and info file inside. */
> + virtpci_debugfs_dir = debugfs_create_dir("virtpci", NULL);
> + info_debugfs_entry = debugfs_create_file("info", S_IRUSR,
> + virtpci_debugfs_dir, NULL, &debugfs_info_fops);
> LOGINF("Leaving\n");
> POSTCODE_LINUX_2(VPCI_CREATE_EXIT_PC, POSTCODE_SEVERITY_INFO);
> return 0;
> @@ -1442,7 +1546,8 @@ static void __exit virtpci_mod_exit(void)
>
> device_unregister(&virtpci_rootbus_device);
> bus_unregister(&virtpci_bus_type);
> -
> + /* debugfs file and directory removal. */
> + debugfs_remove_recursive(virtpci_debugfs_dir);
No need to comment a single function call, as the function call should
be "obvious" as to what is going on here.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
