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

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

On 2021/3/22 14:41, Viresh Kumar wrote:
>
>> +/**
>> + * struct virtio_i2c - virtio I2C data
>> + * @vdev: virtio device for this controller
>> + * @completion: completion of virtio I2C message
>> + * @adap: I2C adapter for this controller
>> + * @i2c_lock: lock for virtqueue processing
> Name mismatch here.


Will fix this typo. Thank you.


>> + * @vq: the virtio virtqueue for communication
>> + */
>> +struct virtio_i2c {
>> +	struct virtio_device *vdev;
>> +	struct completion completion;
>> +	struct i2c_adapter adap;
>> +	struct mutex lock;
>> +	struct virtqueue *vq;
>> +};
>
>> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
>> +					struct virtio_i2c_req *reqs,
>> +					struct i2c_msg *msgs, int nr,
>> +					bool timeout)
>> +{
>> +	struct virtio_i2c_req *req;
>> +	bool err_found = false;
>> +	unsigned int len;
>> +	int i, j = 0;
>> +
>> +	for (i = 0; i < nr; i++) {
>> +		/* Detach the ith request from the vq */
>> +		req = virtqueue_get_buf(vq, &len);
>> +
>> +		if (timeout || err_found)  {
>> +			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
>> +			continue;
>> +		}
>> +
>> +		/*
>> +		 * Condition (req && req == &reqs[i]) should always meet since
>> +		 * we have total nr requests in the vq.
>> +		 */
>> +		if (WARN_ON(!(req && req == &reqs[i])) ||
>> +		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)) {
>> +			err_found = true;
>> +			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
>> +			continue;
>> +		}
>> +
>> +		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], true);
>> +		++j;
>> +	}
> I think you can simplify the code like this here:


I think your optimization has problems...


> 	bool err_found = timeout;
>
> 	for (i = 0; i < nr; i++) {
> 		/* Detach the ith request from the vq */
> 		req = virtqueue_get_buf(vq, &len);
>
> 		/*
> 		 * Condition (req && req == &reqs[i]) should always meet since
> 		 * we have total nr requests in the vq.
> 		 */
> 		if (!err_found &&
>                      (WARN_ON(!(req && req == &reqs[i])) ||
> 		     (req->in_hdr.status != VIRTIO_I2C_MSG_OK))) {
> 			err_found = true;
> 			continue;


Just continue here, the ith buf leaks ?


> 		}
>
> 		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], err_found);


i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !err_found); ?


>                  if (!err_found)
>                          ++j;
>
>> +
>> +	return (timeout ? -ETIMEDOUT : j);
>> +}
>> +
>> +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);
>> +
>> +	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
>> +	if (!time_left) {
>> +		dev_err(&adap->dev, "virtio i2c backend timeout.\n");
>> +		ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, true);
>> +		goto err_unlock_free;
>> +	}
>> +
>> +	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, false);
> And this can be optimized as well:
>
> 	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> 	if (!time_left)
> 		dev_err(&adap->dev, "virtio i2c backend timeout.\n");
>
>          ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, !time_left);


Good optimization here.