Viresh Kumar
2021-Jul-05 12:18 UTC
[PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 29-06-21, 12:43, Wolfram Sang wrote:> > > From the spec: > > > > The case when ``length of \field{write_buf}''=0, and at the same time, > > ``length of \field{read_buf}''=0 doesn't make any sense. > > > > I mentioned this in my first reply and to my understanding I did not get > > a reply that this has changed meanwhile. > > > > Also, this code as mentioned before: > > > + if (!msgs[i].len) > > + break; > > I hope this can extended in the future to allow zero-length messages. If > this is impossible we need to set an adapter quirk instead.Wolfram, I stumbled again upon this while working at the backend implementation. If you look at i2c_smbus_xfer_emulated(), the command is always sent via msgbuf0[0]. Even in the case of I2C_SMBUS_QUICK, where we set msg[0].len = 0, we still send the buf. This is really confusing :( Do I understand correctly that we always need to send msg[0].buf even when msg[0].len is 0 ? If so, it would be difficult to implement this with the current i2c virtio specification, as the msg.len isn't really passed from guest to host, rather it is inferred using the length of the buffer itself. And so we can't really pass a buffer if length is 0. Moreover, the driver uses i2c_get_dma_safe_msg_buf(), which also depends on the length parameter here to allocate the buffer and copy data to it. All in all, the latest version of the driver doesn't work with "i2cdetect -q <bus>". To make it work, I had to add this: diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c index 731267d42292..5b8bd98ae38e 100644 --- a/drivers/i2c/busses/i2c-virtio.c +++ b/drivers/i2c/busses/i2c-virtio.c @@ -73,6 +73,9 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq, sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr)); sgs[outcnt++] = &out_hdr; + if (!msgs[i].len) + msgs[i].len = 1; + if (msgs[i].len) { reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); if (!reqs[i].buf) which made it I2C_SMBUS_BYTE instead of I2C_SMBUS_QUICK. What should we do here Wolfram? Jie, while wolfram comes back and replies to this, I think you need to switch back to NOT supporting zero length transfer and set update virtio_i2c_func() to return: I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); Support for zero-length transfers and I2C_FUNC_SMBUS_QUICK can be added separately. Thanks. -- viresh
On 2021/7/5 20:18, Viresh Kumar wrote:> On 29-06-21, 12:43, Wolfram Sang wrote: >>> From the spec: >>> >>> The case when ``length of \field{write_buf}''=0, and at the same time, >>> ``length of \field{read_buf}''=0 doesn't make any sense. >>> >>> I mentioned this in my first reply and to my understanding I did not get >>> a reply that this has changed meanwhile. >>> >> Also, this code as mentioned before: >> >>> + if (!msgs[i].len) >>> + break; >> I hope this can extended in the future to allow zero-length messages. If >> this is impossible we need to set an adapter quirk instead. > Wolfram, > > I stumbled again upon this while working at the backend implementation. > > If you look at i2c_smbus_xfer_emulated(), the command is always sent via > msgbuf0[0]. Even in the case of I2C_SMBUS_QUICK, where we set msg[0].len = 0, we > still send the buf. This is really confusing :( > > Do I understand correctly that we always need to send msg[0].buf even when > msg[0].len is 0 ? > > If so, it would be difficult to implement this with the current i2c virtio > specification, as the msg.len isn't really passed from guest to host, rather it > is inferred using the length of the buffer itself. And so we can't really pass a > buffer if length is 0. > > Moreover, the driver uses i2c_get_dma_safe_msg_buf(), which also depends on the > length parameter here to allocate the buffer and copy data to it. > > All in all, the latest version of the driver doesn't work with "i2cdetect -q <bus>". > > To make it work, I had to add this: > > diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c > index 731267d42292..5b8bd98ae38e 100644 > --- a/drivers/i2c/busses/i2c-virtio.c > +++ b/drivers/i2c/busses/i2c-virtio.c > @@ -73,6 +73,9 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq, > sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr)); > sgs[outcnt++] = &out_hdr; > > + if (!msgs[i].len) > + msgs[i].len = 1; > + > if (msgs[i].len) { > reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); > if (!reqs[i].buf) > > which made it I2C_SMBUS_BYTE instead of I2C_SMBUS_QUICK. > > What should we do here Wolfram? > > > Jie, while wolfram comes back and replies to this, I think you need to switch > back to NOT supporting zero length transfer and set update virtio_i2c_func() to > return: > > I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); > > Support for zero-length transfers and I2C_FUNC_SMBUS_QUICK can be added > separately. > > Thanks.It's OK to me. Let's see what Wolfram says when he comes back. I will send the updated version then. Thanks.