| Message ID | cover.1623326176.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | gpio: Add virtio based driver | expand |
Hi, Not being very familiar with GPIO, I just have a few general comments and one on the config space layout On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote: > +static int virtio_gpio_req(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; > + struct scatterlist *sgs[2], req_sg, res_sg; > + struct device *dev = &vgpio->vdev->dev; > + unsigned long time_left; > + unsigned int len; > + int ret; > + > + req->type = cpu_to_le16(type); > + req->gpio = cpu_to_le16(gpio); > + req->data = txdata; > + > + sg_init_one(&req_sg, req, sizeof(*req)); > + sg_init_one(&res_sg, res, sizeof(*res)); > + 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; > + } > + > + reinit_completion(&vgpio->completion); > + virtqueue_kick(vgpio->command_vq); > + > + time_left = wait_for_completion_timeout(&vgpio->completion, HZ / 10); > + if (!time_left) { > + dev_err(dev, "virtio GPIO backend timeout\n"); > + return -ETIMEDOUT; mutex is still held > + } > + > + WARN_ON(res != virtqueue_get_buf(vgpio->command_vq, &len)); > + if (unlikely(res->status != VIRTIO_GPIO_STATUS_OK)) { > + dev_err(dev, "virtio GPIO request failed: %d\n", gpio); > + return -EINVAL; and here > + } > + > + if (rxdata) > + *rxdata = res->data; > + > +out: > + mutex_unlock(&vgpio->lock); > + > + return ret; > +} > + > +static int virtio_gpio_request(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + > + return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_ACTIVATE, gpio, 0, NULL); > +} > + > +static void virtio_gpio_free(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + > + virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DEACTIVATE, gpio, 0, NULL); > +} > + > +static int virtio_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + u8 direction; > + int ret; > + > + ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_DIRECTION, gpio, 0, > + &direction); > + if (ret) > + return ret; > + > + return direction; > +} > + > +static int virtio_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + > + return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_IN, gpio, 0, > + NULL); > +} > + > +static int virtio_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, > + int value) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + > + return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_OUT, gpio, (u8) (that dangling cast looks a bit odd to me) > + value, NULL); > +} > + > +static int virtio_gpio_get(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + u8 value; > + int ret; > + > + ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_VALUE, gpio, 0, &value); > + if (ret) > + return ret; > + > + return value; > +} > + > +static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) > +{ > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + > + virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_SET_VALUE, gpio, value, NULL); > +} > + > +static void virtio_gpio_command(struct virtqueue *vq) > +{ > + struct virtio_gpio *vgpio = vq->vdev->priv; > + > + complete(&vgpio->completion); > +} > + > +static int virtio_gpio_alloc_vqs(struct virtio_device *vdev) > +{ > + struct virtio_gpio *vgpio = vdev->priv; > + const char * const names[] = { "command" }; > + vq_callback_t *cbs[] = { > + virtio_gpio_command, > + }; > + struct virtqueue *vqs[1] = {NULL}; > + int ret; > + > + ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL); > + if (ret) { > + dev_err(&vdev->dev, "failed to allocate vqs: %d\n", ret); > + return ret; > + } > + > + vgpio->command_vq = vqs[0]; > + > + /* Mark the device ready to perform operations from within probe() */ > + virtio_device_ready(vgpio->vdev); May fit better in the parent function > + return 0; > +} > + > +static void virtio_gpio_free_vqs(struct virtio_device *vdev) > +{ > + vdev->config->reset(vdev); > + vdev->config->del_vqs(vdev); > +} > + > +static const char **parse_gpio_names(struct virtio_device *vdev, > + struct virtio_gpio_config *config) > +{ > + struct device *dev = &vdev->dev; > + char *str = config->gpio_names; > + const char **names; > + int i, len; > + > + if (!config->gpio_names_size) > + return NULL; > + > + names = devm_kcalloc(dev, config->ngpio, sizeof(names), GFP_KERNEL); > + if (!names) > + return ERR_PTR(-ENOMEM); > + > + /* NULL terminate the string instead of checking it */ > + config->gpio_names[config->gpio_names_size - 1] = '\0'; > + > + for (i = 0; i < config->ngpio; i++) { > + /* > + * We have already marked the last byte with '\0' > + * earlier, this shall be enough here. > + */ > + if (str == config->gpio_names + config->gpio_names_size) { > + dev_err(dev, "Invalid gpio_names\n"); > + return ERR_PTR(-EINVAL); > + } > + > + len = strlen(str); > + if (!len) { > + dev_err(dev, "Missing GPIO name: %d\n", i); > + return ERR_PTR(-EINVAL); > + } > + > + names[i] = str; > + str += len + 1; > + } > + > + return names; > +} > + > +static int virtio_gpio_probe(struct virtio_device *vdev) > +{ > + struct virtio_gpio_config *config; > + struct device *dev = &vdev->dev; > + struct virtio_gpio *vgpio; > + u32 size; > + int ret; > + > + virtio_cread(vdev, struct virtio_gpio_config, gpio_names_size, &size); > + size = cpu_to_le32(size); le32_to_cpu()? > + > + vgpio = devm_kzalloc(dev, sizeof(*vgpio) + size, GFP_KERNEL); > + if (!vgpio) > + return -ENOMEM; > + > + config = &vgpio->config; > + > + /* Read configuration */ > + virtio_cread_bytes(vdev, 0, config, sizeof(*config) + size); > + > + /* NULL terminate the string instead of checking it */ > + config->name[sizeof(config->name) - 1] = '\0'; > + config->ngpio = cpu_to_le16(config->ngpio); > + config->gpio_names_size = cpu_to_le32(config->gpio_names_size); leXX_to_cpu()? > + > + if (!config->ngpio) { > + dev_err(dev, "Number of GPIOs can't be zero\n"); > + return -EINVAL; > + } > + > + vgpio->vdev = vdev; > + vgpio->gc.request = virtio_gpio_request; > + vgpio->gc.free = virtio_gpio_free; > + vgpio->gc.get_direction = virtio_gpio_get_direction; > + vgpio->gc.direction_input = virtio_gpio_direction_input; > + vgpio->gc.direction_output = virtio_gpio_direction_output; > + vgpio->gc.get = virtio_gpio_get; > + vgpio->gc.set = virtio_gpio_set; > + vgpio->gc.ngpio = config->ngpio; > + vgpio->gc.base = -1; /* Allocate base dynamically */ > + vgpio->gc.label = config->name[0] ? > + config->name : dev_name(dev); > + vgpio->gc.parent = dev; > + vgpio->gc.owner = THIS_MODULE; > + vgpio->gc.can_sleep = true; > + vgpio->gc.names = parse_gpio_names(vdev, config); > + if (IS_ERR(vgpio->gc.names)) > + return PTR_ERR(vgpio->gc.names); > + > + vdev->priv = vgpio; > + mutex_init(&vgpio->lock); > + init_completion(&vgpio->completion); > + > + ret = virtio_gpio_alloc_vqs(vdev); > + if (ret) > + return ret; > + > + ret = gpiochip_add(&vgpio->gc); > + if (ret) { > + virtio_gpio_free_vqs(vdev); > + dev_err(dev, "Failed to add virtio-gpio controller\n"); > + } > + > + return ret; > +} > + > +static void virtio_gpio_remove(struct virtio_device *vdev) > +{ > + struct virtio_gpio *vgpio = vdev->priv; > + > + gpiochip_remove(&vgpio->gc); > + virtio_gpio_free_vqs(vdev); > +} > + > +static const struct virtio_device_id id_table[] = { > + { VIRTIO_ID_GPIO, VIRTIO_DEV_ANY_ID }, > + {}, > +}; > +MODULE_DEVICE_TABLE(virtio, id_table); > + > +static struct virtio_driver virtio_gpio_driver = { > + .id_table = id_table, > + .probe = virtio_gpio_probe, > + .remove = virtio_gpio_remove, > + .driver = { > + .name = KBUILD_MODNAME, > + .owner = THIS_MODULE, > + }, > +}; > +module_virtio_driver(virtio_gpio_driver); > + > +MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>"); > +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.org>"); > +MODULE_DESCRIPTION("VirtIO GPIO driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_gpio.h > new file mode 100644 > index 000000000000..e805e33a79cb > --- /dev/null > +++ b/include/uapi/linux/virtio_gpio.h > @@ -0,0 +1,41 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > + > +#ifndef _LINUX_VIRTIO_GPIO_H > +#define _LINUX_VIRTIO_GPIO_H > + > +#include <linux/types.h> > + > +/* Virtio GPIO request types */ > +#define VIRTIO_GPIO_REQ_ACTIVATE 0x01 > +#define VIRTIO_GPIO_REQ_DEACTIVATE 0x02 > +#define VIRTIO_GPIO_REQ_GET_DIRECTION 0x03 > +#define VIRTIO_GPIO_REQ_DIRECTION_IN 0x04 > +#define VIRTIO_GPIO_REQ_DIRECTION_OUT 0x05 > +#define VIRTIO_GPIO_REQ_GET_VALUE 0x06 > +#define VIRTIO_GPIO_REQ_SET_VALUE 0x07 > + > +/* Possible values of the status field */ > +#define VIRTIO_GPIO_STATUS_OK 0x0 > +#define VIRTIO_GPIO_STATUS_ERR 0x1 > + > +struct virtio_gpio_config { > + char name[32]; > + __u16 ngpio; > + __u16 padding; > + __u32 gpio_names_size; > + char gpio_names[0]; A variable-size array here will make it very difficult to append new fields to virtio_gpio_config, when adding new features to the device. (New fields cannot be inserted in between, since older drivers will expect existing fields at a specific offset.) You could replace it with a reference to the string array, for example "__u16 gpio_names_offset" declaring the offset between the beginning of device-specific config and the string array. The 'name' field could also be indirect to avoid setting a fixed 32-char size, but that's not as important. > +} __packed; No need for __packed, because the fields are naturally aligned (as required by the virtio spec) Thanks, Jean > + > +/* Virtio GPIO Request / Response */ > +struct virtio_gpio_request { > + __u16 type; > + __u16 gpio; > + __u8 data; > +}; > + > +struct virtio_gpio_response { > + __u8 status; > + __u8 data; > +}; > + > +#endif /* _LINUX_VIRTIO_GPIO_H */ > diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h > index b89391dff6c9..0c24e8ae2499 100644 > --- a/include/uapi/linux/virtio_ids.h > +++ b/include/uapi/linux/virtio_ids.h > @@ -55,5 +55,6 @@ > #define VIRTIO_ID_PMEM 27 /* virtio pmem */ > #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */ > #define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */ > +#define VIRTIO_ID_GPIO 41 /* virtio GPIO */ > > #endif /* _LINUX_VIRTIO_IDS_H */ > -- > 2.31.1.272.g89b43f80a514 > > -- > Stratos-dev mailing list > Stratos-dev@op-lists.linaro.org > https://op-lists.linaro.org/mailman/listinfo/stratos-dev
On 10-06-21, 19:40, Jean-Philippe Brucker wrote: > On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote: Fixed everything else you suggested. > > +struct virtio_gpio_config { > > + char name[32]; > > + __u16 ngpio; > > + __u16 padding; > > + __u32 gpio_names_size; > > + char gpio_names[0]; > > A variable-size array here will make it very difficult to append new > fields to virtio_gpio_config, when adding new features to the device. (New > fields cannot be inserted in between, since older drivers will expect > existing fields at a specific offset.) Yes, I thought about that earlier and though maybe we will be able to play with that using the virtio-features, I mean a different layout of config structure if we really need to add a field in config, based on the feature flag. > You could replace it with a reference to the string array, for example > "__u16 gpio_names_offset" declaring the offset between the beginning of > device-specific config and the string array. But, I like this idea more and it does make it very flexible. Will adapt to it. > The 'name' field could also be indirect to avoid setting a fixed > 32-char size, but that's not as important. Yeah, 32 bytes is really enough. One won't be able to make any sense out of a bigger name anyway :) > > +} __packed; > > No need for __packed, because the fields are naturally aligned (as > required by the virtio spec) Yeah, I know, but I tend to add that for structures which aren't very simple (like the request/response ones), just to avoid human errors and hours of debugging someone need to go through. __packed won't harm at least :) -- viresh
On Fri, Jun 11, 2021 at 5:39 AM Viresh Kumar via Stratos-dev <stratos-dev@op-lists.linaro.org> wrote: > On 10-06-21, 19:40, Jean-Philippe Brucker wrote: > > On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote: > > > +} __packed; > > > > No need for __packed, because the fields are naturally aligned (as > > required by the virtio spec) > > Yeah, I know, but I tend to add that for structures which aren't very > simple (like the request/response ones), just to avoid human errors > and hours of debugging someone need to go through. __packed won't harm > at least :) Extraneous __packed annotations do cause real problems: - On architectures without hardware unaligned accesses, the compiler is forced to emit byte load/store instructions, which is slower and breaks atomic updates to shared variables - If a function takes a pointer of a packed struct member, and passes that pointer to a function that expects a regular aligned pointer, you get undefined behavior. Newer compilers produce a warning if you do that (we currently shut up that warning because there are many false positives in the kernel), but you can also run into CPU exceptions or broken code even on CPUs that do support unaligned accesses when the variable ends up being actually unaligned (as you just told the compiler that it is allowed to do). Arnd
On 11-06-21, 10:34, Arnd Bergmann wrote: > Extraneous __packed annotations do cause real problems: > > - On architectures without hardware unaligned accesses, the compiler is > forced to emit byte load/store instructions, which is slower and breaks > atomic updates to shared variables > > - If a function takes a pointer of a packed struct member, and passes that > pointer to a function that expects a regular aligned pointer, you > get undefined > behavior. Newer compilers produce a warning if you do that (we currently > shut up that warning because there are many false positives in the kernel), > but you can also run into CPU exceptions or broken code even on CPUs > that do support unaligned accesses when the variable ends up being > actually unaligned (as you just told the compiler that it is allowed to do). I understand that these problems will happen if the structure isn't aligned, but in this case the structure is aligned properly, just that we are explicitly telling the compiler to not add any padding (it won't have added any in here). Is it still harmful ? -- viresh