Date:   Fri, 5 Mar 2021 15:00:51 +0800
From:   Jie Deng <jie.deng@...el.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     linux-i2c@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, mst@...hat.com, wsa@...nel.org,
        jasowang@...hat.com, wsa+renesas@...g-engineering.com,
        andriy.shevchenko@...ux.intel.com, conghui.chen@...el.com,
        arnd@...db.de, kblaiech@...lanox.com,
        jarkko.nikula@...ux.intel.com, Sergey.Semin@...kalelectronics.ru,
        rppt@...nel.org, loic.poulain@...aro.org, tali.perry1@...il.com,
        u.kleine-koenig@...gutronix.de, bjorn.andersson@...aro.org,
        yu1.wang@...el.com, shuo.a.liu@...el.com, stefanha@...hat.com,
        pbonzini@...hat.com,
        Alex Bennée <alex.bennee@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: 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:
>> 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@the-dreams.de/


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

Powered by blists - more mailing lists