diff mbox series

[V3,2/3] gpio: virtio: Add IRQ support

Message ID 911941d4bf19f18abdc9700abca9f26b3c04c343.1623326176.git.viresh.kumar@linaro.org
State New
Headers show
Series gpio: Add virtio based driver | expand

Commit Message

Viresh Kumar June 10, 2021, 12:16 p.m. UTC
This patch adds IRQ support for the virtio GPIO driver. Note that this
uses the irq_bus_lock/unlock() callbacks since the operations over
virtio can sleep.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/gpio/gpio-virtio.c       | 256 ++++++++++++++++++++++++++++++-
 include/uapi/linux/virtio_gpio.h |  15 ++
 2 files changed, 263 insertions(+), 8 deletions(-)

-- 
2.31.1.272.g89b43f80a514

Comments

Linus Walleij June 10, 2021, 9:30 p.m. UTC | #1
Hi Viresh!

thanks for this interesting patch!

On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> This patch adds IRQ support for the virtio GPIO driver. Note that this

> uses the irq_bus_lock/unlock() callbacks since the operations over

> virtio can sleep.

>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>


>  drivers/gpio/gpio-virtio.c       | 256 ++++++++++++++++++++++++++++++-

>  include/uapi/linux/virtio_gpio.h |  15 ++


You also need to
select GPIOLIB_IRQCHIP
in the Kconfig entry for the gpio-virtio driver, because you make
use of it.

> +static void virtio_gpio_irq_mask(struct irq_data *d)

> +{

> +       struct gpio_chip *gc = irq_data_to_gpio_chip(d);

> +       struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);

> +       struct vgpio_line *line = &vgpio->lines[d->hwirq];

> +

> +       line->masked = true;

> +       line->masked_pending = true;

> +}

> +

> +static void virtio_gpio_irq_unmask(struct irq_data *d)

> +{

> +       struct gpio_chip *gc = irq_data_to_gpio_chip(d);

> +       struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);

> +       struct vgpio_line *line = &vgpio->lines[d->hwirq];

> +

> +       line->masked = false;

> +       line->masked_pending = true;

> +}


This looks dangerous in combination with this:

> +static void virtio_gpio_interrupt(struct virtqueue *vq)

> +{

(...)
> +       local_irq_disable();

> +       ret = generic_handle_irq(irq);

> +       local_irq_enable();


Nominally slow IRQs like those being marshalled over
virtio should be nested, handle_nested_irq(irq);
but are they? Or are they just quite slow not super slow?

If a threaded IRQF_ONESHOT was requested the
IRQ core will kick the thread and *MASK* this IRQ,
which means it will call back to your .irq_mask() function
and expect it to be masked from this
point.

But the IRQ will not actually be masked until you issue
your callbacks in the .irq_bus_sync_unlock() callback
right?

So from this point until .irq_bus_sync_unlock()
get called and actually mask the IRQ, it could be
fired again? I suppose the IRQ handler is reentrant?
This would violate the API.

I would say that from this point and until you sync
you need a spinlock or other locking primitive to
stop this IRQ from fireing again, and a spinlock will
imply local_irq_disable() so this gets really complex.

I would say only using nesting IRQs or guarantee this
some other way, one way would be to specify that
whatever is at the other side of virtio cannot send another
GPIO IRQ message before the last one is handled,
so you would need to send a specific (new)
VIRTIO_GPIO_REQ_IRQ_ACK after all other messages
have been sent in .irq_bus_sync_unlock()
so that the next GPIO IRQ can be dispatched after that.

(Is this how messaged signalled interrupts work? No idea.
When in doubt ask the IRQ maintainers.)

Thanks,
Linus Walleij
Viresh Kumar June 14, 2021, 7:08 a.m. UTC | #2
On 10-06-21, 23:30, Linus Walleij wrote:
> On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > +static void virtio_gpio_irq_unmask(struct irq_data *d)

> > +{

> > +       struct gpio_chip *gc = irq_data_to_gpio_chip(d);

> > +       struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);

> > +       struct vgpio_line *line = &vgpio->lines[d->hwirq];

> > +

> > +       line->masked = false;

> > +       line->masked_pending = true;

> > +}

> 

> This looks dangerous in combination with this:

> 

> > +static void virtio_gpio_interrupt(struct virtqueue *vq)

> > +{

> (...)

> > +       local_irq_disable();

> > +       ret = generic_handle_irq(irq);

> > +       local_irq_enable();

> 

> Nominally slow IRQs like those being marshalled over

> virtio should be nested, handle_nested_irq(irq);

> but are they?


Hmm, this is the call trace:

Call trace:
 virtio_gpio_interrupt+0x34/0x168
 vring_interrupt+0x64/0x98
 vp_vring_interrupt+0x5c/0xa8
 vp_interrupt+0x40/0x78
 __handle_irq_event_percpu+0x5c/0x180
 handle_irq_event_percpu+0x38/0x90
 handle_irq_event+0x48/0xe0
 handle_fasteoi_irq+0xb0/0x138
 generic_handle_irq+0x30/0x48
 __handle_domain_irq+0x60/0xb8
 gic_handle_irq+0x58/0x128
 el1_irq+0xb0/0x180
 arch_cpu_idle+0x18/0x28
 default_idle_call+0x24/0x5c
 do_idle+0x1ec/0x288
 cpu_startup_entry+0x28/0x68
 rest_init+0xd8/0xe8
 arch_call_rest_init+0x10/0x1c
 start_kernel+0x508/0x540

I don't see a threaded interrupt in the path and vp_vring_interrupt()
already takes spin_lock_irqsave().

This is what handle_nested_irq() says:

 *	Handle interrupts which are nested into a threaded interrupt
 *	handler. The handler function is called inside the calling
 *	threads context.

So AFAICT, handle_nested_irq() is relevant if the irq-chip's handler
is called in threaded context instead of hard one. In this case it is
called from hard-irq context and so calling generic_handle_irq() looks
to be the right thing.

Right ?

> Or are they just quite slow not super slow?


It doesn't use another slow bus like I2C, but this should be slow
anyway.

> If a threaded IRQF_ONESHOT was requested the

> IRQ core will kick the thread and *MASK* this IRQ,

> which means it will call back to your .irq_mask() function

> and expect it to be masked from this

> point.

> 

> But the IRQ will not actually be masked until you issue

> your callbacks in the .irq_bus_sync_unlock() callback

> right?


Yes.

> So from this point until .irq_bus_sync_unlock()

> get called and actually mask the IRQ, it could be

> fired again?


Since we are defining the spec right now, this is up to us to decide
how we want to do it.

> I suppose the IRQ handler is reentrant?


It shouldn't happen because of the locking in place in the virtqueue
core (vp_vring_interrupt()).

> This would violate the API.

> 

> I would say that from this point and until you sync

> you need a spinlock or other locking primitive to

> stop this IRQ from fireing again, and a spinlock will

> imply local_irq_disable() so this gets really complex.

> 

> I would say only using nesting IRQs or guarantee this

> some other way, one way would be to specify that

> whatever is at the other side of virtio cannot send another

> GPIO IRQ message before the last one is handled,

> so you would need to send a specific (new)

> VIRTIO_GPIO_REQ_IRQ_ACK after all other messages

> have been sent in .irq_bus_sync_unlock()

> so that the next GPIO IRQ can be dispatched after that.


I was thinking of mentioning this clearly in the spec at first, but
now after checking the sequence of things it looks like Linux will do
it anyway. Though adding this clearly in the spec can be better. We
should just send a response message here instead of another message
type VIRTIO_GPIO_REQ_IRQ_ACK.

> (Is this how messaged signalled interrupts work? No idea.

> When in doubt ask the IRQ maintainers.)


-- 
viresh
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c
index 1ba8ddf4693a..8bc4b1876961 100644
--- a/drivers/gpio/gpio-virtio.c
+++ b/drivers/gpio/gpio-virtio.c
@@ -20,6 +20,13 @@ 
 #include <uapi/linux/virtio_gpio.h>
 #include <uapi/linux/virtio_ids.h>
 
+struct vgpio_line {
+	u8 irq_type;
+	bool irq_type_pending;
+	bool masked;
+	bool masked_pending;
+};
+
 struct virtio_gpio {
 	struct virtio_device *vdev;
 	struct mutex lock;
@@ -30,14 +37,20 @@  struct virtio_gpio {
 	struct virtio_gpio_request creq;
 	struct virtio_gpio_response cres;
 
+	struct irq_chip *ic;
+	struct vgpio_line *lines;
+	struct virtqueue *interrupt_vq;
+	struct virtio_gpio_request ireq;
+
 	/* This must be kept as the last entry here, hint: gpio_names[0] */
 	struct virtio_gpio_config config;
 };
 
 #define gpio_chip_to_vgpio(chip) container_of(chip, struct virtio_gpio, gc)
+#define irq_data_to_gpio_chip(d) (d->domain->host_data)
 
-static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio,
-			   u8 txdata, u8 *rxdata)
+static int virtio_gpio_req_unlocked(struct virtio_gpio *vgpio, u16 type,
+				    u16 gpio, u8 txdata, u8 *rxdata)
 {
 	struct virtio_gpio_response *res = &vgpio->cres;
 	struct virtio_gpio_request *req = &vgpio->creq;
@@ -56,11 +69,10 @@  static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio,
 	sgs[0] = &req_sg;
 	sgs[1] = &res_sg;
 
-	mutex_lock(&vgpio->lock);
 	ret = virtqueue_add_sgs(vgpio->command_vq, sgs, 1, 1, res, GFP_KERNEL);
 	if (ret) {
 		dev_err(dev, "failed to add request to vq\n");
-		goto out;
+		return ret;
 	}
 
 	reinit_completion(&vgpio->completion);
@@ -81,7 +93,16 @@  static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio,
 	if (rxdata)
 		*rxdata = res->data;
 
-out:
+	return ret;
+}
+
+static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio,
+			   u8 txdata, u8 *rxdata)
+{
+	int ret;
+
+	mutex_lock(&vgpio->lock);
+	ret = virtio_gpio_req_unlocked(vgpio, type, gpio, txdata, rxdata);
 	mutex_unlock(&vgpio->lock);
 
 	return ret;
@@ -152,6 +173,183 @@  static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
 	virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_SET_VALUE, gpio, value, NULL);
 }
 
+/* Interrupt handling */
+static void vgpio_irq_mask(struct virtio_gpio *vgpio, u16 gpio)
+{
+	int ret;
+
+	ret = virtio_gpio_req_unlocked(vgpio, VIRTIO_GPIO_REQ_IRQ_MASK, gpio, 0,
+				       NULL);
+	if (ret)
+		dev_err(&vgpio->vdev->dev, "failed to mask irq: %d\n", ret);
+}
+
+static void vgpio_irq_unmask(struct virtio_gpio *vgpio, u16 gpio)
+{
+	int ret;
+
+	ret = virtio_gpio_req_unlocked(vgpio, VIRTIO_GPIO_REQ_IRQ_UNMASK, gpio,
+				       0, NULL);
+	if (ret)
+		dev_err(&vgpio->vdev->dev, "failed to unmask irq: %d\n", ret);
+}
+
+static void vgpio_irq_set_type(struct virtio_gpio *vgpio, u16 gpio, u8 type)
+{
+	int ret;
+
+	ret = virtio_gpio_req_unlocked(vgpio, VIRTIO_GPIO_REQ_IRQ_TYPE, gpio,
+				       type, NULL);
+	if (ret)
+		dev_err(&vgpio->vdev->dev, "failed to set irq type: %d\n", ret);
+}
+
+static void virtio_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_to_gpio_chip(d);
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+	struct vgpio_line *line = &vgpio->lines[d->hwirq];
+
+	line->masked = true;
+	line->masked_pending = true;
+}
+
+static void virtio_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_to_gpio_chip(d);
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+	struct vgpio_line *line = &vgpio->lines[d->hwirq];
+
+	line->masked = false;
+	line->masked_pending = true;
+}
+
+static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_to_gpio_chip(d);
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+	struct vgpio_line *line = &vgpio->lines[d->hwirq];
+	u8 irq_type;
+
+	switch (type) {
+	case IRQ_TYPE_NONE:
+		irq_type = VIRTIO_GPIO_IRQ_TYPE_NONE;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		irq_type = VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		irq_type = VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		irq_type = VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH;
+		break;
+	default:
+		dev_err(&vgpio->vdev->dev, "unsupported irq type: %u\n", type);
+		return -EINVAL;
+	}
+
+	line->irq_type = irq_type;
+	line->irq_type_pending = true;
+
+	return 0;
+}
+
+static void virtio_gpio_irq_bus_lock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_to_gpio_chip(d);
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+
+	mutex_lock(&vgpio->lock);
+}
+
+static void virtio_gpio_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_to_gpio_chip(d);
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+	struct vgpio_line *line = &vgpio->lines[d->hwirq];
+
+	if (line->irq_type_pending) {
+		vgpio_irq_set_type(vgpio, d->hwirq, line->irq_type);
+		line->irq_type_pending = false;
+	}
+
+	if (line->masked_pending) {
+		if (line->masked)
+			vgpio_irq_mask(vgpio, d->hwirq);
+		else
+			vgpio_irq_unmask(vgpio, d->hwirq);
+		line->masked_pending = false;
+	}
+
+	mutex_unlock(&vgpio->lock);
+}
+
+static void virtio_gpio_interrupt_prepare(struct virtio_gpio *vgpio)
+{
+	struct scatterlist sg[1];
+
+	/* Clear request buffer */
+	vgpio->ireq.type = 0;
+	vgpio->ireq.gpio = vgpio->config.ngpio;
+
+	sg_init_one(sg, &vgpio->ireq, sizeof(vgpio->ireq));
+	virtqueue_add_inbuf(vgpio->interrupt_vq, sg, 1, vgpio, GFP_KERNEL);
+	virtqueue_kick(vgpio->interrupt_vq);
+}
+
+static void virtio_gpio_interrupt(struct virtqueue *vq)
+{
+	struct virtio_gpio *vgpio = vq->vdev->priv;
+	struct device *dev = &vq->vdev->dev;
+	unsigned int len;
+	int irq, gpio, ret;
+	void *data;
+
+	data = virtqueue_get_buf(vgpio->interrupt_vq, &len);
+	if (!data || !len) {
+		dev_warn(dev, "No data received on interrupt\n");
+		return;
+	}
+
+	if (WARN_ON(data != vgpio))
+		return;
+
+	if (le16_to_cpu(vgpio->ireq.type) != VIRTIO_GPIO_REQ_IRQ_EVENT) {
+		dev_warn(dev, "Invalid irq-type: %d\n", vgpio->ireq.type);
+		goto out;
+	}
+
+	gpio = le16_to_cpu(vgpio->ireq.gpio);
+
+	if (gpio >= vgpio->config.ngpio) {
+		dev_warn(dev, "Invalid GPIO number: %d\n", gpio);
+		goto out;
+	}
+
+	irq = irq_find_mapping(vgpio->gc.irq.domain, gpio);
+	if (!irq) {
+		dev_err(dev, "Failed to find IRQ for gpio: %d\n", gpio);
+		goto out;
+	}
+
+	local_irq_disable();
+	ret = generic_handle_irq(irq);
+	local_irq_enable();
+
+	if (ret)
+		dev_err(dev, "failed to invoke irq handler\n");
+
+out:
+	virtio_gpio_interrupt_prepare(vgpio);
+}
+
 static void virtio_gpio_command(struct virtqueue *vq)
 {
 	struct virtio_gpio *vgpio = vq->vdev->priv;
@@ -162,14 +360,15 @@  static void virtio_gpio_command(struct virtqueue *vq)
 static int virtio_gpio_alloc_vqs(struct virtio_device *vdev)
 {
 	struct virtio_gpio *vgpio = vdev->priv;
-	const char * const names[] = { "command" };
+	const char * const names[] = { "command", "interrupt" };
 	vq_callback_t *cbs[] = {
 		virtio_gpio_command,
+		virtio_gpio_interrupt,
 	};
-	struct virtqueue *vqs[1] = {NULL};
+	struct virtqueue *vqs[2] = {NULL};
 	int ret;
 
-	ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL);
+	ret = virtio_find_vqs(vdev, vgpio->ic ? 2 : 1, vqs, cbs, names, NULL);
 	if (ret) {
 		dev_err(&vdev->dev, "failed to allocate vqs: %d\n", ret);
 		return ret;
@@ -177,6 +376,13 @@  static int virtio_gpio_alloc_vqs(struct virtio_device *vdev)
 
 	vgpio->command_vq = vqs[0];
 
+	if (vgpio->ic) {
+		vgpio->interrupt_vq = vqs[1];
+
+		virtio_gpio_interrupt_prepare(vgpio);
+		virtqueue_enable_cb(vgpio->interrupt_vq);
+	}
+
 	/* Mark the device ready to perform operations from within probe() */
 	virtio_device_ready(vgpio->vdev);
 	return 0;
@@ -278,6 +484,34 @@  static int virtio_gpio_probe(struct virtio_device *vdev)
 	if (IS_ERR(vgpio->gc.names))
 		return PTR_ERR(vgpio->gc.names);
 
+	if (virtio_has_feature(vdev, VIRTIO_GPIO_F_IRQ)) {
+		vgpio->lines = devm_kcalloc(dev, config->ngpio,
+					    sizeof(*vgpio->lines), GFP_KERNEL);
+		if (!vgpio->lines)
+			return -ENOMEM;
+
+		vgpio->ic = devm_kzalloc(dev, sizeof(*vgpio->ic), GFP_KERNEL);
+		if (!vgpio->ic)
+			return -ENOMEM;
+
+		vgpio->gc.irq.chip		= vgpio->ic;
+		vgpio->ic->name			= vgpio->gc.label;
+		vgpio->ic->irq_mask		= virtio_gpio_irq_mask;
+		vgpio->ic->irq_unmask		= virtio_gpio_irq_unmask;
+		vgpio->ic->irq_set_type		= virtio_gpio_irq_set_type;
+
+		/* These are required to implement irqchip for slow busses */
+		vgpio->ic->irq_bus_lock		= virtio_gpio_irq_bus_lock;
+		vgpio->ic->irq_bus_sync_unlock	= virtio_gpio_irq_bus_sync_unlock;
+
+		/* The event comes from the outside so no parent handler */
+		vgpio->gc.irq.parent_handler	= NULL;
+		vgpio->gc.irq.num_parents	= 0;
+		vgpio->gc.irq.parents		= NULL;
+		vgpio->gc.irq.default_type	= IRQ_TYPE_NONE;
+		vgpio->gc.irq.handler		= handle_level_irq;
+	}
+
 	vdev->priv = vgpio;
 	mutex_init(&vgpio->lock);
 	init_completion(&vgpio->completion);
@@ -309,7 +543,13 @@  static const struct virtio_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(virtio, id_table);
 
+static const unsigned int features[] = {
+	VIRTIO_GPIO_F_IRQ,
+};
+
 static struct virtio_driver virtio_gpio_driver = {
+	.feature_table		= features,
+	.feature_table_size	= ARRAY_SIZE(features),
 	.id_table		= id_table,
 	.probe			= virtio_gpio_probe,
 	.remove			= virtio_gpio_remove,
diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_gpio.h
index e805e33a79cb..533d70d77d2c 100644
--- a/include/uapi/linux/virtio_gpio.h
+++ b/include/uapi/linux/virtio_gpio.h
@@ -5,6 +5,9 @@ 
 
 #include <linux/types.h>
 
+/* Virtio GPIO Feature bits */
+#define VIRTIO_GPIO_F_IRQ			0
+
 /* Virtio GPIO request types */
 #define VIRTIO_GPIO_REQ_ACTIVATE		0x01
 #define VIRTIO_GPIO_REQ_DEACTIVATE		0x02
@@ -13,6 +16,18 @@ 
 #define VIRTIO_GPIO_REQ_DIRECTION_OUT		0x05
 #define VIRTIO_GPIO_REQ_GET_VALUE		0x06
 #define VIRTIO_GPIO_REQ_SET_VALUE		0x07
+#define VIRTIO_GPIO_REQ_IRQ_TYPE		0x08
+#define VIRTIO_GPIO_REQ_IRQ_MASK		0x09
+#define VIRTIO_GPIO_REQ_IRQ_UNMASK		0x0a
+#define VIRTIO_GPIO_REQ_IRQ_EVENT		0x0b
+
+/* Virtio GPIO IRQ types */
+#define VIRTIO_GPIO_IRQ_TYPE_NONE		0x00
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING	0x01
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING	0x02
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH		0x03
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH		0x04
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW		0x08
 
 /* Possible values of the status field */
 #define VIRTIO_GPIO_STATUS_OK			0x0