On 03.12.2021 10:59, Jean Delvare wrote: > Hi Heiner, > > On Thu, 02 Dec 2021 10:53:05 +0100, Heiner Kallweit wrote: >> I see no need to read back the registers to verify that the bits >> have actually been cleared. I can't imagine any scenario where >> the bits would remain set after a write to them. > > This happened at least once in the past. See this archived message: > > https://www.spinics.net/lists/linux-i2c/msg02651.html > "My last attempt locked the SMBus, but I was able to recover by repeatedly writing to the HST_STS register, as may times as the block length." OK, this was 11 yrs ago, so at least I wouldn't be able to recall in detail what happened back then .. Question is how you did this "repeatedly writing to the HST_STS register". Something like the following? while (status = in (STATUS)) out(STATUS, status); Or maybe the driver started the loop to process the next byte? I think it's not likely that when writing a status bit it remained set. As we now know E32B is ignored in I2C mode, therefore the chip can read/write only one byte in a row, and w/o setting SMBHSTCNT_START in between it wouldn't touch the next byte. Of course I may be wrong with my assumptions .. > This was in i801_check_post(), not i801_check_pre(), but that was the > same code. Which was removed in > 6cad93c4bbd62ecfa2e1b3a95c1ac4f6f27764c7 because there was little point > in checking the same condition twice. > > Unfortunately it seems that the error messages were copied manually so > we lack the details of which status bit couldn't be cleared exactly. > > Granted, it was caused by a driver bug, which was fixed since (commit > c074c39d62306efa5ba7c69c1a1531bc7333d252) but this shows that the > condition can actually trigger. > >> Whilst at it, change involved syslog messages to use pci_dbg() et al. >> to simplify them. > > Fine with me. > >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> >> --- >> drivers/i2c/busses/i2c-i801.c | 22 +++------------------- >> 1 file changed, 3 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c >> b/drivers/i2c/busses/i2c-i801.c index 720f7e9d0..a82aaef27 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -328,22 +328,14 @@ static int i801_check_pre(struct i801_priv >> *priv) >> status = inb_p(SMBHSTSTS(priv)); >> if (status & SMBHSTSTS_HOST_BUSY) { >> - dev_err(&priv->pci_dev->dev, "SMBus is busy, can't use it!\n"); >> + pci_err(priv->pci_dev, "SMBus is busy, can't use it!\n"); >> return -EBUSY; >> } >> >> status &= STATUS_FLAGS; >> if (status) { >> - dev_dbg(&priv->pci_dev->dev, "Clearing status flags (%02x)\n", >> - status); >> + pci_dbg(priv->pci_dev, "Clearing status flags (%02x)\n", status); >> outb_p(status, SMBHSTSTS(priv)); >> - status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS; >> - if (status) { >> - dev_err(&priv->pci_dev->dev, >> - "Failed clearing status flags (%02x)\n", >> - status); >> - return -EBUSY; >> - } >> } >> >> /* >> @@ -356,16 +348,8 @@ static int i801_check_pre(struct i801_priv *priv) >> if (priv->features & FEATURE_SMBUS_PEC) { >> status = inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE; >> if (status) { >> - dev_dbg(&priv->pci_dev->dev, >> - "Clearing aux status flags (%02x)\n", status); >> + pci_dbg(priv->pci_dev, "Clearing aux status flags (%02x)\n", status); >> outb_p(status, SMBAUXSTS(priv)); >> - status = inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE; >> - if (status) { >> - dev_err(&priv->pci_dev->dev, >> - "Failed clearing aux status flags (%02x)\n", >> - status); >> - return -EBUSY; >> - } >> } >> } >> > > So I'm not too sure what to do with this. On the one hand, the code you > want to remove could be useful to catch and investigate future bugs. > The rest of the code does assume that the status bits are properly > cleared before starting a new transaction. On the other hand, it is > slowing down the driver a bit when all is fine. Is that really worth > optimizing? > In a follow-up mail in the thread you mentioned is the following. I noticed the same (the 1ms delay is too short) and have related patches in my tree. However I'd like to finalize the cleanups first. "While working on this issue, I noticed that the piece of code which is supposed to let the i2c-i801 driver recover in case of a transaction timeout, did not always work."
