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

[Jie Deng]

On 2021/3/5 11:09, Viresh Kumar wrote:
> 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.


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


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


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


> > > > +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@xxxxxxxxxxxxx/


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