On Thu, Sep 03, 2020 at 01:34:45PM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> This driver communicates with the backend driver through a
> virtio I2C message structure which includes following parts:
>
> - Header: i2c_msg addr, flags, len.
> - Data buffer: the pointer to the i2c msg data.
> - Status: the processing result from the backend.
>
> People may implement different backend drivers to emulate
> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.
>
> The virtio device ID 34 is used for this I2C adpter since IDs
> before 34 have been reserved by other virtio devices.
Seems it's slightly different version to what I have reviewed internally.
My comments below. (I admit that some of them maybe new)
...
> +/**
> + * struct virtio_i2c_hdr - the virtio I2C message header structure
> + * @addr: i2c_msg addr, the slave address
> + * @flags: i2c_msg flags
> + * @len: i2c_msg len
> + */
> +struct virtio_i2c_hdr {
> + __virtio16 addr;
> + __virtio16 flags;
> + __virtio16 len;
> +} __packed;
As Misha noticed and somewhere I saw 0-day reports these should be carefully
taken care of.
...
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtio_i2c_msg *vmsg_o, *vmsg_i;
> + struct virtqueue *vq = vi->vq;
> + unsigned long time_left;
> + int len, i, ret = 0;
> +
> + vmsg_o = kzalloc(sizeof(*vmsg_o), GFP_KERNEL);
> + if (!vmsg_o)
> + return -ENOMEM;
> +
> + mutex_lock(&vi->i2c_lock);
> + vmsg_o->buf = NULL;
> + for (i = 0; i < num; i++) {
> + ret = virtio_i2c_add_msg(vq, vmsg_o, &msgs[i]);
> + if (ret) {
> + dev_err(&adap->dev, "failed to add msg[%d] to
> virtqueue.\n", i);
> + goto err_unlock_free;
break;
> + }
> +
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(&vi->completion,
> adap->timeout);
> + if (!time_left) {
> + dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", i,
> msgs[i].addr);
> + ret = i;
> + goto err_unlock_free;
break;
And so on.
> + }
> +
> + vmsg_i = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
> + if (vmsg_i) {
> + /* vmsg_i should point to the same address with vmsg_o
> */
> + if (vmsg_i != vmsg_o) {
> + dev_err(&adap->dev, "msg[%d]: addr=0x%x
> virtqueue error.\n",
> + i, vmsg_i->hdr.addr);
> + ret = i;
> + goto err_unlock_free;
> + }
> + if (vmsg_i->status != VIRTIO_I2C_MSG_OK) {
> + dev_err(&adap->dev, "msg[%d]: addr=0x%x
> error=%d.\n",
> + i, vmsg_i->hdr.addr, vmsg_i->status);
> + ret = i;
> + goto err_unlock_free;
> + }
> + if ((vmsg_i->hdr.flags & I2C_M_RD) && vmsg_i->hdr.len)
> + memcpy(msgs[i].buf, vmsg_i->buf,
> vmsg_i->hdr.len);
> +
> + kfree(vmsg_i->buf);
> + vmsg_i->buf = NULL;
> + }
> + reinit_completion(&vi->completion);
> + }
> + if (i == num)
> + ret = num;
And this conditional seems a dup of the for-loop successfully iterating over
entire queue.
> +err_unlock_free:
Redundant.
> + mutex_unlock(&vi->i2c_lock);
> + kfree(vmsg_o->buf);
> + kfree(vmsg_o);
> + return ret;
> +}
...
> + vi->adap.timeout = HZ / 10;
+ Blank line.
> + ret = i2c_add_adapter(&vi->adap);
> + if (ret) {
> + dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
> + virtio_i2c_del_vqs(vdev);
Usually we do clean up followed by message.
> + }
> +
> + return ret;
--
With Best Regards,
Andy Shevchenko
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
