linux-kernel - Re: [PATCH v12] 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: <20210705063103.4gnrnx6qwheq37lp@vireshk-i7>
Date:   Mon, 5 Jul 2021 12:01:03 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Jie Deng <jie.deng@...el.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-i2c@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, wsa@...nel.org,
        wsa+renesas@...g-engineering.com, mst@...hat.com, arnd@...db.de,
        jasowang@...hat.com, yu1.wang@...el.com, shuo.a.liu@...el.com,
        conghui.chen@...el.com, stefanha@...hat.com
Subject: Re: [PATCH v12] i2c: virtio: add a virtio i2c frontend driver

On 05-07-21, 14:21, Jie Deng wrote:
> 
> On 2021/7/5 10:43, Viresh Kumar wrote:
> > 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.
> 
> 
> Oh, You are not right. I just found we can't remove this variable. The
> "fail" and "failed" have different
> 
> meanings for this function. We need fail to return the result.

Ahh, yes. You are right. Maybe rename fail to timedout, it would make it more
readable, else fail and failed look very similar.
 
> > > > +	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