Re: [PATCH v3 1/3] uio: add ioctl support

[Vlad Zolotarov <vladz@cloudius-systems.com>]

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 <vladz@cloudius-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.
> 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