On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> > 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.
> I have to admit I don't know that quirks stuff, so it's well possible
> that I'm wrong 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.
> 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.
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?
Joe.C
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
