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

From: Jie Deng <jie.deng@intel.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-i2c@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, mst@redhat.com, wsa@kernel.org,
	jasowang@redhat.com, wsa+renesas@sang-engineering.com,
	andriy.shevchenko@linux.intel.com, conghui.chen@intel.com,
	arnd@arndb.de, kblaiech@mellanox.com,
	jarkko.nikula@linux.intel.com, Sergey.Semin@baikalelectronics.ru,
	rppt@kernel.org, loic.poulain@linaro.org, tali.perry1@gmail.com,
	u.kleine-koenig@pengutronix.de, bjorn.andersson@linaro.org,
	yu1.wang@intel.com, shuo.a.liu@intel.com, stefanha@redhat.com,
	pbonzini@redhat.com
Subject: Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
Date: Wed, 24 Mar 2021 14:05:06 +0800	[thread overview]
Message-ID: <70908366-c270-848e-0be3-c85fec7958ec@intel.com> (raw)
In-Reply-To: <20210324042046.idkctj2t7cxi53jf@vireshk-i7>

On 2021/3/24 12:20, Viresh Kumar wrote:
> On 23-03-21, 22:19, Jie Deng wrote:
>> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> +	struct virtio_i2c *vi = i2c_get_adapdata(adap);
>> +	struct virtqueue *vq = vi->vq;
>> +	struct virtio_i2c_req *reqs;
>> +	unsigned long time_left;
>> +	int ret, nr;
>> +
>> +	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
>> +	if (!reqs)
>> +		return -ENOMEM;
>> +
>> +	mutex_lock(&vi->lock);
>> +
>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>> +	if (ret == 0)
>> +		goto err_unlock_free;
>> +
>> +	nr = ret;
>> +	reinit_completion(&vi->completion);
>> +	virtqueue_kick(vq);
> Coming back to this again, what is the expectation from the other side for this
> ? I mean there is no obvious relation between the *msgs* which we are going to
> transfer (from the other side's or host's point of view). When should the host
> OS call its virtqueue_kick() counterpart ?
>
> Lemme give an example for this. Lets say that we need to transfer 3 messages
> here in this routine. What we did was we prepared virtqueue for all 3 messages
> together and then called virtqueue_kick().
>
> Now if the other side (host) processes the first message and sends its reply
> (with virtqueue_kick() counterpart) before processing the other two messages,
> then it will end up calling virtio_i2c_msg_done() here. That will make us call
> virtio_i2c_complete_reqs(), while only the first messages is processed until
> now and so we will fail for the other two messages straight away.
>
> Should we send only 1 message from i2c-virtio linux driver and then wait for
> virtio_i2c_msg_done() to be called, before sending the next message to make sure
> it doesn't break ?

For simplicity, the original patch sent only 1 message to vq each time . 
I changed the way to send

a batch of requests in one time in order to improve efficiency according 
to Jason' suggestion.

As we discussed in the previous emails, the device can raise interrupt 
when some requests are still not completed

though this is not a good operation.  In this case, the remaining 
requests in the vq will be ignored and

the i2c_algorithm. master_xfer will return 1 for your example. I will 
clarify this in the specs.