From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f49.google.com (mail-pa0-f49.google.com [209.85.220.49]) by dpdk.org (Postfix) with ESMTP id 51758C2FC for ; Mon, 25 May 2015 19:41:35 +0200 (CEST) Received: by pabru16 with SMTP id ru16so74789398pab.1 for ; Mon, 25 May 2015 10:41:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type:content-transfer-encoding; bh=uMarIADMQPVwLNX+rcDaF8n9mCdMkQtXiFrtMFKLIsc=; b=jZWqSVpUkgRlZmj6iU++/dZeF5X1zmT5CFZYpAb9B8yVrpHd/Z22xG2Jzwjz+fJH+5 XmQEsG9q09R3Pz8eiUE06XHj3l0B5SHs2YnKl50KvwOFf2g0eU11ICJqvZZCVmELrM3Z xccOPuM2I4qn1kNpooZLn9PUbJcpC5IJNf/lbVrDI54QKdqUGgJGIPIkGLiBoO7HRuSy QTc8zRyOZL9acu//zHY4rvl3fOEupzSNiu7g4OBLFDKOFJXaNHcshw0MXjzfh3MkmUSl 8pkHmPc5VssKOFnKQglK1dMAinPy5Y+pth5GeCf192kkumy+uT3MmBi7r11adSi1RsMt FcIQ== X-Gm-Message-State: ALoCoQkBmoIvNt+VvSW5JPxO4IOoel59cQgu5oj0bBBqXWiiP53C9g0QM3TdlGxmG3dRS5lJjAGz X-Received: by 10.68.98.133 with SMTP id ei5mr41641476pbb.51.1432575694594; Mon, 25 May 2015 10:41:34 -0700 (PDT) Received: from urahara (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by mx.google.com with ESMTPSA id nj7sm10617689pbc.36.2015.05.25.10.41.34 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 25 May 2015 10:41:34 -0700 (PDT) Date: Mon, 25 May 2015 10:41:37 -0700 From: Stephen Hemminger To: "Liang, Cunming" Message-ID: <20150525104137.166e8dd1@urahara> In-Reply-To: <5562BAAA.5050301@intel.com> References: <1431970814-25951-1-git-send-email-stephen@networkplumber.org> <1431970814-25951-5-git-send-email-stephen@networkplumber.org> <5562BAAA.5050301@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, cumming.lian@intel.com Subject: Re: [dpdk-dev] [PATCH 4/5] uio: new driver with MSI-X support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 May 2015 17:41:36 -0000 On Mon, 25 May 2015 14:01:14 +0800 "Liang, Cunming" wrote: > > > 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(). The bulk operation is VFIO is actually a bad design, It forces too many updates when manipulating individual vectors. Personally, the whole VFIO API has some questionable design choices that I did not want to repeat. > > +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. I wanted something that could go upstream. igb_uio has some other things which make it unlikely to get accepted in current form, so seemed best to start fresh. Also, intentionally did not want to address any kernel version earlier than the version where VFIO was added.