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

[Vlad Zolotarov]

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@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >   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

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