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
