linux-kernel - Re: [PATCH 04/11] vmci_driver.patch: VMCI device driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120830210400.GB3276@kroah.com>
Date:	Thu, 30 Aug 2012 14:04:00 -0700
From:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
To:	George Zhang <georgezhang@...are.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"virtualization@...ts.linux-foundation.org" 
	<virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH 04/11] vmci_driver.patch: VMCI device driver.

On Thu, Aug 30, 2012 at 09:40:34AM -0700, George Zhang wrote:
> +struct vmci_device {
> +       struct mutex lock; /* Device access mutex */
> +
> +       unsigned int ioaddr;
> +       unsigned int ioaddr_size;
> +       unsigned int irq;
> +       unsigned int intr_type;
> +       bool exclusive_vectors;
> +       struct msix_entry msix_entries[VMCI_MAX_INTRS];
> +
> +       bool enabled;
> +       spinlock_t dev_spinlock; /* Lock for datagram access synchronization */
> +       atomic_t datagrams_allowed;
> +};

Why are you ignoring the driver model with this code, and the rest of
your infractructure?  Please don't, that's just rude.  Hint, you should
have a "struct device dev" in this structure if you are doing things
right.

> +static long drv_driver_unlocked_ioctl(struct file *filp,
> +                                     u_int iocmd,
> +                                     unsigned long ioarg)
> +{

Ah, a new syscall.  Why not just create a real syscall instead of
multiplexing here?  Are you _sure_ all of these ioctls really are needed
(hint, I know they aren't...)

> +static int __devinit drv_probe_device(struct pci_dev *pdev,
> +                                     const struct pci_device_id *id)
> +{
> +       unsigned int ioaddr;
> +       unsigned int ioaddr_size;
> +       unsigned int capabilities;
> +       int result;
> +
> +       pr_info("Probing for vmci/PCI.");

This is pointless, why are you being noisy?

> +       result = pci_enable_device(pdev);
> +       if (result) {
> +               pr_err("Cannot enable VMCI device %s: error %d",
> +                      pci_name(pdev), result);

Ick, please use dev_err() here, and other dev_* printk functions where
you can (hint, it's quite often in this file.)

> +               return result;
> +       }
> +       pci_set_master(pdev);   /* To enable QueuePair functionality. */
> +       ioaddr = pci_resource_start(pdev, 0);
> +       ioaddr_size = pci_resource_len(pdev, 0);
> +
> +       /*
> +        * Request I/O region with adjusted base address and size. The
> +        * adjusted values are needed and used if we release the
> +        * region in case of failure.
> +        */
> +       if (!request_region(ioaddr, ioaddr_size, MODULE_NAME)) {
> +               pr_info(MODULE_NAME ": Another driver already loaded " \
> +                       "for device in slot %s.", pci_name(pdev));
> +               goto pci_disable;
> +       }
> +
> +       pr_info("Found VMCI PCI device at %#x, irq %u.", ioaddr, pdev->irq);

Ick, noisy, you should NEVER print anything out if all goes well, that's
pointless.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/