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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
