From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753259AbbJEKgo (ORCPT ); Mon, 5 Oct 2015 06:36:44 -0400 Received: from mail-wi0-f179.google.com ([209.85.212.179]:36504 "EHLO mail-wi0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753206AbbJEKgm (ORCPT ); Mon, 5 Oct 2015 06:36:42 -0400 Subject: Re: [PATCH v3 1/3] uio: add ioctl support To: Greg KH References: <1443991398-23761-1-git-send-email-vladz@cloudius-systems.com> <1443991398-23761-2-git-send-email-vladz@cloudius-systems.com> <20151005030352.GA27303@kroah.com> <561227C0.5050607@cloudius-systems.com> <20151005080149.GB1747@kroah.com> Cc: linux-kernel@vger.kernel.org, mst@redhat.com, hjk@hansjkoch.de, corbet@lwn.net, bruce.richardson@intel.com, avi@cloudius-systems.com, gleb@cloudius-systems.com, stephen@networkplumber.org, alexander.duyck@gmail.com From: Vlad Zolotarov Message-ID: <561252B3.4000406@cloudius-systems.com> Date: Mon, 5 Oct 2015 13:36:35 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20151005080149.GB1747@kroah.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/05/15 11:01, Greg KH wrote: > On Mon, Oct 05, 2015 at 10:33:20AM +0300, Vlad Zolotarov wrote: >> On 10/05/15 06:03, Greg KH wrote: >>> On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote: >>>> Signed-off-by: Vlad Zolotarov >>>> --- >>>> drivers/uio/uio.c | 15 +++++++++++++++ >>>> include/linux/uio_driver.h | 3 +++ >>>> 2 files changed, 18 insertions(+) >>> You add an ioctl yet fail to justify _why_ you need/want that ioctl, and >>> you don't document it at all? Come on, you know better than that, no >>> one can take a patch that has no changelog comments at all like this :( >> My bad. U are absolutely right here - it was late and I was tired that I >> missed that to someone it may not be so "crystal clear" like it is to me... >> :) >> Again, my bad - let me clarify it here and if we agree I'll respin the >> series with all relevant updates including the changelog. >> >>> Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so >>> you had better have a really compelling argument as to why this is the >>> _ONLY_ way you can solve this unknown problem by using such a horrid >>> thing... >> Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, but >> only lets the underlying PCI drivers to have them. UIO in this case is only >> a proxy. > Exactly, and I don't want to provide an ioctl "proxy" for UIO drivers. > That way lies madness and horrid code, and other nasty things (hint, > each ioctl is a custom syscall, so you are opening up the box for all > sorts of bad things to happen in drivers...) > > For example, your ioctl you use here is incorrect, and will fail > horribly on a large majority of systems. I don't want to open up the > requirements that more people have to know how to "do it right" in order > to use the UIO interface for their drivers, as people will get it wrong > (as this patch series shows...) Sometimes there is no other (better) way to get things done. And bugs - isn't it what code review is for? ;) I'll fix the "int" issue. > >> The main idea of this series is, as mentioned in PATCH0, to add the MSI and >> MSI-X support for uio_pci_generic driver. > Yes, I know that, but I don't see anything that shows _how_ to use this > api. I get that, i'll extend PATCH3 of this series with a detailed description in v4. U use it as follows: 1. Bind the PCI function to uio_pci_generic. 2. Query for its interrupt mode with UIO_PCI_GENERIC_INT_MODE_GET ioctl. 3. If interrupt mode is INT#x or MSI - use the current UIO interface for polling, namely use the UIO file descriptor. 4. Else 1. Query for the number of MSI-X vectors with UIO_PCI_GENERIC_IRQ_NUM_GET ioctl. 2. Allocate the required number of eventfd descriptors using eventfd() from sys/eventfd.h. 3. Bind them to the required IRQs with UIO_PCI_GENERIC_IRQ_SET ioctl. 5. When done, just unbind the PCI function from the uio_pci_generic. > And then there's the issue of why we even need this, why not just > write a whole new driver for this, like the previous driver did (which > also used ioctls, yes, I didn't have the chance to object to that before > everyone else did...) Which "previous driver" do u refer here? IMHO writing something instead of UIO (not just uio_pci_generic) seems like an overkill for solving this issue. Supporting MSI-X interrupts seem like a very beneficial feature for uio_pci_generic and it's really not _THAT_ complicated API - just look at VFIO for a comparison... ;) uio_pci_generic is clearly missing this important feature. And creating another user space driver infrastructure just to add it seems extremely unjustified. > >> While with MSI the things are quite simple and we may just ride the existing >> infrastructure, with the MSI-X the things get a bit more complicated since >> we may have more than one interrupt vector. Therefore we have to decide >> which interface we want to give to the user. >> >> One option could be to make all existing interrupts trigger the same objects >> in UIO as the current single interrupt does, however this would create an >> awkward, quite not-flexible semantics. For instance a regular (kernel) >> driver has a separate state machine for each interrupt line, which sometimes >> runs on a separate CPU, etc. This way we get to the second option - allow >> indication for each separate interrupt vector. And for obvious reasons >> (mentioned above) we (Stephen has sent a similar series on a dpdk-dev list) >> chose a second approach. >> >> In order not to invent the wheel we mimicked the VFIO approach, which allows >> to bind the pre-allocated eventfd descriptor to the specific interrupt >> vector using the ioctl(). >> >> The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is: >> >> struct uio_pci_generic_irq_set { >> int vec; /* index of the IRQ to connect to starting from 0 */ >> int fd; >> }; > And that's broken :( Good catch. Thanks. Will fix. I'm not a big ioctl fan myself but unfortunately I don't see a good alternative here. proc? Would it make it cleaner? > > NEVER use an "int" for an ioctl, it is wrong and will cause horrible > issues on a large number of systems. That is what the __u16 and friends > variable types are for. You know better than this :) > > greg k-h