[PATCH] uio: add irq control support to uio_pci_generic

[PATCH] uio: add irq control support to uio_pci_generic

[Stephen Hemminger]

The driver already supported INTX interrupts but had no in kernel
function to enable and disable them.

It is possible for userspace to do this by accessing PCI config
directly, but this racy and better handled by same mechanism
that already exists in kernel.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>


--- a/drivers/uio/uio_pci_generic.c	2015-04-15 08:50:15.543900681 -0700
+++ b/drivers/uio/uio_pci_generic.c	2015-04-15 09:00:01.658609786 -0700
@@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s
 	return IRQ_HANDLED;
 }
 
+static int irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+	struct pci_dev *pdev = gdev->pdev;
+
+	pci_cfg_access_lock(pdev);
+	pci_intx(pdev, irq_on);
+	pci_cfg_access_unlock(pdev);
+
+	return 0;
+}
+
 static int probe(struct pci_dev *pdev,
 			   const struct pci_device_id *id)
 {
@@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev,
 	gdev->info.irq = pdev->irq;
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
+	gdev->info.irqcontrol = irqcontrol;
 	gdev->pdev = pdev;
 
 	err = uio_register_device(&pdev->dev, &gdev->info);

Re: [PATCH] uio: add irq control support to uio_pci_generic

[Michael S. Tsirkin]

On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote:
> The driver already supported INTX interrupts but had no in kernel
> function to enable and disable them.
> 
> It is possible for userspace to do this by accessing PCI config
> directly, but this racy

How is it racy? We have userspace using this interface,
if there's a race I want to fix it.

> and better handled by same mechanism
> that already exists in kernel.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> 
> --- a/drivers/uio/uio_pci_generic.c	2015-04-15 08:50:15.543900681 -0700
> +++ b/drivers/uio/uio_pci_generic.c	2015-04-15 09:00:01.658609786 -0700
> @@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s
>  	return IRQ_HANDLED;
>  }
>  
> +static int irqcontrol(struct uio_info *info, s32 irq_on)
> +{
> +	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
> +	struct pci_dev *pdev = gdev->pdev;
> +
> +	pci_cfg_access_lock(pdev);
> +	pci_intx(pdev, irq_on);
> +	pci_cfg_access_unlock(pdev);
> +
> +	return 0;
> +}
> +
>  static int probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *id)
>  {
> @@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev,
>  	gdev->info.irq = pdev->irq;
>  	gdev->info.irq_flags = IRQF_SHARED;
>  	gdev->info.handler = irqhandler;
> +	gdev->info.irqcontrol = irqcontrol;
>  	gdev->pdev = pdev;
>  
>  	err = uio_register_device(&pdev->dev, &gdev->info);

Re: [PATCH] uio: add irq control support to uio_pci_generic

[Stephen Hemminger]

On Thu, 16 Apr 2015 09:43:24 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote:
> > The driver already supported INTX interrupts but had no in kernel
> > function to enable and disable them.
> > 
> > It is possible for userspace to do this by accessing PCI config
> > directly, but this racy
> 
> How is it racy? We have userspace using this interface,
> if there's a race I want to fix it.

There is nothing to prevent two threads in user space doing 
read/modify write at the same time.

The bigger issue is that DPDK needs to support multiple UIO
interface types. And with current model there is no abstraction.
The way to enable/disable IRQ is different depending on the UIO
drivers.

Re: [PATCH] uio: add irq control support to uio_pci_generic

[Michael S. Tsirkin]

On Thu, Apr 16, 2015 at 02:21:10PM -0700, Stephen Hemminger wrote:
> On Thu, 16 Apr 2015 09:43:24 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote:
> > > The driver already supported INTX interrupts but had no in kernel
> > > function to enable and disable them.
> > > 
> > > It is possible for userspace to do this by accessing PCI config
> > > directly, but this racy
> > 
> > How is it racy? We have userspace using this interface,
> > if there's a race I want to fix it.
> 
> There is nothing to prevent two threads in user space doing 
> read/modify write at the same time.

Well that's a userspace bug then - so let's drop that
from commit log lest people think this fixes some
kernel bugs. read/modify/write to the same register
is at least an easy to grasp problem, creating
an extra interface for the same function opens up
the possibility that some userspace will do
read/modify/write from one thread with irqcontrol
from another thread, creating more races.

> The bigger issue is that DPDK needs to support multiple UIO
> interface types. And with current model there is no abstraction.
> The way to enable/disable IRQ is different depending on the UIO
> drivers.

OK compatibility with other devices might be useful, but what are the
other UIO drivers DPDK supports? I only found support for igb_uio so
far, and that doesn't seem to be upstream.

-- 
MST

Re: [PATCH] uio: add irq control support to uio_pci_generic

[Stephen Hemminger]

On Mon, 20 Apr 2015 15:59:06 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Apr 16, 2015 at 02:21:10PM -0700, Stephen Hemminger wrote:
> > On Thu, 16 Apr 2015 09:43:24 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote:
> > > > The driver already supported INTX interrupts but had no in kernel
> > > > function to enable and disable them.
> > > > 
> > > > It is possible for userspace to do this by accessing PCI config
> > > > directly, but this racy
> > > 
> > > How is it racy? We have userspace using this interface,
> > > if there's a race I want to fix it.
> > 
> > There is nothing to prevent two threads in user space doing 
> > read/modify write at the same time.
> 
> Well that's a userspace bug then - so let's drop that
> from commit log lest people think this fixes some
> kernel bugs. read/modify/write to the same register
> is at least an easy to grasp problem, creating
> an extra interface for the same function opens up
> the possibility that some userspace will do
> read/modify/write from one thread with irqcontrol
> from another thread, creating more races.
> 
> > The bigger issue is that DPDK needs to support multiple UIO
> > interface types. And with current model there is no abstraction.
> > The way to enable/disable IRQ is different depending on the UIO
> > drivers.
> 
> OK compatibility with other devices might be useful, but what are the
> other UIO drivers DPDK supports? I only found support for igb_uio so
> far, and that doesn't seem to be upstream.
> 

Currently, supports:
  igb_uio, uio_pci_generic (as well as vfio)

There are additional drivers which been submitted but not accepted for Xen and HyperV
both of which require special uio drivers.

Re: [PATCH] uio: add irq control support to uio_pci_generic

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 08:33:18AM -0700, Stephen Hemminger wrote:
> On Mon, 20 Apr 2015 15:59:06 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Apr 16, 2015 at 02:21:10PM -0700, Stephen Hemminger wrote:
> > > On Thu, 16 Apr 2015 09:43:24 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote:
> > > > > The driver already supported INTX interrupts but had no in kernel
> > > > > function to enable and disable them.
> > > > > 
> > > > > It is possible for userspace to do this by accessing PCI config
> > > > > directly, but this racy
> > > > 
> > > > How is it racy? We have userspace using this interface,
> > > > if there's a race I want to fix it.
> > > 
> > > There is nothing to prevent two threads in user space doing 
> > > read/modify write at the same time.
> > 
> > Well that's a userspace bug then - so let's drop that
> > from commit log lest people think this fixes some
> > kernel bugs. read/modify/write to the same register
> > is at least an easy to grasp problem, creating
> > an extra interface for the same function opens up
> > the possibility that some userspace will do
> > read/modify/write from one thread with irqcontrol
> > from another thread, creating more races.
> > 
> > > The bigger issue is that DPDK needs to support multiple UIO
> > > interface types. And with current model there is no abstraction.
> > > The way to enable/disable IRQ is different depending on the UIO
> > > drivers.
> > 
> > OK compatibility with other devices might be useful, but what are the
> > other UIO drivers DPDK supports? I only found support for igb_uio so
> > far, and that doesn't seem to be upstream.
> > 
> 
> Currently, supports:
>   igb_uio, uio_pci_generic (as well as vfio)
> 
> There are additional drivers which been submitted but not accepted for Xen and HyperV
> both of which require special uio drivers.

Well vfio doesn't have irq_control, does it?  So I'd say it's best to
wait and see before we commit to a new ABI then.  You probably need to
support existing kernels anyway, if igb_uio makes it upstream,
then adding an interface that's consistent with it will make sense.

-- 
MST