On 03.12.2021 13:55, Heiner Kallweit wrote: > 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. I mixed something up, START is needed only once. > 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." >
