Hello,

On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:

On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:

quoted

On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:

quoted

On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
<...>

quoted

quoted

quoted

+		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.

I have to admit I don't know that quirks stuff, so it's well possible
that I'm wrong here.
 

quoted

quoted

quoted

quoted

+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.

Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.

Not sure if I missed anything. i2c_transfer() is a synchronize call. If
we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
off clock before it return, which disable any transaction and clear all
pending irq.

There is no synchronization to prevent unbinding the i2c-bus device
while there is a i2c transfer on the wire. i2c_del_adapter only takes
i2c-core.c's &core_lock while i2c_transfer takes &adapter->bus_lock.
If you want to test for it: do something like that:

	while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done

and while this is running do:

	cd /sys/bus/platform/drivers/mt-i2c
	while true; do
		echo 1100d000.i2c > unbind;
		sleep 1;
		echo 1100d000.i2c > bind;
		sleep 1;
	done
 

Your scenario can only happens when one thread is still running in
i2c_transfer/algo->master_xfer and the other thread is trying to remove
the device. If that happened, then every device data access in
mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
can't find any checking for this case, I can't find anything prevent i2c
device removal before pending i2c_transfer complete either. Would you
give me an example?

I just noticed that even "my" driver is affected. If the above recipe
makes your driver barf there is something to fix, if not ... hmm, then
maybe there is more synchronization than I'm aware of or my recipe is
wrong.

At least another driver author believed me:
http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |