On 7/6/2010 5:18 PM, Jean Delvare wrote: > On Tue, 06 Jul 2010 16:44:14 +0900, Joonyoung Shim wrote: >> On 6/29/2010 8:11 PM, Jean Delvare wrote: >>> On Tue, 29 Jun 2010 11:13:05 +0900, Joonyoung Shim wrote: >>>> On 6/29/2010 2:55 AM, Dmitry Torokhov wrote: >>>>> Also, please CC Jean Delvare to make sure I2C bits look good. >>>> I add him to CC. >>> I can't comment without seeing the full patch. >>> >> Sorry for late response, you can see the full patch in follow site. >> >> https://patchwork.kernel.org/patch/108363/ > > OK, overall it's OK, but your driver is vulnerable to a race condition > due to the use of i2c_master_send() and i2c_master_recv(). > >> +static int qt602240_read_reg(struct i2c_client *client, u16 reg) >> +{ >> + u8 buf[2]; >> + u8 val; >> + >> + buf[0] = reg & 0xff; >> + buf[1] = (reg >> 8) & 0xff; >> + >> + if (i2c_master_send(client, buf, 2) != 2) { >> + dev_err(&client->dev, "%s: i2c send failed\n", __func__); >> + return -EIO; >> + } >> + >> + if (i2c_master_recv(client, &val, 1) != 1) { >> + dev_err(&client->dev, "%s: i2c recv failed\n", __func__); >> + return -EIO; >> + } >> + >> + return val; >> +} > > As you don't have any locking in place, there is no guarantee that > another I2C access to the device won't happen between i2c_master_send() > which sets the register pointer and i2c_master_recv() which reads the > value back. > > There are 2 ways to fix this. First way is to add locking around all > your device register accesses. Second way (much better IMHO) is to use > i2c_transfer() with 2 messages instead of i2c_master_send() + > i2c_master_recv(). i2c_transfer() is guaranteed to be atomic (as far as > the device register pointer is concerned) by i2c-core. > OK, i think second solution is better too. I will fix it. Thanks. > Same applies to qt602240_read_object_table() and > qt602240_read_message(), and maybe other functions I haven't seen. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
