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

[Greg KH]

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...)

> 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.  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...)

> 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 :(

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/