From: Arnd Bergmann <arnd@arndb.de>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Vincent Guittot" <vincent.guittot@linaro.org>,
"Jie Deng" <jie.deng@intel.com>,
"Bill Mills" <bill.mills@linaro.org>,
"QEMU Developers" <qemu-devel@nongnu.org>,
"Arnd Bergmann" <arnd.bergmann@linaro.com>,
"Mike Holmes" <mike.holmes@linaro.org>,
"Alex Bennée" <alex.bennee@linaro.org>,
"Stratos Mailing List" <stratos-dev@op-lists.linaro.org>
Subject: Re: [PATCH 3/5] tools/vhost-user-i2c: Add backend driver
Date: Thu, 25 Mar 2021 17:16:04 +0100 [thread overview]
Message-ID: <CAK8P3a198aR8d0qUPg124O_P7bzO+Yagwe9ydprSGj789ydn2w@mail.gmail.com> (raw)
In-Reply-To: <c269da55e0b3ff984bf538e3001efc4732c6dea7.1616570702.git.viresh.kumar@linaro.org>
On Wed, Mar 24, 2021 at 8:33 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> +static uint8_t vi2c_xfer(VuDev *dev, struct i2c_msg *msg)
> +{
> + VuI2c *i2c = container_of(dev, VuI2c, dev.parent);
> + struct i2c_rdwr_ioctl_data data;
> + VI2cAdapter *adapter;
> +
> + adapter = vi2c_find_adapter(i2c, msg->addr);
> + if (!adapter) {
> + g_printerr("Failed to find adapter for address: %x\n", msg->addr);
> + return VIRTIO_I2C_MSG_ERR;
> + }
> +
> + data.nmsgs = 1;
> + data.msgs = msg;
> +
> + if (ioctl(adapter->fd, I2C_RDWR, &data) < 0) {
> + g_printerr("Failed to transfer data to address %x : %d\n", msg->addr, errno);
> + return VIRTIO_I2C_MSG_ERR;
> + }
As you found during testing, this doesn't work for host kernels
that only implement the SMBUS protocol. Since most i2c clients
only need simple register read/write operations, I think you should
at least handle the common ones (and one two byte read/write)
here to make it more useful.
> +static void vi2c_handle_ctrl(VuDev *dev, int qidx)
> +{
> + VuVirtq *vq = vu_get_queue(dev, qidx);
> + struct i2c_msg msg;
> + struct virtio_i2c_out_hdr *out_hdr;
> + struct virtio_i2c_in_hdr *in_hdr;
> + bool fail_next = false;
> + size_t len, in_hdr_len;
> +
> + for (;;) {
> + VuVirtqElement *elem;
> +
> + elem = vu_queue_pop(dev, vq, sizeof(VuVirtqElement));
> + if (!elem) {
> + break;
> + }
> +
> + g_debug("%s: got queue (in %d, out %d)", __func__, elem->in_num,
> + elem->out_num);
> +
> + /* Validate size of out header */
> + if (elem->out_sg[0].iov_len != sizeof(*out_hdr)) {
> + g_warning("%s: Invalid out hdr %zu : %zu\n", __func__,
> + elem->out_sg[0].iov_len, sizeof(*out_hdr));
> + continue;
> + }
> +
> + out_hdr = elem->out_sg[0].iov_base;
> +
> + /* Bit 0 is reserved in virtio spec */
> + msg.addr = out_hdr->addr >> 1;
> +
> + /* Read Operation */
> + if (elem->out_num == 1 && elem->in_num == 2) {
> + len = elem->in_sg[0].iov_len;
> + if (!len) {
> + g_warning("%s: Read buffer length can't be zero\n", __func__);
> + continue;
> + }
It looks like you are not handling endianness conversion here. As far as I
can tell, the protocol requires little-endian data, but the code might
run on a big-endian CPU.
Jie Deng also pointed out the type differences, but actually handling
them correctly is more important that describing them the right way.
Arnd
next prev parent reply other threads:[~2021-03-25 16:33 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-24 7:33 [PATCH 0/5] virtio: Implement generic vhost-user-i2c backend Viresh Kumar
2021-03-24 7:33 ` [PATCH 1/5] hw/virtio: add boilerplate for vhost-user-i2c device Viresh Kumar
2021-03-29 15:13 ` Alex Bennée
2021-03-24 7:33 ` [PATCH 2/5] hw/virtio: add vhost-user-i2c-pci boilerplate Viresh Kumar
2021-03-29 15:19 ` Alex Bennée
2021-03-24 7:33 ` [PATCH 3/5] tools/vhost-user-i2c: Add backend driver Viresh Kumar
2021-03-25 5:09 ` Jie Deng
2021-03-25 5:22 ` Viresh Kumar
2021-03-25 12:22 ` Alex Bennée
2021-03-30 12:49 ` Alex Bennée
2021-03-25 6:17 ` Jie Deng
2021-03-25 6:36 ` Viresh Kumar
2021-03-25 16:16 ` Arnd Bergmann [this message]
2021-03-26 6:01 ` Viresh Kumar
2021-03-26 8:33 ` Arnd Bergmann
2021-03-26 7:14 ` Viresh Kumar
2021-03-26 8:24 ` Arnd Bergmann
2021-03-24 7:33 ` [PATCH 4/5] docs: add a man page for vhost-user-i2c Viresh Kumar
2021-03-24 7:33 ` [PATCH 5/5] MAINTAINERS: Add entry for virtio-i2c Viresh Kumar
2021-03-24 7:42 ` [PATCH 0/5] virtio: Implement generic vhost-user-i2c backend no-reply
2021-03-24 11:05 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAK8P3a198aR8d0qUPg124O_P7bzO+Yagwe9ydprSGj789ydn2w@mail.gmail.com \
--to=arnd@arndb.de \
--cc=alex.bennee@linaro.org \
--cc=arnd.bergmann@linaro.com \
--cc=bill.mills@linaro.org \
--cc=jie.deng@intel.com \
--cc=mike.holmes@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=stratos-dev@op-lists.linaro.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).