Messages in this thread | | | Subject | Re: [PATCH v3 1/3] uio: add ioctl support | From | Vlad Zolotarov <> | Date | Mon, 5 Oct 2015 13:36:35 +0300 |
| |
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
| |