| Message ID | 679ddcf8-b03d-15eb-e914-bdbaa3f5b890@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: fix potential race in i801_block_transaction_byte_by_byte | expand |
Hi Heiner, On Tue, 29 Aug 2023 08:25:23 +0200, Heiner Kallweit wrote: > Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > receiving the last byte. If we get e.g. preempted before setting > SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > before SMBHSTCNT_LAST_BYTE is set. > Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > is also consistent with what we do in i801_isr_byte_done(). > > Fixes: efa3cb15ad8b ("i2c-i801: Refactor use of LAST_BYTE in i801_block_transaction_byte_by_byte") I don't think this is true. This patch refactored the code but didn't change the logic. The bug existed before already. As far as I see, the race condition already existed when the kernel switched to git, so there's no point in having a Fixes statement. > Reported-by: Jean Delvare <jdelvare@suse.com> > Cc: stable@vger.kernel.org > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 7a0ccc584..8acf09539 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -679,15 +679,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, > return result ? priv->status : -ETIMEDOUT; > } > > - for (i = 1; i <= len; i++) { > - if (i == len && read_write == I2C_SMBUS_READ) > - smbcmd |= SMBHSTCNT_LAST_BYTE; > - outb_p(smbcmd, SMBHSTCNT(priv)); > - > - if (i == 1) > - outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START, > - SMBHSTCNT(priv)); > + if (len == 1 && read_write == I2C_SMBUS_READ) > + smbcmd |= SMBHSTCNT_LAST_BYTE; > + outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv)); > > + for (i = 1; i <= len; i++) { > status = i801_wait_byte_done(priv); > if (status) > return status; > @@ -710,9 +706,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, > data->block[0] = len; > } > > - /* Retrieve/store value in SMBBLKDAT */ > - if (read_write == I2C_SMBUS_READ) > + if (read_write == I2C_SMBUS_READ) { > data->block[i] = inb_p(SMBBLKDAT(priv)); > + if (i == len - 1) > + outb_p(smbcmd | SMBHSTCNT_LAST_BYTE, SMBHSTCNT(priv)); > + } > + > if (read_write == I2C_SMBUS_WRITE && i+1 <= len) > outb_p(data->block[i+1], SMBBLKDAT(priv)); > Looks good and tested OK. Reviewed-by: Jean Delvare <jdelvare@suse.de>
On 01.09.2023 18:44, Jean Delvare wrote: > Hi Heiner, > > On Tue, 29 Aug 2023 08:25:23 +0200, Heiner Kallweit wrote: >> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started >> receiving the last byte. If we get e.g. preempted before setting >> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte >> before SMBHSTCNT_LAST_BYTE is set. >> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing >> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code >> is also consistent with what we do in i801_isr_byte_done(). >> >> Fixes: efa3cb15ad8b ("i2c-i801: Refactor use of LAST_BYTE in i801_block_transaction_byte_by_byte") > > I don't think this is true. This patch refactored the code but didn't > change the logic. The bug existed before already. As far as I see, the > race condition already existed when the kernel switched to git, so > there's no point in having a Fixes statement. > Shall we go with the patch as-is or do you think it's better to resubmit w/o the Fixes tag? >> Reported-by: Jean Delvare <jdelvare@suse.com> >> Cc: stable@vger.kernel.org >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index 7a0ccc584..8acf09539 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -679,15 +679,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, >> return result ? priv->status : -ETIMEDOUT; >> } >> >> - for (i = 1; i <= len; i++) { >> - if (i == len && read_write == I2C_SMBUS_READ) >> - smbcmd |= SMBHSTCNT_LAST_BYTE; >> - outb_p(smbcmd, SMBHSTCNT(priv)); >> - >> - if (i == 1) >> - outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START, >> - SMBHSTCNT(priv)); >> + if (len == 1 && read_write == I2C_SMBUS_READ) >> + smbcmd |= SMBHSTCNT_LAST_BYTE; >> + outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv)); >> >> + for (i = 1; i <= len; i++) { >> status = i801_wait_byte_done(priv); >> if (status) >> return status; >> @@ -710,9 +706,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, >> data->block[0] = len; >> } >> >> - /* Retrieve/store value in SMBBLKDAT */ >> - if (read_write == I2C_SMBUS_READ) >> + if (read_write == I2C_SMBUS_READ) { >> data->block[i] = inb_p(SMBBLKDAT(priv)); >> + if (i == len - 1) >> + outb_p(smbcmd | SMBHSTCNT_LAST_BYTE, SMBHSTCNT(priv)); >> + } >> + >> if (read_write == I2C_SMBUS_WRITE && i+1 <= len) >> outb_p(data->block[i+1], SMBBLKDAT(priv)); >> > > Looks good and tested OK. > > Reviewed-by: Jean Delvare <jdelvare@suse.de> >
On Fri, 1 Sep 2023 19:02:13 +0200, Heiner Kallweit wrote: > On 01.09.2023 18:44, Jean Delvare wrote: > > Hi Heiner, > > > > On Tue, 29 Aug 2023 08:25:23 +0200, Heiner Kallweit wrote: > >> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > >> receiving the last byte. If we get e.g. preempted before setting > >> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > >> before SMBHSTCNT_LAST_BYTE is set. > >> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > >> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > >> is also consistent with what we do in i801_isr_byte_done(). > >> > >> Fixes: efa3cb15ad8b ("i2c-i801: Refactor use of LAST_BYTE in i801_block_transaction_byte_by_byte") > > > > I don't think this is true. This patch refactored the code but didn't > > change the logic. The bug existed before already. As far as I see, the > > race condition already existed when the kernel switched to git, so > > there's no point in having a Fixes statement. > > Shall we go with the patch as-is or do you think it's better to resubmit > w/o the Fixes tag? Please resubmit, for the sake of correctness and to be fair to Daniel.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 7a0ccc584..8acf09539 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -679,15 +679,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, return result ? priv->status : -ETIMEDOUT; } - for (i = 1; i <= len; i++) { - if (i == len && read_write == I2C_SMBUS_READ) - smbcmd |= SMBHSTCNT_LAST_BYTE; - outb_p(smbcmd, SMBHSTCNT(priv)); - - if (i == 1) - outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START, - SMBHSTCNT(priv)); + if (len == 1 && read_write == I2C_SMBUS_READ) + smbcmd |= SMBHSTCNT_LAST_BYTE; + outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv)); + for (i = 1; i <= len; i++) { status = i801_wait_byte_done(priv); if (status) return status; @@ -710,9 +706,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, data->block[0] = len; } - /* Retrieve/store value in SMBBLKDAT */ - if (read_write == I2C_SMBUS_READ) + if (read_write == I2C_SMBUS_READ) { data->block[i] = inb_p(SMBBLKDAT(priv)); + if (i == len - 1) + outb_p(smbcmd | SMBHSTCNT_LAST_BYTE, SMBHSTCNT(priv)); + } + if (read_write == I2C_SMBUS_WRITE && i+1 <= len) outb_p(data->block[i+1], SMBBLKDAT(priv));
Currently we set SMBHSTCNT_LAST_BYTE only after the host has started receiving the last byte. If we get e.g. preempted before setting SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte before SMBHSTCNT_LAST_BYTE is set. Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code is also consistent with what we do in i801_isr_byte_done(). Fixes: efa3cb15ad8b ("i2c-i801: Refactor use of LAST_BYTE in i801_block_transaction_byte_by_byte") Reported-by: Jean Delvare <jdelvare@suse.com> Cc: stable@vger.kernel.org Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)