On 03-11-21, 06:18, Chen, Conghui wrote:> >>> Over the long term, I think the backend should provide that timeout > >>> value and guarantee that its processing time should not exceed that > >>> value. > >> If you mean that the spec should be changed to allow the virtio driver > >> to be able to program a certain timeout for I2C transactions in the > >> virtio device, yes, that does sound reasonable. > > > > > >Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui. > > > >She may work on this in the future. > > > > I'll try to update the spec first.I don't think the spec should be changed for timeout. Timeout-interval here isn't the property of just the host firmware/kernel, but the entire setup plays a role here. Host have its own timeframe to take care of things (I think HZ should really be enough for that, since kernel can manage it for busses normally with just that). Then comes the virtualization, context switches, guest OS, backend, etc, which add to this delay. All this is not part of the virtio protocol and so shouldn't be made part of it. -- viresh
Vincent Whitchurch
2021-Nov-03 14:42 UTC
[PATCH 1/2] i2c: virtio: disable timeout handling
On Wed, Nov 03, 2021 at 07:37:45AM +0100, Viresh Kumar wrote:> On 03-11-21, 06:18, Chen, Conghui wrote: > > >>> Over the long term, I think the backend should provide that timeout > > >>> value and guarantee that its processing time should not exceed that > > >>> value. > > >> If you mean that the spec should be changed to allow the virtio driver > > >> to be able to program a certain timeout for I2C transactions in the > > >> virtio device, yes, that does sound reasonable. > > > > > > > > >Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui. > > > > > >She may work on this in the future. > > > > > > > I'll try to update the spec first. > > I don't think the spec should be changed for timeout. Timeout-interval > here isn't the property of just the host firmware/kernel, but the > entire setup plays a role here. > > Host have its own timeframe to take care of things (I think HZ should > really be enough for that, since kernel can manage it for busses > normally with just that). Then comes the virtualization, context > switches, guest OS, backend, etc, which add to this delay. All this is > not part of the virtio protocol and so shouldn't be made part of it.The suggested timeout is not meant to take into account the overhead of virtualization, but to be used by the virtio device as a timeout for the transaction on the I2C bus (presumably by programming this value to the physical I2C controller, if one exists). Assume that userspace (or an I2C client driver) asks for a timeout of 20 ms for a particular transfer because it, say, knows that the particular connected I2C peripheral either responds within 10 ms to a particular register read or never responds, so it doesn't want to waste time waiting unnecessarily long for the transfer to complete. If the virtio device end does not have any information on what timeout is required (as in the current spec), it must assume some high value which will never cause I2C transactions to spuriously timeout, say 10 seconds. Even if the virtio driver is fixed to copy and hold all buffers to avoid memory corruption and to time out and return to the caller after the requested 20 ms, the next I2C transfer can not be issued until 10 seconds have passed, since the virtio device end will still be waiting for the hardcoded 10 second timeout and may not respond to new requests until that transfer has timed out.