Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Arnd Bergmann <arnd@kernel.org>]

On Mon, Jul 19, 2021 at 11:29 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-07-21, 20:20, Arnd Bergmann wrote:
> > I still don't quite get it. Why would the guest care about a stable name?
>
> For Linux userspace, debugging mostly, at guest userspace. Like what you get out
> of /sys/class/gpio/gpiochipN/label or in debugfs.
>
> i.e. to easily identify the chip there since device names aren't fixed
> otherwise.
>
> Though this was initially added by Enrico and I never though of removing it. If
> you still think it is a waste, will get rid of it.

I still don't see it as an important enough part of the interface to be encoded
in the virtio spec. In the most likely use cases I can think of, you would never
need more than one gpio controller in a virtio machine, since you can have
an arbitrary number of lines connected to it already.

This also simplifies the host side, as it avoids requiring the host to configure
a name that ends up being the same almost always.

If Linus or Bartosz think we should have the name, leave it in there, otherwise
I'd remove it.

> > Right. Any thoughts about just having the names be part of the
> > metadata in the DT? I think that sounds easier than the request,
> > though both methods are fairly straightforward really.
>
> I think we shouldn't divide stuff in between spec and DT as the spec can be used
> without DT as well. And so keeping them entirely in the spec may be better. The
> same holds true otherwise for number of gpio lines as well, or the entire config
> structure.

Encoding the number of gpio lines in the config registers makes sense to me,
but I don't see how you can sensibly get away without some other structure
to describe what the lines are. It doesn't have to be DT, but that's what we use
in practice.

> > That should not be an issue. I think the reply would always be instantaneous
> > here, and the requests can just be processed in order.
>
> They aren't required to be in order after all. Lets say 3 different requests are
> added by 3 different threads in guest, they can all reach in random order at the
> host and the host can send response to them in any order. At least in Linux, you
> can attach a void *data with each transfer and we can easily use that to
> identify the transfer which completed.

Ok, good.

> > Even if the hardware requires the request/free style calls in case of
> > passing through a line to a guest, I would expect that can be handled
> > in host user space, i.e. the virtio device code calls 'request' before it
> > passes down a gpio line to a guest.
>
> That may not always work and this count of refcounting at the backend daemon may
> not be correct.
>
> So, the guest calls set-direction-out-with-val-1, the backend activates the GPIO
> pin, sets direction/value and then deactivates it ? That won't work, right ?
>
> On those lines, I think these operations must be renamed to ACTIVATE/DEACTIVATE
> instead of REQUEST/FREE, that is what we want to do here.

From the drivers you pointed to, it seems to be something like a 'disconnected'
state where the pin is neither input nor output, so this could perhaps
get reflected
in the interface as well, by having each pin be one of IN/OUT/NONE. Not sure
if that is any better.

     Arnd