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

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

 




On 2020/10/12 上午10:45, Jie Deng wrote:


On 2020/10/10 11:14, Jason Wang wrote:

+
+        virtqueue_kick(vq);
+
+        time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
+        if (!time_left) {
+            dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", i, msgs[i].addr);
+            break;
+        }


You don't set error number here. Is this intended?

And using a timeout here is not good, and if the request is finished just after the timeout, in the next xfer you may hit the following check.

It's better to use either interrupt here.

Could you check the I2C drivers in the kernel ? The "wait_for_completion_timeout" mechanism
is commonly used by I2C bus drivers in their i2c_algorithm.master_xfer.


There's a major difference between virtio-i2c and other drivers. In the case of virtio, the device could be a software device emulated by a remote process. This means the timeout might not be rare.

I don't see how timeout is properly handled in this patch (e.g did you notice that you don't set any error when timeout? or is this intended?)




+
+        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
+        /* vmsg should point to the same address with &vi->vmsg */
+        if ((!vmsg) || (vmsg != &vi->vmsg)) {
+            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue error.\n",
+                i, msgs[i].addr);
+            break;
+        }


So I think we can remove this check. Consider only one descriptor will be used at most, unless there's a bug in the device (and no other driver to the similar check), we should not hit this.

Btw, as I replied in the previous version, the device should be cacpable of dealing of a batch of requests through the virtqueue, otherwise it's meaningless to use a queue here.

We should not assume there is no bug in the device. I don't think we can remove this check if we want our code to be robust.


Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?



As I said, currently, we are using the virtqueue to send the msg one by one to the backend. The mechanism is described in the spec.


Which part of the spec describes such "one by one" mechanism? If there is one, I'd happily give a NACK since it doesn't require a queue to work which is conflict with the concept of the virtqueue.


Thanks.



+

+
+#ifndef _UAPI_LINUX_VIRTIO_I2C_H
+#define _UAPI_LINUX_VIRTIO_I2C_H
+
+#include <linux/types.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+/**
+ * struct virtio_i2c_hdr - the virtio I2C message header structure
+ * @addr: i2c_msg addr, the slave address
+ * @flags: i2c_msg flags
+ * @len: i2c_msg len
+ */
+struct virtio_i2c_hdr {
+    __le16 addr;
+    __le16 flags;
+    __le16 len;
+};


I'm afraid this is not complete. E.g the status is missed.

I suspect what virtio-scsi use is better. Which split the in from the out instead of reusing the same buffer. And it can ease the uAPI header export.

Thanks



I think following definition in uAPI for the status is enough.
There is no need to provide a "u8" status in the structure.

/* The final status written by the device */
#define VIRTIO_I2C_MSG_OK    0
#define VIRTIO_I2C_MSG_ERR    1

You can see an example in virtio_blk.

In the spec:

struct virtio_blk_req {
le32 type;
le32 reserved;
le64 sector;
u8 data[];
u8 status;
};

In virtio_blk.h, there is only following definitions.

#define VIRTIO_BLK_S_OK        0
#define VIRTIO_BLK_S_IOERR    1
#define VIRTIO_BLK_S_UNSUPP    2


virtio-blk is a bad example, it's just too late to fix. For any new introduced uAPI it should be a complete one.

Thanks


Thanks.







[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