linux-kernel - Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

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]

Date:	Wed, 21 Jan 2015 14:30:09 +0800
From:	Yingjoe Chen <yingjoe.chen@...iatek.com>
To:	Eddie Huang <eddie.huang@...iatek.com>,
	<u.kleine-koenig@...gutronix.de>
CC:	Mark Rutland <mark.rutland@....com>,
	Wolfram Sang <wsa@...-dreams.de>,
	Andrew Bresticker <abrestic@...omium.org>,
	<linux-kernel@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
	Lee Jones <lee.jones@...aro.org>,
	Jean Delvare <jdelvare@...e.de>,
	Xudong Chen <xudong.chen@...iatek.com>,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Arnd Bergmann <arnd@...db.de>, <yh.chen@...iatek.com>,
	Wei Yan <sledge.yanwei@...wei.com>,
	Bjorn Andersson <bjorn.andersson@...ymobile.com>,
	Grant Likely <grant.likely@...aro.org>,
	<devicetree@...r.kernel.org>, Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Beniamino Galvani <b.galvani@...il.com>,
	Neelesh Gupta <neelegup@...ux.vnet.ibm.com>,
	Rob Herring <robh+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<srv_heupstream@...iatek.com>,
	Anders Berg <anders.berg@...gotech.com>,
	Jim Cromie <jim.cromie@...il.com>,
	Simon Glass <sjg@...omium.org>,
	Max Schwarz <max.schwarz@...ine.de>,
	Doug Anderson <dianders@...omium.org>,
	Sascha Hauer <kernel@...gutronix.de>,
	Kumar Gala <galak@...eaurora.org>
Subject: Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C
 controller


Hi Uwe,

Thanks for your review,

On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
<...>
> > > +		ret = -EINVAL;
> > > +		goto err_exit;
> > > +	}
> > > +
> > > +	if (msgs->buf == NULL) {
> > > +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > +		ret = -EINVAL;
> > > +		goto err_exit;
> > > +	}
> > > +
> > > +	i2c->addr = msgs->addr;
> > > +	i2c->msg_len = msgs->len;
> > > +	i2c->msg_buf = msgs->buf;
> > > +
> > > +	if (msgs->flags & I2C_M_RD)
> > > +		i2c->op = I2C_MASTER_RD;
> > > +	else
> > > +		i2c->op = I2C_MASTER_WR;
> > > +
> > > +	/* combined two messages into one transaction */
> > > +	if (num > 1) {
> > > +		i2c->msg_aux_len = (msgs + 1)->len;
> > > +		i2c->op = I2C_MASTER_WRRD;
> > > +	}
> > This means "write then read", right? You should check here that the
> > first message is really a write and the 2nd a read then.
> > Can this happen at all with the quirks defined below (.max_num_msgs =
> > 1)?
> Yes, mean write then read. Indeed, add check is better.
> If msg number is 1, means normal write or read, not "write then read".

The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.


<...>
> > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > +{
> > > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > +
> > > +	i2c_del_adapter(&i2c->adap);
> > > +	free_i2c_dma_bufs(i2c);
> > > +	platform_set_drvdata(pdev, NULL);
> > > +
> > Here you need to make sure that no irq is running when i2c_del_adapter
> > is called.
> OK, add check here

I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/