Re: [PATCH 4/5] uio: new driver with MSI-X support

["Liang, Cunming" <cunming.liang@intel.com>]

On 5/19/2015 1:40 AM, Stephen Hemminger wrote:
> +
> +/* set the mapping between vector # and existing eventfd. */
> +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> +{
> +	struct uio_msi_irq_ctx *ctx;
> +	struct eventfd_ctx *trigger;
> +	int irq, err;
> +
> +	if (vec >= udev->num_vectors) {
> +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> +			   vec, udev->num_vectors);
> +		return -ERANGE;
> +	}
> +
> +	irq = udev->msix[vec].vector;
> +
> +	/* Clearup existing irq mapping */
> +	ctx = &udev->ctx[vec];
> +	if (ctx->trigger) {
> +		free_irq(irq, ctx->trigger);
> +		eventfd_ctx_put(ctx->trigger);
> +		ctx->trigger = NULL;
> +	}
> +
> +	/* Passing -1 is used to disable interrupt */
> +	if (fd < 0)
> +		return 0;
> +
> +
One unnecessary blank line here.
> +	trigger = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(trigger)) {
> +		err = PTR_ERR(trigger);
> +		dev_notice(&udev->pdev->dev,
> +			   "eventfd ctx get failed: %d\n", err);
> +		return err;
> +	}
> +
> +	err = request_irq(irq, uio_msi_irqhandler, 0, ctx->name, trigger);
> +	if (err) {
> +		dev_notice(&udev->pdev->dev,
> +			   "request irq failed: %d\n", err);
> +		eventfd_ctx_put(trigger);
> +		return err;
> +	}
> +
> +	dev_dbg(&udev->pdev->dev, "map vector %u to fd %d trigger %p\n",
> +		  vec, fd, trigger);
> +	ctx->trigger = trigger;
> +	return 0;
> +}
> +
> +static int
> +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	struct uio_msi_irq_set hdr;
> +	int err;
> +
> +	switch (cmd) {
> +	case UIO_MSI_IRQ_SET:
> +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> +			return -EFAULT;
> +
> +		mutex_lock(&udev->mutex);
> +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> +		mutex_unlock(&udev->mutex);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +	return err;
> +}
"uio_msi_irq_set" defines in single pattern. Compare with the bulk set 
in "vfio_irq_set", it requires additional syscall during uio_msix_enable().
> +
> +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct uio_msi_pci_dev *udev;
> +	int i, err, vectors;
> +
> +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> +	if (!udev)
> +		return -ENOMEM;
> +
> +	err = pci_enable_device(pdev);
> +	if (err != 0) {
> +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> +		goto fail_free;
> +	}
> +
> +	vectors = pci_msix_vec_count(pdev);
> +	if (vectors < 0) {
> +		dev_err(&pdev->dev, "device does not support MSI-X\n");
> +		err = -EINVAL;
> +		goto fail_disable;
> +	}
pci_msix_vec_count() is available since v3.14,  it requires a compatible 
check.
In order to support older version, probably a function 
'uio_msix_vec_count()' is necessary.

I've one overall question, is there a special reason not enhance igb_uio 
but define a new uio_msi?  And the looks like the piece could be add 
into igb_uio or uio_pci_generic as well. Do you have plan for the 
latter? Thanks.