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

[Jie Deng]

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.