linux-kernel - Re: [PATCH v9] i2c: virtio: add a virtio i2c frontend driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dbb5dfe9-8ee6-e3f8-3681-d0ec83282930@intel.com>
Date:   Mon, 22 Mar 2021 15:53:36 +0800
From:   Jie Deng <jie.deng@...el.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     linux-i2c@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, mst@...hat.com, wsa@...nel.org,
        jasowang@...hat.com, wsa+renesas@...g-engineering.com,
        andriy.shevchenko@...ux.intel.com, conghui.chen@...el.com,
        arnd@...db.de, kblaiech@...lanox.com,
        jarkko.nikula@...ux.intel.com, Sergey.Semin@...kalelectronics.ru,
        rppt@...nel.org, loic.poulain@...aro.org, tali.perry1@...il.com,
        u.kleine-koenig@...gutronix.de, bjorn.andersson@...aro.org,
        yu1.wang@...el.com, shuo.a.liu@...el.com, stefanha@...hat.com,
        pbonzini@...hat.com
Subject: Re: [PATCH v9] i2c: virtio: add a virtio i2c frontend driver


On 2021/3/22 14:41, Viresh Kumar wrote:
>
>> +/**
>> + * struct virtio_i2c - virtio I2C data
>> + * @vdev: virtio device for this controller
>> + * @completion: completion of virtio I2C message
>> + * @adap: I2C adapter for this controller
>> + * @i2c_lock: lock for virtqueue processing
> Name mismatch here.


Will fix this typo. Thank you.


>> + * @vq: the virtio virtqueue for communication
>> + */
>> +struct virtio_i2c {
>> +	struct virtio_device *vdev;
>> +	struct completion completion;
>> +	struct i2c_adapter adap;
>> +	struct mutex lock;
>> +	struct virtqueue *vq;
>> +};
>
>> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
>> +					struct virtio_i2c_req *reqs,
>> +					struct i2c_msg *msgs, int nr,
>> +					bool timeout)
>> +{
>> +	struct virtio_i2c_req *req;
>> +	bool err_found = false;
>> +	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);
>> +
>> +		if (timeout || err_found)  {
>> +			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
>> +			continue;
>> +		}
>> +
>> +		/*
>> +		 * Condition (req && req == &reqs[i]) should always meet since
>> +		 * we have total nr requests in the vq.
>> +		 */
>> +		if (WARN_ON(!(req && req == &reqs[i])) ||
>> +		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)) {
>> +			err_found = true;
>> +			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
>> +			continue;
>> +		}
>> +
>> +		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], true);
>> +		++j;
>> +	}
> I think you can simplify the code like this here:


I think your optimization has problems...


> 	bool err_found = timeout;
>
> 	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 (!err_found &&
>                      (WARN_ON(!(req && req == &reqs[i])) ||
> 		     (req->in_hdr.status != VIRTIO_I2C_MSG_OK))) {
> 			err_found = true;
> 			continue;


Just continue here, the ith buf leaks ?


> 		}
>
> 		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], err_found);


i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !err_found); ?


>                  if (!err_found)
>                          ++j;
>
>> +
>> +	return (timeout ? -ETIMEDOUT : j);
>> +}
>> +
>> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> +	struct virtio_i2c *vi = i2c_get_adapdata(adap);
>> +	struct virtqueue *vq = vi->vq;
>> +	struct virtio_i2c_req *reqs;
>> +	unsigned long time_left;
>> +	int ret, nr;
>> +
>> +	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
>> +	if (!reqs)
>> +		return -ENOMEM;
>> +
>> +	mutex_lock(&vi->lock);
>> +
>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>> +	if (ret == 0)
>> +		goto err_unlock_free;
>> +
>> +	nr = ret;
>> +	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, nr, true);
>> +		goto err_unlock_free;
>> +	}
>> +
>> +	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, false);
> And this can be optimized as well:
>
> 	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, nr, !time_left);


Good optimization here.