From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752264AbbJEHd0 (ORCPT ); Mon, 5 Oct 2015 03:33:26 -0400 Received: from mail-wi0-f170.google.com ([209.85.212.170]:38692 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750825AbbJEHdY (ORCPT ); Mon, 5 Oct 2015 03:33:24 -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> 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: <561227C0.5050607@cloudius-systems.com> Date: Mon, 5 Oct 2015 10:33:20 +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: <20151005030352.GA27303@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 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. The main idea of this series is, as mentioned in PATCH0, to add the MSI and MSI-X support for uio_pci_generic driver. 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; }; where "vec" is an index of the IRQ starting from 0 and "fd" is an eventfd file descriptor a user wants to poll() for in order to get the interrupt indications. If "fd" is less than 0, ioctl() will unbind the interrupt from the previously bound eventfd descriptor. This way a user may poll() for any IRQ it wants separately, or epoll() for any subset of them, or do whatever he/she wants to do. That's why we needed the ioctl(). I admit that it may not be the _ONLY_ way to achieve the goal described above but again we took VFIO approach as a template for a solution and just followed it. If u think there is more elegant/robust/better way to do so, pls., share. :) thanks, vlad > > thanks, > > greg k-h 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 :( > > 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... > > thanks, > > greg k-h