Andy Shevchenko
2021-Jul-02 09:58 UTC
[PATCH v12] i2c: virtio: add a virtio i2c frontend driver
On Fri, Jul 02, 2021 at 04:46:47PM +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. > > The device specification can be found on > https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html. > > By following the specification, people may implement different > backend drivers to emulate different controllers according to > their needs....> +static int virtio_i2c_complete_reqs(struct virtqueue *vq, > + struct virtio_i2c_req *reqs, > + struct i2c_msg *msgs, int nr, > + bool fail) > +{ > + struct virtio_i2c_req *req; > + bool failed = fail; > + unsigned int len; > + int i, j = 0; > + > + for (i = 0; i < nr; i++) { > + /* Detach the ith request from the vq */ > + req = virtqueue_get_buf(vq, &len); > + > + /* > + * Condition (req && req == &reqs[i]) should always meet since > + * we have total nr requests in the vq. > + */ > + if (!failed && (WARN_ON(!(req && req == &reqs[i])) || > + (req->in_hdr.status != VIRTIO_I2C_MSG_OK))) > + failed = true;...and after failed is true, we are continuing the loop, why?> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed); > + if (!failed)> + ++j;Besides better to read j++ the j itself can be renamed to something more verbose.> + }> + return (fail ? -ETIMEDOUT : j);Redundant parentheses.> +}...> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num); > + if (ret != num) { > + virtio_i2c_complete_reqs(vq, reqs, msgs, ret, true);Below you check the returned code, here is not.> + ret = 0; > + goto err_free; > + } > + > + reinit_completion(&vi->completion); > + virtqueue_kick(vq); > + > + time_left = wait_for_completion_timeout(&vi->completion, adap->timeout); > + if (!time_left) > + dev_err(&adap->dev, "virtio i2c backend timeout.\n"); > + > + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, num, !time_left); > + > +err_free: > + kfree(reqs); > + return ret;> +++ b/include/uapi/linux/virtio_i2c.h> +#include <linux/types.h> > + > +/* The bit 0 of the @virtio_i2c_out_hdr. at flags, used to group the requests */ > +#define VIRTIO_I2C_FLAGS_FAIL_NEXT BIT(0)It's _BITUL() or so from linux/const.h. https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/const.h#L28 You may not use internal definitions in UAPI headers. -- With Best Regards, Andy Shevchenko
Viresh Kumar
2021-Jul-05 02:43 UTC
[PATCH v12] i2c: virtio: add a virtio i2c frontend driver
On 02-07-21, 12:58, Andy Shevchenko wrote:> On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote: > > +static int virtio_i2c_complete_reqs(struct virtqueue *vq, > > + struct virtio_i2c_req *reqs, > > + struct i2c_msg *msgs, int nr, > > + bool fail) > > +{ > > + struct virtio_i2c_req *req; > > + bool failed = fail;Jie, you can actually get rid of this variable too. Jut rename fail to failed and everything shall work as you want.> > + unsigned int len; > > + int i, j = 0; > > + > > + for (i = 0; i < nr; i++) { > > + /* Detach the ith request from the vq */ > > + req = virtqueue_get_buf(vq, &len); > > + > > + /* > > + * Condition (req && req == &reqs[i]) should always meet since > > + * we have total nr requests in the vq. > > + */ > > + if (!failed && (WARN_ON(!(req && req == &reqs[i])) || > > + (req->in_hdr.status != VIRTIO_I2C_MSG_OK))) > > + failed = true; > > ...and after failed is true, we are continuing the loop, why?Actually this function can be called with fail set to true. We proceed as we need to call i2c_put_dma_safe_msg_buf() for all buffers we allocated earlier.> > + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed); > > + if (!failed) > > > + ++j; > > Besides better to read j++ the j itself can be renamed to something more > verbose. > > > + } > > > + return (fail ? -ETIMEDOUT : j); > > Redundant parentheses. > > > +}-- viresh
On 2021/7/2 17:58, Andy Shevchenko wrote:> On Fri, Jul 02, 2021 at 04:46:47PM +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. >> >> The device specification can be found on >> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html. >> >> By following the specification, people may implement different >> backend drivers to emulate different controllers according to >> their needs. > ... > >> +static int virtio_i2c_complete_reqs(struct virtqueue *vq, >> + struct virtio_i2c_req *reqs, >> + struct i2c_msg *msgs, int nr, >> + bool fail) >> +{ >> + struct virtio_i2c_req *req; >> + bool failed = fail; >> + unsigned int len; >> + int i, j = 0; >> + >> + for (i = 0; i < nr; i++) { >> + /* Detach the ith request from the vq */ >> + req = virtqueue_get_buf(vq, &len); >> + >> + /* >> + * Condition (req && req == &reqs[i]) should always meet since >> + * we have total nr requests in the vq. >> + */ >> + if (!failed && (WARN_ON(!(req && req == &reqs[i])) || >> + (req->in_hdr.status != VIRTIO_I2C_MSG_OK))) >> + failed = true; > ...and after failed is true, we are continuing the loop, why?Still need to continue to do some cleanup.> >> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed); >> + if (!failed) >> + ++j; > Besides better to read j++ the j itself can be renamed to something more > verbose.Will change it to j++.>> + } >> + return (fail ? -ETIMEDOUT : j); > Redundant parentheses.Will remove the parentheses.> >> +} > ... > >> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num); >> + if (ret != num) { >> + virtio_i2c_complete_reqs(vq, reqs, msgs, ret, true); > Below you check the returned code, here is not.I will do some optimization here.> >> + ret = 0; >> + goto err_free; >> + } >> + >> + reinit_completion(&vi->completion); >> + virtqueue_kick(vq); >> + >> + time_left = wait_for_completion_timeout(&vi->completion, adap->timeout); >> + if (!time_left) >> + dev_err(&adap->dev, "virtio i2c backend timeout.\n"); >> + >> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, num, !time_left); >> + >> +err_free: >> + kfree(reqs); >> + return ret; >> +++ b/include/uapi/linux/virtio_i2c.h >> +#include <linux/types.h> >> + >> +/* The bit 0 of the @virtio_i2c_out_hdr. at flags, used to group the requests */ >> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT BIT(0) > It's _BITUL() or so from linux/const.h. > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/const.h#L28 > You may not use internal definitions in UAPI headers.Let's use this _BITUL() from linux/const.h instead. Thank you !