* [PATCH] uio: add irq control support to uio_pci_generic
@ 2015-04-15 16:59 Stephen Hemminger
2015-04-16 7:43 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2015-04-15 16:59 UTC (permalink / raw)
To: Michael S. Tsirkin, Hans J. Koch, Greg Kroah-Hartman; +Cc: kvm, linux-kernel
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] uio: add irq control support to uio_pci_generic
2015-04-15 16:59 [PATCH] uio: add irq control support to uio_pci_generic Stephen Hemminger
@ 2015-04-16 7:43 ` Michael S. Tsirkin
2015-04-16 21:21 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-04-16 7:43 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Hans J. Koch, Greg Kroah-Hartman, kvm, linux-kernel
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] uio: add irq control support to uio_pci_generic
2015-04-16 7:43 ` Michael S. Tsirkin
@ 2015-04-16 21:21 ` Stephen Hemminger
2015-04-20 13:59 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2015-04-16 21:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Hans J. Koch, Greg Kroah-Hartman, kvm, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] uio: add irq control support to uio_pci_generic
2015-04-16 21:21 ` Stephen Hemminger
@ 2015-04-20 13:59 ` Michael S. Tsirkin
2015-04-20 15:33 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-04-20 13:59 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Hans J. Koch, Greg Kroah-Hartman, kvm, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] uio: add irq control support to uio_pci_generic
2015-04-20 13:59 ` Michael S. Tsirkin
@ 2015-04-20 15:33 ` Stephen Hemminger
2015-04-20 15:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2015-04-20 15:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Hans J. Koch, Greg Kroah-Hartman, kvm, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] uio: add irq control support to uio_pci_generic
2015-04-20 15:33 ` Stephen Hemminger
@ 2015-04-20 15:40 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-04-20 15:40 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Hans J. Koch, Greg Kroah-Hartman, kvm, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-04-20 15:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-15 16:59 [PATCH] uio: add irq control support to uio_pci_generic Stephen Hemminger
2015-04-16 7:43 ` Michael S. Tsirkin
2015-04-16 21:21 ` Stephen Hemminger
2015-04-20 13:59 ` Michael S. Tsirkin
2015-04-20 15:33 ` Stephen Hemminger
2015-04-20 15:40 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox