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.
