On Tue, Jul 08, 2014 at 01:21:31PM -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>
> ---
> drivers/staging/unisys/virtpci/virtpci.c | 122 ++++++++++++++++++++++++++++++-
> 1 file changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c
> index 7d840b0..6c0aa96 100644
> --- a/drivers/staging/unisys/virtpci/virtpci.c
> +++ b/drivers/staging/unisys/virtpci/virtpci.c
> @@ -37,6 +37,7 @@
> #include <linux/if_ether.h>
> #include <linux/version.h>
> #include "version.h"
> + #include <linux/debugfs.h>
> #include "guestlinuxdebug.h"
> #include "timskmodutils.h"
>
> @@ -100,6 +101,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 +146,19 @@ 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
> + */
> +#define DIR_DEBUGFS_ENTRY "virtpci"
> +#define INFO_DEBUGFS_ENTRY_FN "info"
>
> struct virtpci_busdev {
> struct device virtpci_bus_device;
> @@ -588,7 +607,8 @@ static void delete_all(void)
> /* deletes all vnics or vhbas
> * returns 0 failure, 1 success,
> */
> -static int delete_all_virt(VIRTPCI_DEV_TYPE devtype, struct del_vbus_guestpart *delparams)
> +static int delete_all_virt(VIRTPCI_DEV_TYPE devtype,
> + struct del_vbus_guestpart *delparams)
> {
> int i;
> unsigned char busid[BUS_ID_SIZE];
> @@ -1375,6 +1395,95 @@ void virtpci_unregister_driver(struct virtpci_driver *drv)
> DBGINF("Leaving\n");
> }
> EXPORT_SYMBOL_GPL(virtpci_unregister_driver);
> +/*****************************************************/
> +/* debugfs filesystem functions */
> +/*****************************************************/
> +struct print_vbus_info {
> + int *length;
> + char *buf;
> +};
> +
> +static int print_vbus(struct device *vbus, void *data)
> +{
> + struct print_vbus_info *p = (struct print_vbus_info *) data;
Remove the space after the cast to remind that casting is high
precedence.
struct print_vbus_info *p = (struct print_vbus_info *)data;
> + int l = *(p->length);
Remove the extra parens.
int l = *p->length;
> +
> + *(p->length) = l + sprintf(p->buf + l, "bus_id:%s\n", dev_name(vbus));
You don't really need the "l" at all.
*p->length += sprintf(p->buf + *p->length, "bus_id:%s\n",
dev_name(vbus));
> + return 0; /* no error */
> +}
> +
> +static ssize_t info_debugfs_read(struct file *file, char __user *buf,
> + size_t len, loff_t *offset)
> +{
> + int length = 0;
"length" is going to be confusing because we already have "len".
> + struct virtpci_dev *tmpvpcidev;
> + unsigned long flags;
> + struct print_vbus_info printparam;
> + char *vbuf;
> + loff_t pos = *offset;
> +
> + if (pos < 0)
> + return -EINVAL;
> +
> + if (pos > 0 || !len)
> + return 0;
I don't understand the point of updating *offset and then not allowing
pos > 0. (This could be because I haven't paid attention as well
though).
> +
> + vbuf = kzalloc(len, GFP_KERNEL);
> + if (!vbuf)
> + return -ENOMEM;
> +
> + length += sprintf(vbuf + length, "CHANSOCK is not defined.\n");
This is memory corruption here because we don't verify that len is large
enough for the string. None of the sprintf() calls have checking.
> +
> + length += sprintf(vbuf + length, "\n Virtual PCI Bus devices\n");
> + printparam.length = &length;
> + printparam.buf = vbuf;
> + if (bus_for_each_dev(&virtpci_bus_type, NULL,
> + (void *) &printparam, print_vbus))
> + LOGERR("delete of all vbus failed\n");
"delete"? Probably cut and paste bug.
> +
> + length += sprintf(vbuf + length, "\n Virtual PCI devices\n");
> + read_lock_irqsave(&VpcidevListLock, flags);
> + tmpvpcidev = VpcidevListHead;
> + while (tmpvpcidev) {
> + if (tmpvpcidev->devtype == VIRTHBA_TYPE) {
> + length += sprintf(vbuf + length,
> + "[%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 {
> + length += sprintf(vbuf + length, "[%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);
> + }
> + length +=
> + sprintf(vbuf + length, " chanptr:%p\n",
> + tmpvpcidev->queueinfo.chan);
> + tmpvpcidev = tmpvpcidev->next;
> + }
> + read_unlock_irqrestore(&VpcidevListLock, flags);
> +
> + length += sprintf(vbuf + length, "\n");
> + if (copy_to_user(buf, vbuf, length)) {
There should be some length checking here as well.
> + kfree(vbuf);
> + return -EFAULT;
> + }
> +
> + kfree(vbuf);
> + *offset += length;
> + return length;
> +}
>
> /*****************************************************/
> /* Module Init & Exit functions */
> @@ -1426,7 +1535,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*/
> + virtpci_debugfs_dir = debugfs_create_dir(DIR_DEBUGFS_ENTRY, NULL);
> + info_debugfs_entry = debugfs_create_file(INFO_DEBUGFS_ENTRY_FN, 0660,
The owner has write permision but there is no ->write op. Don't use the
numbers use S_IRUSR or whatever.
> + virtpci_debugfs_dir, NULL, &debugfs_info_fops);
> LOGINF("Leaving\n");
> POSTCODE_LINUX_2(VPCI_CREATE_EXIT_PC, POSTCODE_SEVERITY_INFO);
> return 0;
> @@ -1442,7 +1554,9 @@ static void __exit virtpci_mod_exit(void)
>
> device_unregister(&virtpci_rootbus_device);
> bus_unregister(&virtpci_bus_type);
> -
> + /*debug_fs file and direcroty removal*/
> + debugfs_remove(info_debugfs_entry);
> + debugfs_remove(virtpci_debugfs_dir);
These are not indented correctly.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
