Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver

[Jason Wang <jasowang@redhat.com>]

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
>