OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-comment] [PATCH v6] virtio-i2c: add the device specification

On 2021/1/5 2:02, Stefan Hajnoczi wrote:
On Mon, Jan 04, 2021 at 10:31:48AM +0800, Jie Deng wrote:
On 2020/12/31 20:42, Stefan Hajnoczi wrote:
This approach causes a bottleneck when there are multiple I2C devices.
It requires exclusive access to the virtqueue and virtual I2C bus,
slowing down applications that are accessing separate I2C devices.

This is inefficient, especially if the I2C devices are emulated or on
separate physical I2C busses where request concurrent processing is

This also creates starvation/denial-of-service problems in the driver
because an application can submit many requests and needs exclusive
access to the virtqueue until all requests have completed.

I suggest defining a VIRTIO_I2C_FLAGS_CONTINUATION bit in struct
virtio_i2c_out_hdr::flags. When a request fails and this flag is set
then the device fails all later requests until it reaches a request
where the flag is zero. This allows the driver to enqueue a sequence of
requests that fail as a group without waiting for an empty virtqueue.
The flags is designed to be a "driver->device" field for extensibility. It
is read-onlyï to the device.
So when a request fails, the device can't set this read-only field.
Sorry, "and this flag is set" means "and this flag was set by the
driver". My intention was that the driver marks a request so the device
knows that it needs to fail the next request too when this request
No. A device doesn't need to be told by a driver if it needs to fail the
next request.
Actually just the opposite, a driver may need to be notified. A driver can't
know which
one is the "next" because only a device know which request is the first
failed one.
I think we are miscommunicating. The driver is telling the device which
requests are grouped together - that is the purpose of the
VIRTIO_I2C_FLAGS_FAIL_NEXT flag. When a request fails the device is able
to fail the remaining requests in that group. Requests that are not part
of the group won't be failed.

So do you mean to mark the first one of each grouped requests ?

For example, if a driver kicks twice and the virtqueue has following requests.

R11 R12 R21 R22 R23

(First kick: R11 R12, Second kick: R21 R22 R23)

Do you want the driver to set the flags of R11 and R21 to be VIRTIO_I2C_FLAGS_FAIL_NEXT ?

Once a device fails to handle a request, it shall fail the existing
remaining requests
since these requests needs to be handled in order.
Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
name for this flag.

We just need to make sure that once the driver adds some requests to the
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.
Existing VIRTIO devices do not have special empty queue semantics and I
don't think virtio-i2c should introduce a special case here. The
VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
more freedom to enqueue requests, and eliminates the race condition with
polled device implementations (they could complete failed request #1
before request #2 is enqueued and then request #2 would not be
automatically failed).
The I2C core is using a synchronization semantics. It means once a I2C bus
driver send
some messages, it should wait for the device to complete them (either
success or fail)
before sending new messages. The virtio-i2c driver also have to follow this
to register it into
I2C subsystem. So the freedom is already limited by the I2C core, and as a
result, we don't have
such race conditions.
VIRTIO devices are not specific to Linux so the design should avoid the
limitations of the current Linux driver behavior, if possible. It's easy
to include the VIRTIO_I2C_FLAGS_FAIL_NEXT bit that I described, both in
the spec and in the Linux driver, and it will allow flexibility in the


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]