| Message ID | 1439212864-12954-4-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-08-10 at 15:20 +0200, Eric Auger wrote: > A single handler now is registered whatever the use case: automasked > or not. A function pointer is set according to the wished behavior > and the handler calls this function. > > The irq lock is taken/released in the root handler. eventfd_signal can > be called in regions not allowed to sleep. > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > v4: creation > --- > drivers/vfio/platform/vfio_platform_irq.c | 21 +++++++++++++++------ > drivers/vfio/platform/vfio_platform_private.h | 1 + > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c > index 40f057a..b31b1f0 100644 > --- a/drivers/vfio/platform/vfio_platform_irq.c > +++ b/drivers/vfio/platform/vfio_platform_irq.c > @@ -148,11 +148,8 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev, > static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id) > { > struct vfio_platform_irq *irq_ctx = dev_id; > - unsigned long flags; > int ret = IRQ_NONE; > > - spin_lock_irqsave(&irq_ctx->lock, flags); > - > if (!irq_ctx->masked) { > ret = IRQ_HANDLED; > > @@ -161,8 +158,6 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id) > irq_ctx->masked = true; > } > > - spin_unlock_irqrestore(&irq_ctx->lock, flags); > - > if (ret == IRQ_HANDLED) > eventfd_signal(irq_ctx->trigger, 1); Has this been run with lockdep to check whether this is safe to call with spinlock_irqsave held? > > @@ -178,6 +173,19 @@ static irqreturn_t vfio_irq_handler(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static irqreturn_t vfio_handler(int irq, void *dev_id) > +{ > + struct vfio_platform_irq *irq_ctx = dev_id; > + unsigned long flags; > + irqreturn_t ret; > + > + spin_lock_irqsave(&irq_ctx->lock, flags); > + ret = irq_ctx->handler(irq, dev_id); > + spin_unlock_irqrestore(&irq_ctx->lock, flags); > + > + return ret; > +} > + > static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod) > { > } > @@ -229,9 +237,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, > } > > irq->trigger = trigger; > + irq->handler = handler; > > irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN); > - ret = request_irq(irq->hwirq, handler, 0, irq->name, irq); > + ret = request_irq(irq->hwirq, vfio_handler, 0, irq->name, irq); > if (ret) { > kfree(irq->name); > eventfd_ctx_put(trigger); > diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > index 8b4f814..f848a6b 100644 > --- a/drivers/vfio/platform/vfio_platform_private.h > +++ b/drivers/vfio/platform/vfio_platform_private.h > @@ -40,6 +40,7 @@ struct vfio_platform_irq { > struct virqfd *mask; > struct irq_bypass_producer producer; > bool forwarded; > + irqreturn_t (*handler)(int irq, void *dev_id); > }; > > struct vfio_platform_region {
Alex, On 08/12/2015 08:56 PM, Alex Williamson wrote: > On Mon, 2015-08-10 at 15:20 +0200, Eric Auger wrote: >> A single handler now is registered whatever the use case: automasked >> or not. A function pointer is set according to the wished behavior >> and the handler calls this function. >> >> The irq lock is taken/released in the root handler. eventfd_signal can >> be called in regions not allowed to sleep. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> v4: creation >> --- >> drivers/vfio/platform/vfio_platform_irq.c | 21 +++++++++++++++------ >> drivers/vfio/platform/vfio_platform_private.h | 1 + >> 2 files changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c >> index 40f057a..b31b1f0 100644 >> --- a/drivers/vfio/platform/vfio_platform_irq.c >> +++ b/drivers/vfio/platform/vfio_platform_irq.c >> @@ -148,11 +148,8 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev, >> static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id) >> { >> struct vfio_platform_irq *irq_ctx = dev_id; >> - unsigned long flags; >> int ret = IRQ_NONE; >> >> - spin_lock_irqsave(&irq_ctx->lock, flags); >> - >> if (!irq_ctx->masked) { >> ret = IRQ_HANDLED; >> >> @@ -161,8 +158,6 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id) >> irq_ctx->masked = true; >> } >> >> - spin_unlock_irqrestore(&irq_ctx->lock, flags); >> - >> if (ret == IRQ_HANDLED) >> eventfd_signal(irq_ctx->trigger, 1); > > Has this been run with lockdep to check whether this is safe to call > with spinlock_irqsave held? No I did not check with lockdep and I will do. There is a comment in fs/eventfd.c in eventfd_signal function comments that says: "This function is supposed to be called by the kernel in paths that do not allow sleeping. In this function we allow the counter to reach the ULLONG_MAX value, and we signal this as overflow condition by returining a POLLERR to poll(2)." so I understood from this it is safe. Best Regards Eric > >> >> @@ -178,6 +173,19 @@ static irqreturn_t vfio_irq_handler(int irq, void *dev_id) >> return IRQ_HANDLED; >> } >> >> +static irqreturn_t vfio_handler(int irq, void *dev_id) >> +{ >> + struct vfio_platform_irq *irq_ctx = dev_id; >> + unsigned long flags; >> + irqreturn_t ret; >> + >> + spin_lock_irqsave(&irq_ctx->lock, flags); >> + ret = irq_ctx->handler(irq, dev_id); >> + spin_unlock_irqrestore(&irq_ctx->lock, flags); >> + >> + return ret; >> +} >> + >> static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod) >> { >> } >> @@ -229,9 +237,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, >> } >> >> irq->trigger = trigger; >> + irq->handler = handler; >> >> irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN); >> - ret = request_irq(irq->hwirq, handler, 0, irq->name, irq); >> + ret = request_irq(irq->hwirq, vfio_handler, 0, irq->name, irq); >> if (ret) { >> kfree(irq->name); >> eventfd_ctx_put(trigger); >> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h >> index 8b4f814..f848a6b 100644 >> --- a/drivers/vfio/platform/vfio_platform_private.h >> +++ b/drivers/vfio/platform/vfio_platform_private.h >> @@ -40,6 +40,7 @@ struct vfio_platform_irq { >> struct virqfd *mask; >> struct irq_bypass_producer producer; >> bool forwarded; >> + irqreturn_t (*handler)(int irq, void *dev_id); >> }; >> >> struct vfio_platform_region { > > >
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c index 40f057a..b31b1f0 100644 --- a/drivers/vfio/platform/vfio_platform_irq.c +++ b/drivers/vfio/platform/vfio_platform_irq.c @@ -148,11 +148,8 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev, static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id) { struct vfio_platform_irq *irq_ctx = dev_id; - unsigned long flags; int ret = IRQ_NONE; - spin_lock_irqsave(&irq_ctx->lock, flags); - if (!irq_ctx->masked) { ret = IRQ_HANDLED; @@ -161,8 +158,6 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id) irq_ctx->masked = true; } - spin_unlock_irqrestore(&irq_ctx->lock, flags); - if (ret == IRQ_HANDLED) eventfd_signal(irq_ctx->trigger, 1); @@ -178,6 +173,19 @@ static irqreturn_t vfio_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; } +static irqreturn_t vfio_handler(int irq, void *dev_id) +{ + struct vfio_platform_irq *irq_ctx = dev_id; + unsigned long flags; + irqreturn_t ret; + + spin_lock_irqsave(&irq_ctx->lock, flags); + ret = irq_ctx->handler(irq, dev_id); + spin_unlock_irqrestore(&irq_ctx->lock, flags); + + return ret; +} + static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod) { } @@ -229,9 +237,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, } irq->trigger = trigger; + irq->handler = handler; irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN); - ret = request_irq(irq->hwirq, handler, 0, irq->name, irq); + ret = request_irq(irq->hwirq, vfio_handler, 0, irq->name, irq); if (ret) { kfree(irq->name); eventfd_ctx_put(trigger); diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h index 8b4f814..f848a6b 100644 --- a/drivers/vfio/platform/vfio_platform_private.h +++ b/drivers/vfio/platform/vfio_platform_private.h @@ -40,6 +40,7 @@ struct vfio_platform_irq { struct virqfd *mask; struct irq_bypass_producer producer; bool forwarded; + irqreturn_t (*handler)(int irq, void *dev_id); }; struct vfio_platform_region {
A single handler now is registered whatever the use case: automasked or not. A function pointer is set according to the wished behavior and the handler calls this function. The irq lock is taken/released in the root handler. eventfd_signal can be called in regions not allowed to sleep. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- v4: creation --- drivers/vfio/platform/vfio_platform_irq.c | 21 +++++++++++++++------ drivers/vfio/platform/vfio_platform_private.h | 1 + 2 files changed, 16 insertions(+), 6 deletions(-)