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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Mar 2021 16:12:44 +0800
From:   Jie Deng <jie.deng@...el.com>
To:     Jason Wang <jasowang@...hat.com>, linux-i2c@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Cc:     mst@...hat.com, wsa@...nel.org, 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, viresh.kumar@...aro.org,
        stefanha@...hat.com, pbonzini@...hat.com
Subject: Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver

On 2021/3/5 15:23, Jason Wang wrote:

>
>>>> +    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;
>>>> +        goto err_unlock_free;
>>>
>>>
>>> So if the request is finished after the timerout, all the following 
>>> request will fail, is this expected?
>>>
>>>
>> This is an expected behavior. If timeout happens, we don't need to 
>> care about the requests whether
>> really completed by "HW" or not. Just return error and let the i2c 
>> core to decide whether to resend.
>
>
> So you need at least reinit the completion at least?
>

Right. Will fix it. Thank you.


>
>>>> +    }
>>>> +
>>>> +    ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
>>>
>>>
>>> So consider driver queue N requests, can device raise interrupt if 
>>> it completes the first request? If yes, the code break, if not it 
>>> need to be clarified in the spec.
>> The device can raise interrupt when some requests are still not 
>> completed though this is not a good operation.
>
>
> Then you need forbid this in the spec.
>

Yeah, but I think we can add some description to explain this clearly 
instead of forbid it directly.


>
>> In this case, the remaining requests in the vq will be ignored and 
>> the i2c_algorithm. master_xfer will return 1 for
>> your example. And let the i2c core to decide whether to resend.
>>>
>>> Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in 
>>> previous versions, and after reading the spec I still don't get the 
>>> motivation for that (it may complicates both driver and device 
>>> actually).
>>>
>> This flag is introduced by Stefan. Please check following link for 
>> the details
>> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00075.html. 
>>
>
>
> > We just need to make sure that once the driver adds some requests to 
> the
> > virtqueue,
> > it should complete them (either success or fail) before adding new 
> requests.
> > I think this
> > is a behavior of physical I2C adapter drivers and we can follow.
>
>
> Is this a driver requirement or device? If it's the driver, the code 
> have already did something like this. E.g you wait for the completion 
> of the request and forbid new request via i2c_lock.
>
> Thanks
>

The driver.  VIRTIO_I2C_FLAGS_FAIL_NEXT doesn't help in Linux driver. 
But I agree with Stefan that
VIRTIO is not specific to Linux so the specs design should avoid the 
limitations of the current
Linux driver behavior.


>
>>
>