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.