Jason Wang
2020-Dec-07 03:48 UTC
[PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/4 ??5:36, Enrico Weigelt, metux IT consult wrote:> On 04.12.20 04:35, Jason Wang wrote: > > Hi, > >> Is the plan to keep this doc synced with the one in the virtio >> specification? > Yes, of course. I'm still in progress of doing the beaurocratic stuff w/ > virtio-tc folks (ID registration, ...) - yet have to see whether they > wanna add it to their spec documents ... > > BTW: if you feel, sometings not good w/ the current spec, please raise > your voice now.But, has the spec path posted?> >> I think it's better to use u8 ot uint8_t here.Git grep told me the >> former is more popular under Documentation/. > thx, I'll fix that > >>> +- for version field currently only value 1 supported. >>> +- the line names block holds a stream of zero-terminated strings, >>> +? holding the individual line names. >> I'm not sure but does this mean we don't have a fixed length of config >> space? Need to check whether it can bring any trouble to >> migration(compatibility). > Yes, it depends on how many gpio lines are present and how much space > their names take up. > > A fixed size would either put unpleasent limits on the max number of > lines or waste a lot space when only few lines present. > > Not that virtio-gpio is also meant for small embedded workloads running > under some hypervisor. > >>> +- unspecified fields are reserved for future use and should be zero. >>> + >>> +------------------------ >>> +Virtqueues and messages: >>> +------------------------ >>> + >>> +- Queue #0: transmission from host to guest >>> +- Queue #1: transmission from guest to host >> >> Virtio became more a popular in the area without virtualization. So I >> think it's better to use "device/driver" instead of "host/guest" here. > Good point. But I'd prefer "cpu" instead of "driver" in that case. > >> Not a native speaker but event sounds like something driver read from >> device. Looking at the below lists, most of them except for >> VIRTIO_GPIO_EV_HOST_LEVEL looks more like a command. > okay, shall I name it "message" ?It might be better.> >> Another question is, what's the benefit of unifying the message format >> of the two queues. E.g VIRTIO_GPIO_EV_HOST_LEVEL can only works fro rxq. > Simplicity. Those fields that aren't really relevant (eg. replies also > carry the line id), can just be ignored. > >> Not familiar with GPIO but I wonder the value of a standalone >> VIRTIO_GPIO_EV_GUEST_DIRECTION_INPUT/OUTPUT. Can we simply imply them in >> SET/GET_VALUE? > Would introduce more complexity. Somewhere I'd have to fit in some extra > bit for differenciating between line state and line direction. The > direction tells whether the line currently acts as input or output. The > "value" (hmm, maybe I should rethink terminology here) is the current > line level (high/low or active/inactive).Ok.> >>> +---------------------- >>> +Data flow: >>> +---------------------- >>> + >>> +- all operations, except ``VIRTIO_GPIO_EV_HOST_LEVEL``, are >>> guest-initiated >>> +- host replies ``VIRTIO_GPIO_EV_HOST_LEVEL`` OR'ed to the ``type`` field >>> +- ``VIRTIO_GPIO_EV_HOST_LEVEL`` is only sent asynchronically from >>> host to guest >>> +- in replies, a negative ``value`` field denotes an unix-style errno >>> code >> >> Virtio is in a different scope, so we need to define the error code on >> our own. >> >> E.g for virtio-net we define: >> >> >> #define VIRTIO_NET_OK???? 0 >> #define VIRTIO_NET_ERR??? 1 > hmm, so I'd need to define all the error codes that possibly could happen ?Yes, I think you need.> >>> ? +config GPIO_VIRTIO >>> +??? tristate "VirtIO GPIO support" >>> +??? depends on VIRTIO >> >> Let's use select, since there's no prompt for VIRTIO and it doesn't have >> any dependencies. > Ok. I just was under the impression that subsystems and busses should > not be select'ed, but depends on (eg. some time ago tried that w/ gpio > subsys and failed). > >>> +??? help >>> +????? Say Y here to enable guest support for virtio-based GPIOs. >>> + >>> +????? These virtual GPIOs can be routed to real GPIOs or attached to >>> +????? simulators on the host (qemu). >> >> It's better to avoid talking host and qemu here for new virtio devices. > Ok, dropped that line. > >>> +static int virtio_gpio_xmit(struct virtio_gpio_priv *priv, int type, >>> +??????????????? int pin, int value, struct virtio_gpio_event *ev) >>> +{ >>> +??? struct scatterlist sg[1]; >>> +??? int ret; >>> +??? unsigned long flags; >>> + >>> +??? WARN_ON(!ev); >>> + >>> +??? ev->type = type; >>> +??? ev->pin = pin; >>> +??? ev->value = value; >>> + >>> +??? sg_init_table(sg, 1); >>> +??? sg_set_buf(&sg[0], ev, sizeof(struct virtio_gpio_event)); >>> + >>> +??? spin_lock_irqsave(&priv->vq_lock, flags); >>> +??? ret = virtqueue_add_outbuf(priv->vq_tx, sg, ARRAY_SIZE(sg), >>> +?????????????????? priv, GFP_KERNEL); >>> +??? if (ret < 0) { >>> +??????? dev_err(&priv->vdev->dev, >>> +??????????? "virtqueue_add_outbuf() failed: %d\n", ret); >>> +??????? goto out; >> >> So except for the error log, the failure is silently ignored by the >> caller. Is this intended? > ups, I've forgotten the error handling in the caller. fixed in v3. > >>> +static int virtio_gpio_req(struct virtio_gpio_priv *priv, int type, >>> +?????????????? int pin, int value) >>> +{ >>> +??? struct virtio_gpio_event *ev >>> +??????? = kzalloc(&priv->vdev->dev, sizeof(struct virtio_gpio_event), >>> +????????????? GFP_KERNEL); >>> + >>> +??? if (!ev) >>> +??????? return -ENOMEM; >>> + >>> +??? clear_event(priv, type); >>> +??? virtio_gpio_xmit(priv, type, pin, value, ev); >>> +??? wait_event_interruptible(priv->waitq, check_event(priv, type)); >> >> If I read the code correctly, this expects there will be at most a >> single type of event that can be processed at the same time. E.g can >> upper layer want to read from different lines in parallel? If yes, we >> need to deal with that. > @Linus @Bartosz: can that happen or does gpio subsys already serialize > requests ? > > Initially, I tried to protect it by spinlock (so, only one request may > run at a time, other calls just wait until the first is finished), but > it crashed when gpio cdev registration calls into the driver (fetches > the status) while still in bootup. > > Don't recall the exact error anymore, but something like an > inconsistency in the spinlock calls. > > Did I just use the wrong type of lock ?I'm not sure since I am not familiar with GPIO. But a question is, if at most one request is allowed, I'm not sure virtio is the best choice here since we don't even need a queue(virtqueue) here.> >>> +static void virtio_gpio_data_rx(struct virtqueue *vq) >>> +{ >>> +??? struct virtio_gpio_priv *priv = vq->vdev->priv; >>> +??? void *data; >>> +??? unsigned int len; >>> +??? struct virtio_gpio_event *ev; >>> + >>> +??? data = virtqueue_get_buf(priv->vq_rx, &len); >>> +??? if (!data || !len) { >>> +??????? dev_warn(&vq->vdev->dev, "RX received no data ! %d\n", len); >>> +??????? return; >>> +??? } >>> + >>> +??? ev = data; >>> +??? WARN_ON(data != &priv->rcv_buf); >>> + >>> +??? memcpy(&priv->last, &priv->rcv_buf, sizeof(struct >>> virtio_gpio_event)); >>> + >>> +??? switch (ev->type) { >>> +??? case VIRTIO_GPIO_EV_HOST_LEVEL: >>> +??????? virtio_gpio_signal(priv, ev->type, ev->pin, ev->value); >>> +??????? break; >>> +??? default: >>> +??????? wakeup_event(priv, ev->type & ~VIRTIO_GPIO_EV_REPLY); >> >> This looks suspicious, it looks to me what is done here is, consider we >> want to do VIRTIO_GPIO_EV_GUEST_SET_VALUE >> >> 1) put the event in txq, wait >> 2) the result is returned from rxq, wakeup >> >> It looks to me this is racy since the device should be able to process a >> batch of descriptors and there's no guarantee that the descriptor is >> processed in order from the virtio level. > Not sure whether we're on the same page, but: > > VIRTIO_GPIO_EV_HOST_LEVEL is kinda interrupt - it tells cpu when the > input has changed level. We can receive this async event, it shouldn't > matter whether somebody else (another thread) is doing a regular call, > thus waiting for reply at the same time. The reply will be next in > queue. > > What could go wrong here ?I think it's still about whether or not we need allow a batch of requests via a queue. Consider you've submitted two request A and B, and if B is done first, current code won't work. This is because, the reply is transported via rxq buffers not just reuse the txq buffer if I read the code correctly.> > >> I wonder why not introduce two virtqueues: >> >> 1) command vq >> 2) event vq >> >> All commands were sent via command vq and then device can write back to >> the command buffer as other virtio device did. Then there's no worries >> of batching or out of order completion. > I've been under the impression that queues only work in only one > direction. (at least that's what my web research was telling). > > Could you please give an example how bi-directional transmission within > the same queue could look like ?You can check how virtio-blk did this in: https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-2500006> >>> +??????? break; >>> +??? } >>> +??? virtio_gpio_prepare_inbuf(priv); >> >> This assumes at most one event could be generated, is this how GPIO >> device expect to behave? I think level could change several times. > Should I add more buffers ? > > Maybe add one new buffer per request and one new per received async > signal ?It would be safe to fill the whole rxq and do the refill e.g when half of the queue is used.> >>> +static int virtio_gpio_probe(struct virtio_device *vdev) >>> +{ >>> +??? struct virtio_gpio_priv *priv; >>> +??? struct virtio_gpio_config cf = {}; >>> +??? char *name_buffer; >>> +??? const char **gpio_names = NULL; >>> +??? struct device *dev = &vdev->dev; >>> + >>> +??? priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> +??? if (!priv) >>> +??????? return -ENOMEM; >> >> Is devres guaranteed to be enabled here? > How should it not ? Could virtio probing so early that even devm > isn't working yet ?I think you are right, I misread the patch. Thanks> > > --mtx >