This patch adds IRQ support for the virtio GPIO driver. Note that this uses the irq_bus_lock/unlock() callbacks, since those operations over virtio may sleep.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Bartosz,
The spec changes are close to merging now, I will let you know once the ballot is closed and the spec changes are merged. You can then pick this patch for 5.16.
V5->V6: - Sent it separately as the other patches are already merged. - Queue the buffers only after enabling the irq (as per latest spec). - Migrate to raw_spinlock_t.
drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-virtio.c | 310 ++++++++++++++++++++++++++++++- include/uapi/linux/virtio_gpio.h | 25 +++ 3 files changed, 332 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index fae5141251e5..bfa723ff8e7c 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1674,6 +1674,7 @@ config GPIO_MOCKUP config GPIO_VIRTIO tristate "VirtIO GPIO support" depends on VIRTIO + select GPIOLIB_IRQCHIP help Say Y here to enable guest support for virtio-based GPIO controllers.
diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c index d24f1c9264bc..73fbe2eda4b9 100644 --- a/drivers/gpio/gpio-virtio.c +++ b/drivers/gpio/gpio-virtio.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/spinlock.h> #include <linux/virtio_config.h> #include <uapi/linux/virtio_gpio.h> #include <uapi/linux/virtio_ids.h> @@ -28,12 +29,30 @@ struct virtio_gpio_line { unsigned int rxlen; };
+struct vgpio_irq_line { + u8 type; + bool disabled; + bool masked; + bool queued; + bool update_pending; + bool queue_pending; + + struct virtio_gpio_irq_request ireq ____cacheline_aligned; + struct virtio_gpio_irq_response ires ____cacheline_aligned; +}; + struct virtio_gpio { struct virtio_device *vdev; struct mutex lock; /* Protects virtqueue operation */ struct gpio_chip gc; struct virtio_gpio_line *lines; struct virtqueue *request_vq; + + /* irq support */ + struct virtqueue *event_vq; + struct mutex irq_lock; /* Protects irq operation */ + raw_spinlock_t eventq_lock; /* Protects queuing of the buffer */ + struct vgpio_irq_line *irq_lines; };
static int _virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, @@ -186,6 +205,244 @@ static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) virtio_gpio_req(vgpio, VIRTIO_GPIO_MSG_SET_VALUE, gpio, value, NULL); }
+/* Interrupt handling */ +static void virtio_gpio_irq_prepare(struct virtio_gpio *vgpio, u16 gpio) +{ + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[gpio]; + struct virtio_gpio_irq_request *ireq = &irq_line->ireq; + struct virtio_gpio_irq_response *ires = &irq_line->ires; + struct scatterlist *sgs[2], req_sg, res_sg; + int ret; + + if (WARN_ON(irq_line->queued || irq_line->masked || irq_line->disabled)) + return; + + ireq->gpio = cpu_to_le16(gpio); + sg_init_one(&req_sg, ireq, sizeof(*ireq)); + sg_init_one(&res_sg, ires, sizeof(*ires)); + sgs[0] = &req_sg; + sgs[1] = &res_sg; + + ret = virtqueue_add_sgs(vgpio->event_vq, sgs, 1, 1, irq_line, GFP_ATOMIC); + if (ret) { + dev_err(&vgpio->vdev->dev, "failed to add request to eventq\n"); + return; + } + + irq_line->queued = true; + virtqueue_kick(vgpio->event_vq); +} + +static void virtio_gpio_irq_enable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; + + raw_spin_lock(&vgpio->eventq_lock); + irq_line->disabled = false; + irq_line->masked = false; + irq_line->queue_pending = true; + raw_spin_unlock(&vgpio->eventq_lock); + + irq_line->update_pending = true; +} + +static void virtio_gpio_irq_disable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; + + raw_spin_lock(&vgpio->eventq_lock); + irq_line->disabled = true; + irq_line->masked = true; + irq_line->queue_pending = false; + raw_spin_unlock(&vgpio->eventq_lock); + + irq_line->update_pending = true; +} + +static void virtio_gpio_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; + + raw_spin_lock(&vgpio->eventq_lock); + irq_line->masked = true; + raw_spin_unlock(&vgpio->eventq_lock); +} + +static void virtio_gpio_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; + + raw_spin_lock(&vgpio->eventq_lock); + irq_line->masked = false; + + /* Queue the buffer unconditionally on unmask */ + virtio_gpio_irq_prepare(vgpio, d->hwirq); + raw_spin_unlock(&vgpio->eventq_lock); +} + +static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; + + switch (type) { + case IRQ_TYPE_NONE: + type = VIRTIO_GPIO_IRQ_TYPE_NONE; + break; + case IRQ_TYPE_EDGE_RISING: + type = VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING; + break; + case IRQ_TYPE_EDGE_FALLING: + type = VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING; + break; + case IRQ_TYPE_EDGE_BOTH: + type = VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH; + break; + case IRQ_TYPE_LEVEL_LOW: + type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW; + break; + case IRQ_TYPE_LEVEL_HIGH: + type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH; + break; + default: + dev_err(&vgpio->vdev->dev, "unsupported irq type: %u\n", type); + return -EINVAL; + } + + irq_line->type = type; + irq_line->update_pending = true; + + return 0; +} + +static void virtio_gpio_irq_bus_lock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + + mutex_lock(&vgpio->irq_lock); +} + +static void virtio_gpio_irq_bus_sync_unlock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct virtio_gpio *vgpio = gpiochip_get_data(gc); + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; + u8 type = irq_line->disabled ? VIRTIO_GPIO_IRQ_TYPE_NONE : irq_line->type; + unsigned long flags; + + if (irq_line->update_pending) { + irq_line->update_pending = false; + virtio_gpio_req(vgpio, VIRTIO_GPIO_MSG_IRQ_TYPE, d->hwirq, type, + NULL); + + /* Queue the buffer only after interrupt is enabled */ + raw_spin_lock_irqsave(&vgpio->eventq_lock, flags); + if (irq_line->queue_pending) { + irq_line->queue_pending = false; + virtio_gpio_irq_prepare(vgpio, d->hwirq); + } + raw_spin_unlock_irqrestore(&vgpio->eventq_lock, flags); + } + + mutex_unlock(&vgpio->irq_lock); +} + +static struct irq_chip vgpio_irq_chip = { + .name = "virtio-gpio", + .irq_enable = virtio_gpio_irq_enable, + .irq_disable = virtio_gpio_irq_disable, + .irq_mask = virtio_gpio_irq_mask, + .irq_unmask = virtio_gpio_irq_unmask, + .irq_set_type = virtio_gpio_irq_set_type, + + /* These are required to implement irqchip for slow busses */ + .irq_bus_lock = virtio_gpio_irq_bus_lock, + .irq_bus_sync_unlock = virtio_gpio_irq_bus_sync_unlock, +}; + +static bool ignore_irq(struct virtio_gpio *vgpio, int gpio, + struct vgpio_irq_line *irq_line) +{ + bool ignore = false; + + raw_spin_lock(&vgpio->eventq_lock); + irq_line->queued = false; + + /* Interrupt is disabled currently */ + if (irq_line->masked || irq_line->disabled) { + ignore = true; + goto unlock; + } + + /* + * Buffer is returned as the interrupt was disabled earlier, but is + * enabled again now. Requeue the buffers. + */ + if (irq_line->ires.status == VIRTIO_GPIO_IRQ_STATUS_INVALID) { + virtio_gpio_irq_prepare(vgpio, gpio); + ignore = true; + goto unlock; + } + + if (WARN_ON(irq_line->ires.status != VIRTIO_GPIO_IRQ_STATUS_VALID)) + ignore = true; + +unlock: + raw_spin_unlock(&vgpio->eventq_lock); + + return ignore; +} + +static void virtio_gpio_event_vq(struct virtqueue *vq) +{ + struct virtio_gpio *vgpio = vq->vdev->priv; + struct device *dev = &vgpio->vdev->dev; + struct vgpio_irq_line *irq_line; + int irq, gpio, ret; + unsigned int len; + + while (true) { + irq_line = virtqueue_get_buf(vgpio->event_vq, &len); + if (!irq_line) + break; + + if (len != sizeof(irq_line->ires)) { + dev_err(dev, "irq with incorrect length (%u : %u)\n", + len, (unsigned int)sizeof(irq_line->ires)); + continue; + } + + /* + * Find GPIO line number from the offset of irq_line within the + * irq_lines block. We can also get GPIO number from + * irq-request, but better not to rely on a buffer returned by + * remote. + */ + gpio = irq_line - vgpio->irq_lines; + WARN_ON(gpio >= vgpio->gc.ngpio); + + if (unlikely(ignore_irq(vgpio, gpio, irq_line))) + continue; + + irq = irq_find_mapping(vgpio->gc.irq.domain, gpio); + WARN_ON(!irq); + + ret = generic_handle_irq(irq); + if (ret) + dev_err(dev, "failed to handle interrupt: %d\n", ret); + }; +} + static void virtio_gpio_request_vq(struct virtqueue *vq) { struct virtio_gpio_line *line; @@ -210,14 +467,15 @@ static void virtio_gpio_free_vqs(struct virtio_device *vdev) static int virtio_gpio_alloc_vqs(struct virtio_gpio *vgpio, struct virtio_device *vdev) { - const char * const names[] = { "requestq" }; + const char * const names[] = { "requestq", "eventq" }; vq_callback_t *cbs[] = { virtio_gpio_request_vq, + virtio_gpio_event_vq, }; - struct virtqueue *vqs[1] = { NULL }; + struct virtqueue *vqs[2] = { NULL, NULL }; int ret;
- ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL); + ret = virtio_find_vqs(vdev, vgpio->irq_lines ? 2 : 1, vqs, cbs, names, NULL); if (ret) { dev_err(&vdev->dev, "failed to find vqs: %d\n", ret); return ret; @@ -225,11 +483,23 @@ static int virtio_gpio_alloc_vqs(struct virtio_gpio *vgpio,
if (!vqs[0]) { dev_err(&vdev->dev, "failed to find requestq vq\n"); - return -ENODEV; + goto out; } vgpio->request_vq = vqs[0];
+ if (vgpio->irq_lines && !vqs[1]) { + dev_err(&vdev->dev, "failed to find eventq vq\n"); + goto out; + } + vgpio->event_vq = vqs[1]; + return 0; + +out: + if (vqs[0] || vqs[1]) + virtio_gpio_free_vqs(vdev); + + return -ENODEV; }
static const char **virtio_gpio_get_names(struct virtio_gpio *vgpio, @@ -325,6 +595,32 @@ static int virtio_gpio_probe(struct virtio_device *vdev) vgpio->gc.owner = THIS_MODULE; vgpio->gc.can_sleep = true;
+ /* Interrupt support */ + if (virtio_has_feature(vdev, VIRTIO_GPIO_F_IRQ)) { + vgpio->irq_lines = devm_kcalloc(dev, ngpio, + sizeof(*vgpio->irq_lines), + GFP_KERNEL); + if (!vgpio->irq_lines) + return -ENOMEM; + + /* 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; + vgpio->gc.irq.chip = &vgpio_irq_chip; + + for (i = 0; i < ngpio; i++) { + vgpio->irq_lines[i].type = VIRTIO_GPIO_IRQ_TYPE_NONE; + vgpio->irq_lines[i].disabled = true; + vgpio->irq_lines[i].masked = true; + } + + mutex_init(&vgpio->irq_lock); + raw_spin_lock_init(&vgpio->eventq_lock); + } + ret = virtio_gpio_alloc_vqs(vgpio, vdev); if (ret) return ret; @@ -357,7 +653,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 0445f905d8cc..d04af9c5f0de 100644 --- a/include/uapi/linux/virtio_gpio.h +++ b/include/uapi/linux/virtio_gpio.h @@ -5,12 +5,16 @@
#include <linux/types.h>
+/* Virtio GPIO Feature bits */ +#define VIRTIO_GPIO_F_IRQ 0 + /* Virtio GPIO request types */ #define VIRTIO_GPIO_MSG_GET_NAMES 0x0001 #define VIRTIO_GPIO_MSG_GET_DIRECTION 0x0002 #define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 #define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 #define VIRTIO_GPIO_MSG_SET_VALUE 0x0005 +#define VIRTIO_GPIO_MSG_IRQ_TYPE 0x0006
/* Possible values of the status field */ #define VIRTIO_GPIO_STATUS_OK 0x0 @@ -21,6 +25,14 @@ #define VIRTIO_GPIO_DIRECTION_OUT 0x01 #define VIRTIO_GPIO_DIRECTION_IN 0x02
+/* 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 + struct virtio_gpio_config { __le16 ngpio; __u8 padding[2]; @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names { __u8 value[]; };
+/* Virtio GPIO IRQ Request / Response */ +struct virtio_gpio_irq_request { + __le16 gpio; +}; + +struct virtio_gpio_irq_response { + __u8 status; +}; + +/* Possible values of the interrupt status field */ +#define VIRTIO_GPIO_IRQ_STATUS_INVALID 0x0 +#define VIRTIO_GPIO_IRQ_STATUS_VALID 0x1 + #endif /* _LINUX_VIRTIO_GPIO_H */
On Wednesday, October 20, 2021, 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 those operations over virtio may sleep.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Bartosz,
The spec changes are close to merging now, I will let you know once the ballot is closed and the spec changes are merged. You can then pick this patch for 5.16.
V5->V6:
- Sent it separately as the other patches are already merged.
- Queue the buffers only after enabling the irq (as per latest spec).
- Migrate to raw_spinlock_t.
drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-virtio.c | 310 ++++++++++++++++++++++++++++++- include/uapi/linux/virtio_gpio.h | 25 +++ 3 files changed, 332 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index fae5141251e5..bfa723ff8e7c 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1674,6 +1674,7 @@ config GPIO_MOCKUP config GPIO_VIRTIO tristate "VirtIO GPIO support" depends on VIRTIO
select GPIOLIB_IRQCHIP help Say Y here to enable guest support for virtio-based GPIOcontrollers.
diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c index d24f1c9264bc..73fbe2eda4b9 100644 --- a/drivers/gpio/gpio-virtio.c +++ b/drivers/gpio/gpio-virtio.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/spinlock.h> #include <linux/virtio_config.h> #include <uapi/linux/virtio_gpio.h> #include <uapi/linux/virtio_ids.h> @@ -28,12 +29,30 @@ struct virtio_gpio_line { unsigned int rxlen; };
+struct vgpio_irq_line {
u8 type;bool disabled;bool masked;bool queued;bool update_pending;bool queue_pending;struct virtio_gpio_irq_request ireq ____cacheline_aligned;struct virtio_gpio_irq_response ires ____cacheline_aligned;+};
struct virtio_gpio { struct virtio_device *vdev; struct mutex lock; /* Protects virtqueue operation */ struct gpio_chip gc; struct virtio_gpio_line *lines; struct virtqueue *request_vq;
/* irq support */struct virtqueue *event_vq;struct mutex irq_lock; /* Protects irq operation */raw_spinlock_t eventq_lock; /* Protects queuing of the buffer */struct vgpio_irq_line *irq_lines;};
static int _virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, @@ -186,6 +205,244 @@ static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) virtio_gpio_req(vgpio, VIRTIO_GPIO_MSG_SET_VALUE, gpio, value, NULL); }
+/* Interrupt handling */ +static void virtio_gpio_irq_prepare(struct virtio_gpio *vgpio, u16 gpio) +{
struct vgpio_irq_line *irq_line = &vgpio->irq_lines[gpio];struct virtio_gpio_irq_request *ireq = &irq_line->ireq;struct virtio_gpio_irq_response *ires = &irq_line->ires;struct scatterlist *sgs[2], req_sg, res_sg;int ret;if (WARN_ON(irq_line->queued || irq_line->masked ||irq_line->disabled))
return;ireq->gpio = cpu_to_le16(gpio);sg_init_one(&req_sg, ireq, sizeof(*ireq));sg_init_one(&res_sg, ires, sizeof(*ires));sgs[0] = &req_sg;sgs[1] = &res_sg;ret = virtqueue_add_sgs(vgpio->event_vq, sgs, 1, 1, irq_line,GFP_ATOMIC);
if (ret) {dev_err(&vgpio->vdev->dev, "failed to add request toeventq\n");
return;}irq_line->queued = true;virtqueue_kick(vgpio->event_vq);+}
+static void virtio_gpio_irq_enable(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];raw_spin_lock(&vgpio->eventq_lock);irq_line->disabled = false;irq_line->masked = false;irq_line->queue_pending = true;raw_spin_unlock(&vgpio->eventq_lock);irq_line->update_pending = true;+}
+static void virtio_gpio_irq_disable(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];raw_spin_lock(&vgpio->eventq_lock);irq_line->disabled = true;irq_line->masked = true;irq_line->queue_pending = false;raw_spin_unlock(&vgpio->eventq_lock);irq_line->update_pending = true;+}
+static void virtio_gpio_irq_mask(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];raw_spin_lock(&vgpio->eventq_lock);irq_line->masked = true;raw_spin_unlock(&vgpio->eventq_lock);+}
+static void virtio_gpio_irq_unmask(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];raw_spin_lock(&vgpio->eventq_lock);irq_line->masked = false;/* Queue the buffer unconditionally on unmask */virtio_gpio_irq_prepare(vgpio, d->hwirq);raw_spin_unlock(&vgpio->eventq_lock);+}
+static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int type) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];switch (type) {case IRQ_TYPE_NONE:type = VIRTIO_GPIO_IRQ_TYPE_NONE;break;
IIRC you add dead code. IRQ framework never calls this if type is not set.
case IRQ_TYPE_EDGE_RISING:type = VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING;break;case IRQ_TYPE_EDGE_FALLING:type = VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING;break;case IRQ_TYPE_EDGE_BOTH:type = VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH;break;case IRQ_TYPE_LEVEL_LOW:type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW;break;case IRQ_TYPE_LEVEL_HIGH:type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH;break;default:dev_err(&vgpio->vdev->dev, "unsupported irq type: %u\n",type);
return -EINVAL;}irq_line->type = type;irq_line->update_pending = true;return 0;+}
+static void virtio_gpio_irq_bus_lock(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);mutex_lock(&vgpio->irq_lock);+}
+static void virtio_gpio_irq_bus_sync_unlock(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];u8 type = irq_line->disabled ? VIRTIO_GPIO_IRQ_TYPE_NONE :irq_line->type;
unsigned long flags;if (irq_line->update_pending) {irq_line->update_pending = false;virtio_gpio_req(vgpio, VIRTIO_GPIO_MSG_IRQ_TYPE, d->hwirq,type,
NULL);/* Queue the buffer only after interrupt is enabled */raw_spin_lock_irqsave(&vgpio->eventq_lock, flags);if (irq_line->queue_pending) {irq_line->queue_pending = false;virtio_gpio_irq_prepare(vgpio, d->hwirq);}raw_spin_unlock_irqrestore(&vgpio->eventq_lock, flags);}mutex_unlock(&vgpio->irq_lock);+}
+static struct irq_chip vgpio_irq_chip = {
.name = "virtio-gpio",.irq_enable = virtio_gpio_irq_enable,.irq_disable = virtio_gpio_irq_disable,.irq_mask = virtio_gpio_irq_mask,.irq_unmask = virtio_gpio_irq_unmask,.irq_set_type = virtio_gpio_irq_set_type,/* These are required to implement irqchip for slow busses */.irq_bus_lock = virtio_gpio_irq_bus_lock,.irq_bus_sync_unlock = virtio_gpio_irq_bus_sync_unlock,+};
+static bool ignore_irq(struct virtio_gpio *vgpio, int gpio,
struct vgpio_irq_line *irq_line)+{
bool ignore = false;raw_spin_lock(&vgpio->eventq_lock);irq_line->queued = false;/* Interrupt is disabled currently */if (irq_line->masked || irq_line->disabled) {ignore = true;goto unlock;}/** Buffer is returned as the interrupt was disabled earlier, but is* enabled again now. Requeue the buffers.*/if (irq_line->ires.status == VIRTIO_GPIO_IRQ_STATUS_INVALID) {virtio_gpio_irq_prepare(vgpio, gpio);ignore = true;goto unlock;}if (WARN_ON(irq_line->ires.status != VIRTIO_GPIO_IRQ_STATUS_VALID))ignore = true;+unlock:
raw_spin_unlock(&vgpio->eventq_lock);return ignore;+}
+static void virtio_gpio_event_vq(struct virtqueue *vq) +{
struct virtio_gpio *vgpio = vq->vdev->priv;struct device *dev = &vgpio->vdev->dev;struct vgpio_irq_line *irq_line;int irq, gpio, ret;unsigned int len;while (true) {irq_line = virtqueue_get_buf(vgpio->event_vq, &len);if (!irq_line)break;if (len != sizeof(irq_line->ires)) {dev_err(dev, "irq with incorrect length (%u :%u)\n",
len, (unsigned int)sizeof(irq_line->ires));continue;}/** Find GPIO line number from the offset of irq_linewithin the
* irq_lines block. We can also get GPIO number from* irq-request, but better not to rely on a bufferreturned by
* remote.*/gpio = irq_line - vgpio->irq_lines;WARN_ON(gpio >= vgpio->gc.ngpio);if (unlikely(ignore_irq(vgpio, gpio, irq_line)))continue;irq = irq_find_mapping(vgpio->gc.irq.domain, gpio);WARN_ON(!irq);ret = generic_handle_irq(irq);
IIRC there is a new API that basically combines the two above.
if (ret)dev_err(dev, "failed to handle interrupt: %d\n",ret);
};+}
static void virtio_gpio_request_vq(struct virtqueue *vq) { struct virtio_gpio_line *line; @@ -210,14 +467,15 @@ static void virtio_gpio_free_vqs(struct virtio_device *vdev) static int virtio_gpio_alloc_vqs(struct virtio_gpio *vgpio, struct virtio_device *vdev) {
const char * const names[] = { "requestq" };
const char * const names[] = { "requestq", "eventq" }; vq_callback_t *cbs[] = { virtio_gpio_request_vq,virtio_gpio_event_vq, };
struct virtqueue *vqs[1] = { NULL };
struct virtqueue *vqs[2] = { NULL, NULL }; int ret;
ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL);
ret = virtio_find_vqs(vdev, vgpio->irq_lines ? 2 : 1, vqs, cbs,names, NULL); if (ret) { dev_err(&vdev->dev, "failed to find vqs: %d\n", ret); return ret; @@ -225,11 +483,23 @@ static int virtio_gpio_alloc_vqs(struct virtio_gpio *vgpio,
if (!vqs[0]) { dev_err(&vdev->dev, "failed to find requestq vq\n");
return -ENODEV;
goto out; } vgpio->request_vq = vqs[0];if (vgpio->irq_lines && !vqs[1]) {dev_err(&vdev->dev, "failed to find eventq vq\n");goto out;}vgpio->event_vq = vqs[1];return 0;+out:
if (vqs[0] || vqs[1])virtio_gpio_free_vqs(vdev);return -ENODEV;}
static const char **virtio_gpio_get_names(struct virtio_gpio *vgpio, @@ -325,6 +595,32 @@ static int virtio_gpio_probe(struct virtio_device *vdev) vgpio->gc.owner = THIS_MODULE; vgpio->gc.can_sleep = true;
/* Interrupt support */if (virtio_has_feature(vdev, VIRTIO_GPIO_F_IRQ)) {vgpio->irq_lines = devm_kcalloc(dev, ngpio,sizeof(*vgpio->irq_lines),GFP_KERNEL);
One line?
if (!vgpio->irq_lines)return -ENOMEM;/* 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;vgpio->gc.irq.chip = &vgpio_irq_chip;for (i = 0; i < ngpio; i++) {vgpio->irq_lines[i].type =VIRTIO_GPIO_IRQ_TYPE_NONE;
vgpio->irq_lines[i].disabled = true;vgpio->irq_lines[i].masked = true;}mutex_init(&vgpio->irq_lock);raw_spin_lock_init(&vgpio->eventq_lock);}ret = virtio_gpio_alloc_vqs(vgpio, vdev); if (ret) return ret;@@ -357,7 +653,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 0445f905d8cc..d04af9c5f0de 100644 --- a/include/uapi/linux/virtio_gpio.h +++ b/include/uapi/linux/virtio_gpio.h @@ -5,12 +5,16 @@
#include <linux/types.h>
+/* Virtio GPIO Feature bits */ +#define VIRTIO_GPIO_F_IRQ 0
/* Virtio GPIO request types */ #define VIRTIO_GPIO_MSG_GET_NAMES 0x0001 #define VIRTIO_GPIO_MSG_GET_DIRECTION 0x0002 #define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 #define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 #define VIRTIO_GPIO_MSG_SET_VALUE 0x0005 +#define VIRTIO_GPIO_MSG_IRQ_TYPE 0x0006
/* Possible values of the status field */ #define VIRTIO_GPIO_STATUS_OK 0x0 @@ -21,6 +25,14 @@ #define VIRTIO_GPIO_DIRECTION_OUT 0x01 #define VIRTIO_GPIO_DIRECTION_IN 0x02
+/* 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
struct virtio_gpio_config { __le16 ngpio; __u8 padding[2]; @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names { __u8 value[]; };
+/* Virtio GPIO IRQ Request / Response */ +struct virtio_gpio_irq_request {
__le16 gpio;+};
+struct virtio_gpio_irq_response {
__u8 status;+};
I’m wondering if those above should be packed.
+/* Possible values of the interrupt status field */ +#define VIRTIO_GPIO_IRQ_STATUS_INVALID 0x0 +#define VIRTIO_GPIO_IRQ_STATUS_VALID 0x1
#endif /* _LINUX_VIRTIO_GPIO_H */
2.31.1.272.g89b43f80a514
On 20-10-21, 18:10, Andy Shevchenko wrote:
On Wednesday, October 20, 2021, Viresh Kumar viresh.kumar@linaro.org wrote:
+static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int type) +{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);struct virtio_gpio *vgpio = gpiochip_get_data(gc);struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];switch (type) {case IRQ_TYPE_NONE:type = VIRTIO_GPIO_IRQ_TYPE_NONE;break;IIRC you add dead code. IRQ framework never calls this if type is not set.
Yes, but it is allowed to call
irq_set_irq_type(irq, IRQ_TYPE_NONE);
and the irq framework won't disallow it AFAICT.
+static void virtio_gpio_event_vq(struct virtqueue *vq) +{
irq = irq_find_mapping(vgpio->gc.irq.domain, gpio);WARN_ON(!irq);ret = generic_handle_irq(irq);IIRC there is a new API that basically combines the two above.
generic_handle_domain_irq(), thanks.
struct virtio_gpio_config { __le16 ngpio; __u8 padding[2]; @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names { __u8 value[]; };
+/* Virtio GPIO IRQ Request / Response */ +struct virtio_gpio_irq_request {
__le16 gpio;+};
+struct virtio_gpio_irq_response {
__u8 status;+};
I’m wondering if those above should be packed.
You are talking about the newly added ones or the ones before ?
In any case, they are all already packed (i.e. they have explicit padding wherever required) and properly aligned. Compiler won't add any other padding to them.
On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 20-10-21, 18:10, Andy Shevchenko wrote:
On Wednesday, October 20, 2021, Viresh Kumar viresh.kumar@linaro.org wrote:
...
case IRQ_TYPE_NONE:type = VIRTIO_GPIO_IRQ_TYPE_NONE;break;IIRC you add dead code. IRQ framework never calls this if type is not set.
Yes, but it is allowed to call
irq_set_irq_type(irq, IRQ_TYPE_NONE);
and the irq framework won't disallow it AFAICT.
That's true, but how you may end up in this callback with a such? What the meaning of that call to the user?
...
struct virtio_gpio_config { __le16 ngpio; __u8 padding[2]; @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names { __u8 value[]; };
+/* Virtio GPIO IRQ Request / Response */ +struct virtio_gpio_irq_request {
__le16 gpio;+};
+struct virtio_gpio_irq_response {
__u8 status;+};
I’m wondering if those above should be packed.
You are talking about the newly added ones or the ones before ?
In any case, they are all already packed (i.e. they have explicit padding wherever required) and properly aligned. Compiler won't add any other padding to them.
Is it only for 64-bit to 64-bit communications? If there is a possibility to have 32-bit to 64-bit or vice versa communication you have a problem.
On 21-10-21, 12:42, Andy Shevchenko wrote:
On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 20-10-21, 18:10, Andy Shevchenko wrote:
IIRC you add dead code. IRQ framework never calls this if type is not set.
Yes, but it is allowed to call
irq_set_irq_type(irq, IRQ_TYPE_NONE);
and the irq framework won't disallow it AFAICT.
That's true, but how you may end up in this callback with a such? What the meaning of that call to the user?
I can see few calls like this in the kernel (mostly from irq-providers only), but yeah sure I can drop it. We will error out if it ever gets called and so can get it back later if required.
struct virtio_gpio_config { __le16 ngpio; __u8 padding[2]; @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names { __u8 value[]; };
+/* Virtio GPIO IRQ Request / Response */ +struct virtio_gpio_irq_request {
__le16 gpio;+};
+struct virtio_gpio_irq_response {
__u8 status;+};
I’m wondering if those above should be packed.
You are talking about the newly added ones or the ones before ?
In any case, they are all already packed (i.e. they have explicit padding wherever required) and properly aligned. Compiler won't add any other padding to them.
Is it only for 64-bit to 64-bit communications?
That's what I have been looking at.
If there is a possibility to have 32-bit to 64-bit or vice versa communication you have a problem.
This should work as well.
The structure will get aligned to the size of largest element and each element will be aligned to itself. I don't see how this will break even in case of 32/64 bit communication.
On Thu, Oct 21, 2021 at 12:52 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 21-10-21, 12:42, Andy Shevchenko wrote:
On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 20-10-21, 18:10, Andy Shevchenko wrote:
...
struct virtio_gpio_config { __le16 ngpio; __u8 padding[2]; @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names { __u8 value[]; };
+/* Virtio GPIO IRQ Request / Response */ +struct virtio_gpio_irq_request {
__le16 gpio;+};
+struct virtio_gpio_irq_response {
__u8 status;+};
I’m wondering if those above should be packed.
You are talking about the newly added ones or the ones before ?
In any case, they are all already packed (i.e. they have explicit padding wherever required) and properly aligned. Compiler won't add any other padding to them.
Is it only for 64-bit to 64-bit communications?
That's what I have been looking at.
If there is a possibility to have 32-bit to 64-bit or vice versa communication you have a problem.
This should work as well.
The structure will get aligned to the size of largest element and each element will be aligned to itself. I don't see how this will break even in case of 32/64 bit communication.
I admit I haven't looked into the specification, but in the past we had had quite an issue exactly in GPIO on kernel side because of this kind of design mistake. The problem here if in the future one wants to supply more than one item at a time, it will be not possible with this interface. Yes, I understand that in current design it's rather missed scalability, but hey, I believe in the future we may need performance-wise calls.
On 21-10-21, 12:58, Andy Shevchenko wrote:
I admit I haven't looked into the specification, but in the past we had had quite an issue exactly in GPIO on kernel side because of this kind of design mistake.
The problem here if in the future one wants to supply more than one item at a time, it will be not possible with this interface.
Why ?
Yes, I understand that in current design it's rather missed scalability, but hey, I believe in the future we may need performance-wise calls.
On Thu, Oct 21, 2021 at 11:58 AM Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Oct 21, 2021 at 12:52 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 21-10-21, 12:42, Andy Shevchenko wrote:
On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 20-10-21, 18:10, Andy Shevchenko wrote:
If there is a possibility to have 32-bit to 64-bit or vice versa communication you have a problem.
This should work as well.
The structure will get aligned to the size of largest element and each element will be aligned to itself. I don't see how this will break even in case of 32/64 bit communication.
I admit I haven't looked into the specification, but in the past we had had quite an issue exactly in GPIO on kernel side because of this kind of design mistake. The problem here if in the future one wants to supply more than one item at a time, it will be not possible with this interface. Yes, I understand that in current design it's rather missed scalability, but hey, I believe in the future we may need performance-wise calls.
In my experience, adding __packed to structures causes more problems than it solves, please don't do that.
The rules for the virtio structures should be roughly the same that I documented in Documentation/driver-api/ioctl.rst, and the layout that Viresh has picked does not suffer from any of the common issues that are listed there.
Arnd
Hi Viresh,
On Thu, Oct 21, 2021 at 11:52 AM Viresh Kumar viresh.kumar@linaro.org wrote:
The structure will get aligned to the size of largest element and each element will be aligned to itself. I don't see how this will break even in case of 32/64 bit communication.
Structures are not aligned to the size of the largest element, but to the largest alignment needed for each member. This can be smaller than the size of the largest element. E.g. alignof(long long) might be 4, not 8. And m68k aligns to two bytes at most.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 21-10-21, 12:07, Geert Uytterhoeven wrote:
On Thu, Oct 21, 2021 at 11:52 AM Viresh Kumar viresh.kumar@linaro.org wrote:
The structure will get aligned to the size of largest element and each element will be aligned to itself. I don't see how this will break even in case of 32/64 bit communication.
Structures are not aligned to the size of the largest element, but to the largest alignment needed for each member.
Right, I was talking in terms of the structures we have here for GPIO. The biggest member here (for any structure) is 32bits long, and compiler shouldn't add extra padding here.
This can be smaller than the size of the largest element. E.g. alignof(long long) might be 4, not 8.
Right.
And m68k aligns to two bytes at most.
Interesting, I assumed that it will be 4bytes for 32 bit systems. So in case of m68k, we will see something like this ?
struct foo { u8 a; // aligned to 2 bytes
// padding of 1 byte
u32 b; // aligned to 2 bytes }
Hi Viresh,
On Thu, Oct 21, 2021 at 12:49 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 21-10-21, 12:07, Geert Uytterhoeven wrote:
On Thu, Oct 21, 2021 at 11:52 AM Viresh Kumar viresh.kumar@linaro.org wrote:
The structure will get aligned to the size of largest element and each element will be aligned to itself. I don't see how this will break even in case of 32/64 bit communication.
Structures are not aligned to the size of the largest element, but to the largest alignment needed for each member.
Right, I was talking in terms of the structures we have here for GPIO. The biggest member here (for any structure) is 32bits long, and compiler shouldn't add extra padding here.
This can be smaller than the size of the largest element. E.g. alignof(long long) might be 4, not 8.
Right.
And m68k aligns to two bytes at most.
Interesting, I assumed that it will be 4bytes for 32 bit systems. So in case of m68k, we will see something like this ?
struct foo { u8 a; // aligned to 2 bytes
// padding of 1 byte u32 b; // aligned to 2 bytes}
Exactly. And on CRIS (no longer supported by Linux), there won't be any padding.
So I recommend to always add explicit padding, to make sure all members are aligned naturally on all systems.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
stratos-dev@op-lists.linaro.org
-
Andy Shevchenko -
Arnd Bergmann -
Geert Uytterhoeven -
Viresh Kumar