On 02-03-21, 09:31, Viresh Kumar wrote:
> On 01-03-21, 16:19, Arnd Bergmann wrote:
> > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <[email protected]> wrote:
> >
> > > --- /dev/null
> > > +++ b/include/uapi/linux/virtio_i2c.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> > > +/*
> > > + * Definitions for virtio I2C Adpter
> > > + *
> > > + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> > > +#define _UAPI_LINUX_VIRTIO_I2C_H
> >
> > Why is this a uapi header? Can't this all be moved into the driver
> > itself?
> >
> > > +/**
> > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > + * @out_hdr: the OUT header of the virtio I2C message
> > > + * @write_buf: contains one I2C segment being written to the device
> > > + * @read_buf: contains one I2C segment being read from the device
> > > + * @in_hdr: the IN header of the virtio I2C message
> > > + */
> > > +struct virtio_i2c_req {
> > > + struct virtio_i2c_out_hdr out_hdr;
> > > + u8 *write_buf;
> > > + u8 *read_buf;
> > > + struct virtio_i2c_in_hdr in_hdr;
> > > +};
> >
> > In particular, this structure looks like it is only ever usable between
> > the transfer functions in the driver itself, it is shared with neither
> > user space nor the virtio host side.
>
> Why is it so ? Won't you expect hypervisors or userspace apps to use
> these ?
This comment applies only for the first two structures as the third
one is never exchanged over virtio.
--
viresh
On 2021/3/2 12:22, Viresh Kumar wrote:
> On 02-03-21, 09:31, Viresh Kumar wrote:
>> On 01-03-21, 16:19, Arnd Bergmann wrote:
>>> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <[email protected]> wrote:
>>>
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/virtio_i2c.h
>>>> @@ -0,0 +1,56 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
>>>> +/*
>>>> + * Definitions for virtio I2C Adpter
>>>> + *
>>>> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
>>>> + */
>>>> +
>>>> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
>>>> +#define _UAPI_LINUX_VIRTIO_I2C_H
>>> Why is this a uapi header? Can't this all be moved into the driver
>>> itself?
>>>
>>>> +/**
>>>> + * struct virtio_i2c_req - the virtio I2C request structure
>>>> + * @out_hdr: the OUT header of the virtio I2C message
>>>> + * @write_buf: contains one I2C segment being written to the device
>>>> + * @read_buf: contains one I2C segment being read from the device
>>>> + * @in_hdr: the IN header of the virtio I2C message
>>>> + */
>>>> +struct virtio_i2c_req {
>>>> + struct virtio_i2c_out_hdr out_hdr;
>>>> + u8 *write_buf;
>>>> + u8 *read_buf;
>>>> + struct virtio_i2c_in_hdr in_hdr;
>>>> +};
>>> In particular, this structure looks like it is only ever usable between
>>> the transfer functions in the driver itself, it is shared with neither
>>> user space nor the virtio host side.
>> Why is it so ? Won't you expect hypervisors or userspace apps to use
>> these ?
> This comment applies only for the first two structures as the third
> one is never exchanged over virtio.
Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need
to keep
the first two in uapi and move "struct virtio_i2c_req" into the driver.
But Jason wanted to include "struct virtio_i2c_req" in uapi. He
explained in this link
https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
Do you agree with that explanation ?
On 02-03-21, 13:06, Jie Deng wrote:
> Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
> and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to
> keep
> the first two in uapi and move "struct virtio_i2c_req" into the driver.
>
> But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in
> this link
> https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
> Do you agree with that explanation ?
I am not sure I understood his reasoning well, but it doesn't make any
sense to keep this in uapi header if this is never going to get
transferred over the wire.
Moreover, the struct virtio_i2c_req in spec is misleading to me and
rather creates unnecessary confusion. There is no structure like this
which ever get passed here, but rather there are multiple vq
transactions which take place, one with just the out header, then one
with buffer and finally one with in header.
I am not sure what's the right way of documenting it or if this is a
standard virtio world follows.
--
viresh
On 2021/3/2 1:16 下午, Viresh Kumar wrote:
> On 02-03-21, 13:06, Jie Deng wrote:
>> Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
>> and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to
>> keep
>> the first two in uapi and move "struct virtio_i2c_req" into the driver.
>>
>> But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in
>> this link
>> https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
>> Do you agree with that explanation ?
> I am not sure I understood his reasoning well, but it doesn't make any
> sense to keep this in uapi header if this is never going to get
> transferred over the wire.
I think I was wrong. It should be sufficient have in_hdr and out_hdr in
uAPI.
Thanks
>
> Moreover, the struct virtio_i2c_req in spec is misleading to me and
> rather creates unnecessary confusion. There is no structure like this
> which ever get passed here, but rather there are multiple vq
> transactions which take place, one with just the out header, then one
> with buffer and finally one with in header.
>
> I am not sure what's the right way of documenting it or if this is a
> standard virtio world follows.
>