Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-02 Thread Viresh Kumar
On 02-07-21, 15:15, Jie Deng wrote:
> Then what is the need to design this interface as "return the number of
> messages successfully
> processed, or a negative value on error". Just return success or fail is
> enough.

Right, that isn't clear to me as well. And so I asked Wolfram this yesterday.

I think it is left for the clients handle this, i.e. what they want to do with
it if something fails in between.

> Here, we didn't break the contract with the interface "master_xfer", so if
> there is a problem then
> the contract may be the problem.

So in your case here, either you should return 0 or nr (the number of transfers
requested) and anything else can only be sent if the host reports partial
failures.

Also, since this driver is pretty much independent of everything else, and won't
break anything in the kernel, there is still a good chance of getting it merged
for 5.14-rc1/2.. So it would be better if you resend the next version as soon as
possible :)

-- 
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-02 Thread Jie Deng



On 2021/7/2 14:56, Viresh Kumar wrote:

On 02-07-21, 14:52, Jie Deng wrote:

This is not efficient. If adding the ith request to the queue fails, we can
still send

the requests before it.

Not really. Normally the requests which are sent together by clients, are linked
together, like a state machine. So if the first one is sent, but not the second
one, then there is not going to be any meaningful result of that.

The i2c core doesn't club requests together from different clients in a single
i2c_transfer() call. So you must assume i2c_transfer(), irrespective of the
number of underlying messages in it, as atomic. If you fail, the client is going
to retry everything again or assume it failed completely.



Then what is the need to design this interface as "return the number of 
messages successfully
processed, or a negative value on error". Just return success or fail is 
enough.


Here, we didn't break the contract with the interface "master_xfer", so 
if there is a problem then

the contract may be the problem.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-02 Thread Viresh Kumar
On 02-07-21, 14:52, Jie Deng wrote:
> This is not efficient. If adding the ith request to the queue fails, we can
> still send
> 
> the requests before it.

Not really. Normally the requests which are sent together by clients, are linked
together, like a state machine. So if the first one is sent, but not the second
one, then there is not going to be any meaningful result of that.

The i2c core doesn't club requests together from different clients in a single
i2c_transfer() call. So you must assume i2c_transfer(), irrespective of the
number of underlying messages in it, as atomic. If you fail, the client is going
to retry everything again or assume it failed completely.

> We don't need to know if it fails here (adding to
> the queue)
> 
> or there (later in the host). The "master_xfer" just need to return final
> number of
> 
> messages successfully processed.

No, that isn't going to help and it is rather inefficient, trying to send
transfer when we already know part of it failed.

-- 
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-02 Thread Jie Deng



On 2021/7/2 12:55, Viresh Kumar wrote:

On 01-07-21, 21:24, Wolfram Sang wrote:

I just noticed this now, but this function even tries to send data
partially, which isn't right. If the caller (i2c device's driver)
calls this for 5 struct i2c_msg instances, then all 5 need to get
through or none.. where as we try to send as many as possible here.

This looks broken to me. Rather return an error value here on success,
or make it complete failure.

Though to be fair I see i2c-core also returns number of messages
processed from i2c_transfer().

Wolfram, what's expected here ? Shouldn't all message transfer or
none?

Well, on a physical bus, it can simply happen that after message 3 of 5,
the bus is stalled, so we need to bail out.

Right, and in that case the transfer will have any meaning left? I believe it
needs to be fully retried as the requests may have been dependent on each other.


Again, I am missing details of a virtqueue, but I'd think it is
different. If adding to the queue fails, then it probably make sense to
drop the whole transfer.

Exactly my point.



This is not efficient. If adding the ith request to the queue fails, we 
can still send


the requests before it. We don't need to know if it fails here (adding 
to the queue)


or there (later in the host). The "master_xfer" just need to return 
final number of


messages successfully processed.



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Viresh Kumar
On 02-07-21, 11:36, Jie Deng wrote:
> OK. Let's add the following two lines to make sure that msg_buf is only
> sent when the msgs len is not zero. And backend judges whether it is
> a zero-length request by checking the number of elements received.
> 
>  + if (msgs[i].len) {
>    reqs[i].buf = i2c_get_dma_safe_msg_buf([i], 1);
>    if (!reqs[i].buf)
>    break;
> 
>   sg_init_one(_buf, reqs[i].buf, msgs[i].len);
> 
>   if (msgs[i].flags & I2C_M_RD)
>   sgs[outcnt + incnt++] = _buf;
>   else
>   sgs[outcnt++] = _buf;
> +}

Perfect. Thanks.

-- 
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Viresh Kumar
On 01-07-21, 21:24, Wolfram Sang wrote:
> 
> > I just noticed this now, but this function even tries to send data
> > partially, which isn't right. If the caller (i2c device's driver)
> > calls this for 5 struct i2c_msg instances, then all 5 need to get
> > through or none.. where as we try to send as many as possible here.
> > 
> > This looks broken to me. Rather return an error value here on success,
> > or make it complete failure.
> > 
> > Though to be fair I see i2c-core also returns number of messages
> > processed from i2c_transfer().
> > 
> > Wolfram, what's expected here ? Shouldn't all message transfer or
> > none?
> 
> Well, on a physical bus, it can simply happen that after message 3 of 5,
> the bus is stalled, so we need to bail out.

Right, and in that case the transfer will have any meaning left? I believe it
needs to be fully retried as the requests may have been dependent on each other.

> Again, I am missing details of a virtqueue, but I'd think it is
> different. If adding to the queue fails, then it probably make sense to
> drop the whole transfer.

Exactly my point.

> Of course, it can later happen on the physical bus of the host, though,
> that the bus is stalled after message 3 of 5, and I2C_RDWR will bail
> out.

Basically we fail as soon as we know something is not right, correct?

-- 
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Jie Deng


On 2021/7/1 14:18, Viresh Kumar wrote:

On 01-07-21, 14:10, Jie Deng wrote:

I think a fixed number of sgs will make things easier to develop backend.

Yeah, but it looks awkward to send a message buffer which isn't used
at all. From protocol's point of view, it just looks wrong/buggy.

The backend can just look at the number of elements received, they
can either be 2 (in case of zero-length) transfer, or 3 (for
read/write) and any other number is invalid.



OK. Let's add the following two lines to make sure that msg_buf is only
sent when the msgs len is not zero. And backend judges whether it is
a zero-length request by checking the number of elements received.

 + if (msgs[i].len) {
   reqs[i].buf = i2c_get_dma_safe_msg_buf([i], 1);
   if (!reqs[i].buf)
   break;

  sg_init_one(_buf, reqs[i].buf, msgs[i].len);

  if (msgs[i].flags & I2C_M_RD)
  sgs[outcnt + incnt++] = _buf;
  else
  sgs[outcnt++] = _buf;
+}


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Jie Deng



On 2021/7/1 18:00, kernel test robot wrote:

Hi Jie,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linux/master linus/master v5.13 next-20210630]
[cannot apply to vhost/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210701-112619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
i2c/for-next
config: i386-randconfig-c021-20210630 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
 # 
https://github.com/0day-ci/linux/commit/e8dedd2a8577148d7655d0affe35adf34efbbf15
 git remote add linux-review https://github.com/0day-ci/linux
 git fetch --no-tags linux-review 
Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210701-112619
 git checkout e8dedd2a8577148d7655d0affe35adf34efbbf15
 # save the attached .config to linux build tree
 mkdir build_dir
 make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All errors (new ones prefixed by >>):

In file included from :32:

./usr/include/linux/virtio_i2c.h:12:10: fatal error: linux/bits.h: No such file 
or directory

   12 | #include 
  |  ^~
compilation terminated.



I didn't see this error. Why did you say no such file? Anything wrong ?

https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/tree/include/linux/bits.h

Thank you !


---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Jie Deng



On 2021/7/1 18:45, Andy Shevchenko wrote:

On Thu, Jul 01, 2021 at 11:24:46AM +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/msg8.html.

By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
- Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".

Why is that?



Please refer to https://lkml.org/lkml/2021/3/23/285.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Andy Shevchenko
On Thu, Jul 01, 2021 at 11:24:46AM +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/msg8.html.
> 
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.

>   - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".

Why is that?

-- 
With Best Regards,
Andy Shevchenko


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread kernel test robot
Hi Jie,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linux/master linus/master v5.13 next-20210630]
[cannot apply to vhost/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210701-112619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
i2c/for-next
config: i386-randconfig-c021-20210630 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# 
https://github.com/0day-ci/linux/commit/e8dedd2a8577148d7655d0affe35adf34efbbf15
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210701-112619
git checkout e8dedd2a8577148d7655d0affe35adf34efbbf15
# save the attached .config to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All errors (new ones prefixed by >>):

   In file included from :32:
>> ./usr/include/linux/virtio_i2c.h:12:10: fatal error: linux/bits.h: No such 
>> file or directory
  12 | #include 
 |  ^~
   compilation terminated.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread kernel test robot
Hi Jie,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linux/master linus/master v5.13 next-20210630]
[cannot apply to vhost/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210701-112619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
i2c/for-next
config: x86_64-randconfig-r012-20210630 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 
e7e71e9454ed76c1b3d8140170b5333c28bef1be)
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# 
https://github.com/0day-ci/linux/commit/e8dedd2a8577148d7655d0affe35adf34efbbf15
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210701-112619
git checkout e8dedd2a8577148d7655d0affe35adf34efbbf15
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir 
ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All errors (new ones prefixed by >>):

   In file included from :1:
>> ./usr/include/linux/virtio_i2c.h:12:10: fatal error: 'linux/bits.h' file not 
>> found
   #include 
^~
   1 error generated.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Viresh Kumar
On 01-07-21, 14:10, Jie Deng wrote:
> I think a fixed number of sgs will make things easier to develop backend.

Yeah, but it looks awkward to send a message buffer which isn't used
at all. From protocol's point of view, it just looks wrong/buggy.

The backend can just look at the number of elements received, they
can either be 2 (in case of zero-length) transfer, or 3 (for
read/write) and any other number is invalid.

> If you prefer to parse the number of descriptors instead of using the msg
> length to
> 
> distinguish the zero-length request from other requests, I'm OK to set a
> limit.

My concern is more about the specification here first.

> if (!msgs[i].len) {
>     sg_init_one(_buf, reqs[i].buf, msgs[i].len);
> 
>     if (msgs[i].flags & I2C_M_RD)
>         sgs[outcnt + incnt++] = _buf;
>     else
>         sgs[outcnt++] = _buf;
> }

> > > +#ifdef CONFIG_PM_SLEEP
> > You could avoid this pair of ifdef by creating dummy versions of below
> > routines for !CONFIG_PM_SLEEP case. Up to you.
> 
> 
> Thank you. I'd like to keep the same.

Sure.

-- 
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-07-01 Thread Jie Deng


On 2021/7/1 12:04, Viresh Kumar wrote:

On 01-07-21, 11:24, Jie Deng wrote:

Changes v10 -> v11
- Remove vi->adap.class = I2C_CLASS_DEPRECATED.
- Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
- Remove "struct mutex lock" in "struct virtio_i2c".
- Support zero-length request.
- Remove unnecessary logs.
- Remove vi->adap.timeout = HZ / 10, just use the default value.
- Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
- Add the virtio_device index to adapter's naming mechanism.

Thanks Jie.

I hope you are going to send a fix for specification as well (for the
zero-length request) ?



Yes. I will send that fix once this patch get merged.





diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+   struct virtio_i2c_req *reqs,
+   struct i2c_msg *msgs, int nr)
+{
+   struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+   int i, outcnt, incnt, err = 0;
+
+   for (i = 0; i < nr; i++) {
+   /*
+* Only 7-bit mode supported for this moment. For the address 
format,
+* Please check the Virtio I2C Specification.
+*/
+   reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+   if (i != nr - 1)
+   reqs[i].out_hdr.flags = 
cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
+
+   outcnt = incnt = 0;
+   sg_init_one(_hdr, [i].out_hdr, 
sizeof(reqs[i].out_hdr));
+   sgs[outcnt++] = _hdr;
+
+   reqs[i].buf = i2c_get_dma_safe_msg_buf([i], 1);
+   if (!reqs[i].buf)
+   break;
+
+   sg_init_one(_buf, reqs[i].buf, msgs[i].len);

The len can be zero here for zero-length transfers.


+
+   if (msgs[i].flags & I2C_M_RD)
+   sgs[outcnt + incnt++] = _buf;
+   else
+   sgs[outcnt++] = _buf;
+
+   sg_init_one(_hdr, [i].in_hdr, sizeof(reqs[i].in_hdr));
+   sgs[outcnt + incnt++] = _hdr;

Why are we still sending the msg_buf if the length is 0? Sending the
buffer makes sense if you have some data to send, but otherwise it is
just an extra sg element, which isn't required to be sent.



I think a fixed number of sgs will make things easier to develop backend.

If you prefer to parse the number of descriptors instead of using the 
msg length to


distinguish the zero-length request from other requests, I'm OK to set a 
limit.


if (!msgs[i].len) {
    sg_init_one(_buf, reqs[i].buf, msgs[i].len);

    if (msgs[i].flags & I2C_M_RD)
        sgs[outcnt + incnt++] = _buf;
    else
        sgs[outcnt++] = _buf;
}






+#ifdef CONFIG_PM_SLEEP
+static int virtio_i2c_freeze(struct virtio_device *vdev)
+{
+   virtio_i2c_del_vqs(vdev);
+   return 0;
+}
+
+static int virtio_i2c_restore(struct virtio_device *vdev)
+{
+   return virtio_i2c_setup_vqs(vdev->priv);
+}
+#endif
+
+static struct virtio_driver virtio_i2c_driver = {
+   .id_table   = id_table,
+   .probe  = virtio_i2c_probe,
+   .remove = virtio_i2c_remove,
+   .driver = {
+   .name   = "i2c_virtio",
+   },
+#ifdef CONFIG_PM_SLEEP

You could avoid this pair of ifdef by creating dummy versions of below
routines for !CONFIG_PM_SLEEP case. Up to you.



Thank you. I'd like to keep the same.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver

2021-06-30 Thread Viresh Kumar
On 01-07-21, 11:24, Jie Deng wrote:
> Changes v10 -> v11
>   - Remove vi->adap.class = I2C_CLASS_DEPRECATED.
>   - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
>   - Remove "struct mutex lock" in "struct virtio_i2c".
>   - Support zero-length request.
>   - Remove unnecessary logs.
>   - Remove vi->adap.timeout = HZ / 10, just use the default value.
>   - Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
>   - Add the virtio_device index to adapter's naming mechanism.

Thanks Jie.

I hope you are going to send a fix for specification as well (for the
zero-length request) ?

> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> +
> + for (i = 0; i < nr; i++) {
> + /*
> +  * Only 7-bit mode supported for this moment. For the address 
> format,
> +  * Please check the Virtio I2C Specification.
> +  */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != nr - 1)
> + reqs[i].out_hdr.flags = 
> cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + outcnt = incnt = 0;
> + sg_init_one(_hdr, [i].out_hdr, 
> sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = _hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf([i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(_buf, reqs[i].buf, msgs[i].len);

The len can be zero here for zero-length transfers.

> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = _buf;
> + else
> + sgs[outcnt++] = _buf;
> +
> + sg_init_one(_hdr, [i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = _hdr;

Why are we still sending the msg_buf if the length is 0? Sending the
buffer makes sense if you have some data to send, but otherwise it is
just an extra sg element, which isn't required to be sent.

> +
> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i], 
> GFP_KERNEL);
> + if (err < 0) {
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], false);
> + break;
> + }
> + }
> +
> + return i;

I just noticed this now, but this function even tries to send data
partially, which isn't right. If the caller (i2c device's driver)
calls this for 5 struct i2c_msg instances, then all 5 need to get
through or none.. where as we try to send as many as possible here.

This looks broken to me. Rather return an error value here on success,
or make it complete failure.

Though to be fair I see i2c-core also returns number of messages
processed from i2c_transfer().

Wolfram, what's expected here ? Shouldn't all message transfer or
none?

> +#ifdef CONFIG_PM_SLEEP
> +static int virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> + virtio_i2c_del_vqs(vdev);
> + return 0;
> +}
> +
> +static int virtio_i2c_restore(struct virtio_device *vdev)
> +{
> + return virtio_i2c_setup_vqs(vdev->priv);
> +}
> +#endif
> +
> +static struct virtio_driver virtio_i2c_driver = {
> + .id_table   = id_table,
> + .probe  = virtio_i2c_probe,
> + .remove = virtio_i2c_remove,
> + .driver = {
> + .name   = "i2c_virtio",
> + },
> +#ifdef CONFIG_PM_SLEEP

You could avoid this pair of ifdef by creating dummy versions of below
routines for !CONFIG_PM_SLEEP case. Up to you.

> + .freeze = virtio_i2c_freeze,
> + .restore = virtio_i2c_restore,
> +#endif
> +};

-- 
viresh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization