Message-ID: <83529dac-7b87-aec9-55f0-85face47e7b6@redhat.com>
Date:   Mon, 12 Oct 2020 11:43:32 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Jie Deng <jie.deng@...el.com>, linux-i2c@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Cc:     mst@...hat.com, wsa+renesas@...g-engineering.com, wsa@...nel.org,
        andriy.shevchenko@...ux.intel.com, jarkko.nikula@...ux.intel.com,
        jdelvare@...e.de, Sergey.Semin@...kalelectronics.ru,
        krzk@...nel.org, rppt@...nel.org, loic.poulain@...aro.org,
        tali.perry1@...il.com, bjorn.andersson@...aro.org,
        shuo.a.liu@...el.com, conghui.chen@...el.com, yu1.wang@...el.com
Subject: Re: [PATCH v3] i2c: virtio: add a virtio i2c frontend driver


On 2020/10/12 上午10:45, Jie Deng wrote:
>
>
> On 2020/10/10 11:14, Jason Wang wrote:
>>
>>> +
>>> +        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);
>>> +            break;
>>> +        }
>>
>>
>> You don't set error number here. Is this intended?
>>
>> And using a timeout here is not good, and if the request is finished 
>> just after the timeout, in the next xfer you may hit the following 
>> check.
>>
>> It's better to use either interrupt here.
>>
> Could you check the I2C drivers in the kernel ? The 
> "wait_for_completion_timeout" mechanism
> is commonly used by I2C bus drivers in their i2c_algorithm.master_xfer.


There's a major difference between virtio-i2c and other drivers. In the 
case of virtio, the device could be a software device emulated by a 
remote process. This means the timeout might not be rare.

I don't see how timeout is properly handled in this patch (e.g did you 
notice that you don't set any error when timeout? or is this intended?)


>
>>
>>> +
>>> +        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
>>> +        /* vmsg should point to the same address with &vi->vmsg */
>>> +        if ((!vmsg) || (vmsg != &vi->vmsg)) {
>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue 
>>> error.\n",
>>> +                i, msgs[i].addr);
>>> +            break;
>>> +        }
>>
>>
>> So I think we can remove this check. Consider only one descriptor 
>> will be used at most, unless there's a bug in the device (and no 
>> other driver to the similar check), we should not hit this.
>>
>> Btw, as I replied in the previous version, the device should be 
>> cacpable of dealing of a batch of requests through the virtqueue, 
>> otherwise it's meaningless to use a queue here.
>>
> We should not assume there is no bug in the device. I don't think we 
> can remove this check if we want our code to be robust.


Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?



> As I said, currently, we are using the virtqueue to send the msg one 
> by one to the backend. The mechanism is described in the spec. 


Which part of the spec describes such "one by one" mechanism? If there 
is one, I'd happily give a NACK since it doesn't require a queue to work 
which is conflict with the concept of the virtqueue.


> Thanks.
>
>
>>
>>> +
>>>
>>> +
>>> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
>>> +#define _UAPI_LINUX_VIRTIO_I2C_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/virtio_ids.h>
>>> +#include <linux/virtio_config.h>
>>> +
>>> +/**
>>> + * 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 {
>>> +    __le16 addr;
>>> +    __le16 flags;
>>> +    __le16 len;
>>> +};
>>
>>
>> I'm afraid this is not complete. E.g the status is missed.
>>
>> I suspect what virtio-scsi use is better. Which split the in from the 
>> out instead of reusing the same buffer. And it can ease the uAPI 
>> header export.
>>
>> Thanks
>>
>>
>
> I think following definition in uAPI for the status is enough.
> There is no need to provide a "u8" status in the structure.
>
> /* The final status written by the device */
> #define VIRTIO_I2C_MSG_OK    0
> #define VIRTIO_I2C_MSG_ERR    1
>
> You can see an example in virtio_blk.
>
> In the spec:
>
> struct virtio_blk_req {
> le32 type;
> le32 reserved;
> le64 sector;
> u8 data[];
> u8 status;
> };
>
> In virtio_blk.h, there is only following definitions.
>
> #define VIRTIO_BLK_S_OK        0
> #define VIRTIO_BLK_S_IOERR    1
> #define VIRTIO_BLK_S_UNSUPP    2
>

virtio-blk is a bad example, it's just too late to fix. For any new 
introduced uAPI it should be a complete one.

Thanks


> Thanks.
>
>
>

Powered by blists - more mailing lists