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

[Viresh Kumar]

On 05-03-21, 09:46, Jie Deng wrote:
> On 2021/3/4 14:06, Viresh Kumar wrote:
> > 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. )

> > > +		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 :)

> > 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.

> > > +	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 ?

> > 
> > > +
> > > +	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.

> > > +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...

> > > +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.

-- 
viresh