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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.





[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux