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

On 2021/3/5 11:09, Viresh Kumar wrote:

On 05-03-21, 09:46, Jie Deng wrote:

quoted

On 2021/3/4 14:06, Viresh Kumar wrote:

quoted

depends on I2C as well ?

No need that. The dependency of I2C is included in the Kconfig in its parent
directory.

Sorry about that, I must have figured that out myself.

(Though a note on the way we reply to messages, please leave an empty line
before and after your messages, it gets difficult to find the inline replies
otherwise. )

quoted

quoted

quoted

+		if (!(req && req == &reqs[i])) {

I find this less readable compared to:
		if (!req || req != &reqs[i]) {

Different people may have different tastes.

Please check Andy's comment in this link.

https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html

Heh, everyone wants you to do it differently :)

If we leave compilers optimizations aside (because it will generate the exact
same code for both the cases, I tested it as well to be doubly sure), The
original statement used in this patch has 3 conditional statements in it and the
way I suggested has only two.

Andy, thoughts ?

And anyway, this isn't biggest of my worries, just that I had to notice it
somehow :)

quoted

quoted

For all the above errors where you simply break out, you still need to free the
memory for buf, right ?

Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.

I think it would be better to have all such deallocations done at a single
place, i.e. after the completion callback is finished.. Trying to solve this
everywhere is going to make this more messy.

I think there is no need to have allocations/deallocations/memcpy for 
reqs[i].buf any more
if using msgs[i].buf directly.

quoted

quoted

quoted

+	mutex_lock(&vi->i2c_lock);

I have never worked with i2c stuff earlier, but I don't think you need a lock
here. The transactions seem to be serialized by the i2c-core by itself (look at
i2c_transfer() in i2c-core-base.c), though there is another exported version
__i2c_transfer() but the comment over it says the callers must take adapter lock
before calling it.

Lock is needed since no "lock_ops" is registered in this i2c_adapter.

drivers/i2c/i2c-core-base.c:

static int i2c_register_adapter(struct i2c_adapter *adap)
{
         ...

         if (!adap->lock_ops)
                 adap->lock_ops = &i2c_adapter_lock_ops;

         ...
}

This should take care of it ?

The problem is that you can't guarantee that adap->algo->master_xfer is 
only called
from i2c_transfer. Any function who holds the adapter can call 
adap->algo->master_xfer
directly. So I think it is safer to have a lock in virtio_i2c_xfer.

quoted

quoted

quoted

+
+	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+	if (ret == 0)
+		goto err_unlock_free;
+
+	nr = ret;
+
+	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 = -ETIMEDOUT;

You need to free bufs of the requests here as well..

Just want to make sure you didn't miss this comment.

Will try to use msgs[i].buf directly. So it should be no free bufs any more.

quoted

quoted

quoted

+static struct i2c_adapter virtio_adapter = {
+	.owner = THIS_MODULE,
+	.name = "Virtio I2C Adapter",
+	.class = I2C_CLASS_DEPRECATED,

Why are we using something that is deprecated here ?

Warn users that the adapter doesn't support classes anymore.

So this is the right thing to do? Or this is what we expect from new drivers?
Sorry, I am just new to this stuff and so...

https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-wsa@the-dreams.de/

quoted

quoted

quoted

+struct virtio_i2c_out_hdr {
+	__le16 addr;
+	__le16 padding;
+	__le32 flags;
+};

It might be worth setting __packed for the structures here, even when we have
taken care of padding ourselves, for both the structures..

Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.
I agreed to remove "__packed" .

When Michael commented the structure looked like this:

Actually it can be ignored as the compiler isn't going to add any padding by
itself in this case (since you already took care of it) as the structure will be
aligned to the size of the biggest element here. I generally consider it to be a
good coding-style to make sure we don't add any unwanted stuff in there by
mistake.

Anyway, we can see it in future if this is going to be required or not, if and
when we add new fields here.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization