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

[Jie Deng <jie.deng@intel.com>]

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.


>
>>
>