Message-ID: <561227C0.5050607@cloudius-systems.com>
Date:	Mon, 5 Oct 2015 10:33:20 +0300
From:	Vlad Zolotarov <vladz@...udius-systems.com>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	linux-kernel@...r.kernel.org, mst@...hat.com, hjk@...sjkoch.de,
	corbet@....net, bruce.richardson@...el.com,
	avi@...udius-systems.com, gleb@...udius-systems.com,
	stephen@...workplumber.org, alexander.duyck@...il.com
Subject: Re: [PATCH v3 1/3] uio: add ioctl support




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 <vladz@...udius-systems.com>
>> ---
>>   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 <vladz@...udius-systems.com>
>> ---
>>   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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists