Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng <jie.deng@intel.com>]

On 2021/3/1 19:54, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
>> +/**
>> + * 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;
>> +};
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?
>
> What about this on top of your patch ?
>
> ---
>   drivers/i2c/busses/i2c-virtio.c | 31 +++++++++++--------------------
>   include/uapi/linux/virtio_i2c.h |  3 +--
>   2 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> index 8c8bc9545418..e71ab1d2c83f 100644
> --- a/drivers/i2c/busses/i2c-virtio.c
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
>   		if (!buf)
>   			break;
>   
> +		reqs[i].buf = buf;
> +		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
>   		if (msgs[i].flags & I2C_M_RD) {
> -			reqs[i].read_buf = buf;
> -			sg_init_one(&msg_buf, reqs[i].read_buf, msgs[i].len);
>   			sgs[outcnt + incnt++] = &msg_buf;
>   		} else {
> -			reqs[i].write_buf = buf;
> -			memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
> -			sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
> +			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
>   			sgs[outcnt++] = &msg_buf;
>   		}
>   
> @@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
>   		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
>   		if (err < 0) {
>   			pr_err("failed to add msg[%d] to virtqueue.\n", i);
> -			if (msgs[i].flags & I2C_M_RD) {
> -				kfree(reqs[i].read_buf);
> -				reqs[i].read_buf = NULL;
> -			} else {
> -				kfree(reqs[i].write_buf);
> -				reqs[i].write_buf = NULL;
> -			}
> +			kfree(reqs[i].buf);
> +			reqs[i].buf = NULL;
>   			break;
>   		}
>   	}
> @@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
>   			break;
>   		}
>   
> -		if (msgs[i].flags & I2C_M_RD) {
> -			memcpy(msgs[i].buf, req->read_buf, msgs[i].len);
> -			kfree(req->read_buf);
> -			req->read_buf = NULL;
> -		} else {
> -			kfree(req->write_buf);
> -			req->write_buf = NULL;
> -		}
> +		if (msgs[i].flags & I2C_M_RD)
> +			memcpy(msgs[i].buf, req->buf, msgs[i].len);
> +
> +		kfree(req->buf);
> +		req->buf = NULL;
>   	}
>   
>   	return i;
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> index 92febf0c527e..61f0086ac75b 100644
> --- a/include/uapi/linux/virtio_i2c.h
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
>    */
>   struct virtio_i2c_req {
>   	struct virtio_i2c_out_hdr out_hdr;
> -	u8 *write_buf;
> -	u8 *read_buf;
> +	u8 *buf;
>   	struct virtio_i2c_in_hdr in_hdr;
>   };
>   

That's my original proposal. I used to mirror this interface with 
"struct i2c_msg".

But the design philosophy of virtio TC is that VIRTIO devices are not 
specific to Linux
so the specs design should avoid the limitations of the current Linux 
driver behavior.

We had some discussion about this. You may check these links to learn 
the story.
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html