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

[Jie Deng]

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.


> >