[PATCH 0/3] i2c: i801: Avoid potential issue in i801_check_post and cosmetic improvements

[PATCH 0/3] i2c: i801: Avoid potential issue in i801_check_post and cosmetic improvements

[Heiner Kallweit]

This series avoids a potential issue in i801_check_post () and
includes smaller, more cosmetic improvements.

Heiner Kallweit (3):
  i2c: i801: Cosmetic improvements
  i2c: i801: Move i801_wait_intr and i801_wait_byte_done in the code
  i2c: i801: Improve too small kill wait time in i801_check_post

 drivers/i2c/busses/i2c-i801.c | 125 ++++++++++++++++------------------
 1 file changed, 57 insertions(+), 68 deletions(-)

-- 
2.43.0

[PATCH 1/3] i2c: i801: Cosmetic improvements

[Heiner Kallweit]

- Use pci_err et al instead of dev_err to simplify the code
- Combine an error and subsequent debug message in i801_check_post()
- use format %pr to print resource

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 54 ++++++++++++++---------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 918c794c7..4951d7d90 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -398,25 +398,22 @@ static int i801_check_post(struct i801_priv *priv, int status)
 	 * If the SMBus is still busy, we give up
 	 */
 	if (unlikely(status < 0)) {
-		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
+		pci_err(priv->pci_dev, "Timeout, terminating transaction\n");
 		/* try to stop the current command */
-		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
 		outb_p(SMBHSTCNT_KILL, SMBHSTCNT(priv));
 		usleep_range(1000, 2000);
 		outb_p(0, SMBHSTCNT(priv));
 
 		/* Check if it worked */
 		status = inb_p(SMBHSTSTS(priv));
-		if ((status & SMBHSTSTS_HOST_BUSY) ||
-		    !(status & SMBHSTSTS_FAILED))
-			dev_err(&priv->pci_dev->dev,
-				"Failed terminating the transaction\n");
+		if ((status & SMBHSTSTS_HOST_BUSY) || !(status & SMBHSTSTS_FAILED))
+			pci_err(priv->pci_dev, "Failed terminating the transaction\n");
 		return -ETIMEDOUT;
 	}
 
 	if (status & SMBHSTSTS_FAILED) {
 		result = -EIO;
-		dev_err(&priv->pci_dev->dev, "Transaction failed\n");
+		pci_err(priv->pci_dev, "Transaction failed\n");
 	}
 	if (status & SMBHSTSTS_DEV_ERR) {
 		/*
@@ -444,7 +441,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
 	}
 	if (status & SMBHSTSTS_BUS_ERR) {
 		result = -EAGAIN;
-		dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
+		pci_dbg(priv->pci_dev, "Lost arbitration\n");
 	}
 
 	return result;
@@ -572,8 +569,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 		if (priv->count < priv->len)
 			priv->data[priv->count++] = inb(SMBBLKDAT(priv));
 		else
-			dev_dbg(&priv->pci_dev->dev,
-				"Discarding extra byte on block read\n");
+			pci_dbg(priv->pci_dev, "Discarding extra byte on block read\n");
 
 		/* Set LAST_BYTE for last byte of read transaction */
 		if (priv->count == priv->len - 1)
@@ -1262,7 +1258,7 @@ static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
 	}
 
 	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
-		dev_warn(&priv->pci_dev->dev,
+		pci_warn(priv->pci_dev,
 			 "Accelerometer lis3lv02d is present on SMBus but its"
 			 " address is unknown, skipping registration\n");
 		return;
@@ -1533,7 +1529,7 @@ static void i801_add_tco(struct i801_priv *priv)
 		priv->tco_pdev = i801_add_tco_spt(pci_dev, tco_res);
 
 	if (IS_ERR(priv->tco_pdev))
-		dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
+		pci_warn(pci_dev, "failed to create iTCO device\n");
 }
 
 #ifdef CONFIG_ACPI
@@ -1562,8 +1558,8 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
 		priv->acpi_reserved = true;
 
-		dev_warn(&pdev->dev, "BIOS is accessing SMBus registers\n");
-		dev_warn(&pdev->dev, "Driver SMBus register access inhibited\n");
+		pci_warn(pdev, "BIOS is accessing SMBus registers\n");
+		pci_warn(pdev, "Driver SMBus register access inhibited\n");
 
 		/*
 		 * BIOS is accessing the host controller so prevent it from
@@ -1644,8 +1640,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Disable features on user request */
 	for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) {
 		if (priv->features & disable_features & (1 << i))
-			dev_notice(&dev->dev, "%s disabled by user\n",
-				   i801_feature_names[i]);
+			pci_notice(dev, "%s disabled by user\n", i801_feature_names[i]);
 	}
 	priv->features &= ~disable_features;
 
@@ -1655,8 +1650,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	err = pcim_enable_device(dev);
 	if (err) {
-		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
-			err);
+		pci_err(dev, "Failed to enable SMBus PCI device (%d)\n", err);
 		return err;
 	}
 	pcim_pin_device(dev);
@@ -1664,8 +1658,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Determine the address of the SMBus area */
 	priv->smba = pci_resource_start(dev, SMBBAR);
 	if (!priv->smba) {
-		dev_err(&dev->dev,
-			"SMBus base address uninitialized, upgrade BIOS\n");
+		pci_err(dev, "SMBus base address uninitialized, upgrade BIOS\n");
 		return -ENODEV;
 	}
 
@@ -1674,26 +1667,24 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	err = pcim_iomap_regions(dev, 1 << SMBBAR, DRV_NAME);
 	if (err) {
-		dev_err(&dev->dev,
-			"Failed to request SMBus region 0x%lx-0x%Lx\n",
-			priv->smba,
-			(unsigned long long)pci_resource_end(dev, SMBBAR));
+		pci_err(dev, "Failed to request SMBus region %pr\n",
+			pci_resource_n(dev, SMBBAR));
 		i801_acpi_remove(priv);
 		return err;
 	}
 
-	pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &priv->original_hstcfg);
+	pci_read_config_byte(dev, SMBHSTCFG, &priv->original_hstcfg);
 	i801_setup_hstcfg(priv);
 	if (!(priv->original_hstcfg & SMBHSTCFG_HST_EN))
-		dev_info(&dev->dev, "Enabling SMBus device\n");
+		pci_info(dev, "Enabling SMBus device\n");
 
 	if (priv->original_hstcfg & SMBHSTCFG_SMB_SMI_EN) {
-		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
+		pci_dbg(dev, "SMBus using interrupt SMI#\n");
 		/* Disable SMBus interrupt feature if SMBus using SMI# */
 		priv->features &= ~FEATURE_IRQ;
 	}
 	if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
-		dev_info(&dev->dev, "SPD Write Disable is set\n");
+		pci_info(dev, "SPD Write Disable is set\n");
 
 	/* Clear special mode bits */
 	if (priv->features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))
@@ -1712,7 +1703,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		/* Complain if an interrupt is already pending */
 		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
 		if (pcists & PCI_STATUS_INTERRUPT)
-			dev_warn(&dev->dev, "An interrupt is pending!\n");
+			pci_warn(dev, "An interrupt is pending!\n");
 	}
 
 	if (priv->features & FEATURE_IRQ) {
@@ -1721,12 +1712,11 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,
 				       IRQF_SHARED, DRV_NAME, priv);
 		if (err) {
-			dev_err(&dev->dev, "Failed to allocate irq %d: %d\n",
-				dev->irq, err);
+			pci_err(dev, "Failed to allocate irq %d: %d\n", dev->irq, err);
 			priv->features &= ~FEATURE_IRQ;
 		}
 	}
-	dev_info(&dev->dev, "SMBus using %s\n",
+	pci_info(dev, "SMBus using %s\n",
 		 priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
 
 	/* Host notification uses an interrupt */
-- 
2.43.0

[PATCH 2/3] i2c: i801: Move i801_wait_intr and i801_wait_byte_done in the code

[Heiner Kallweit]

Move both functions to avoid forward declarations in a subsequent patch.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 68 +++++++++++++++++------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 4951d7d90..751b7c6d2 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -330,6 +330,40 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
 	"\t\t  0x10  don't use interrupts\n"
 	"\t\t  0x20  disable SMBus Host Notify ");
 
+/* Wait for BUSY being cleared and either INTR or an error flag being set */
+static int i801_wait_intr(struct i801_priv *priv)
+{
+	unsigned long timeout = jiffies + priv->adapter.timeout;
+	int status, busy;
+
+	do {
+		usleep_range(250, 500);
+		status = inb_p(SMBHSTSTS(priv));
+		busy = status & SMBHSTSTS_HOST_BUSY;
+		status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
+		if (!busy && status)
+			return status & STATUS_ERROR_FLAGS;
+	} while (time_is_after_eq_jiffies(timeout));
+
+	return -ETIMEDOUT;
+}
+
+/* Wait for either BYTE_DONE or an error flag being set */
+static int i801_wait_byte_done(struct i801_priv *priv)
+{
+	unsigned long timeout = jiffies + priv->adapter.timeout;
+	int status;
+
+	do {
+		usleep_range(250, 500);
+		status = inb_p(SMBHSTSTS(priv));
+		if (status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE))
+			return status & STATUS_ERROR_FLAGS;
+	} while (time_is_after_eq_jiffies(timeout));
+
+	return -ETIMEDOUT;
+}
+
 static int i801_get_block_len(struct i801_priv *priv)
 {
 	u8 len = inb_p(SMBHSTDAT0(priv));
@@ -447,40 +481,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
 	return result;
 }
 
-/* Wait for BUSY being cleared and either INTR or an error flag being set */
-static int i801_wait_intr(struct i801_priv *priv)
-{
-	unsigned long timeout = jiffies + priv->adapter.timeout;
-	int status, busy;
-
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-		busy = status & SMBHSTSTS_HOST_BUSY;
-		status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
-		if (!busy && status)
-			return status & STATUS_ERROR_FLAGS;
-	} while (time_is_after_eq_jiffies(timeout));
-
-	return -ETIMEDOUT;
-}
-
-/* Wait for either BYTE_DONE or an error flag being set */
-static int i801_wait_byte_done(struct i801_priv *priv)
-{
-	unsigned long timeout = jiffies + priv->adapter.timeout;
-	int status;
-
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-		if (status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE))
-			return status & STATUS_ERROR_FLAGS;
-	} while (time_is_after_eq_jiffies(timeout));
-
-	return -ETIMEDOUT;
-}
-
 static int i801_transaction(struct i801_priv *priv, int xact)
 {
 	unsigned long result;
-- 
2.43.0

[PATCH 3/3] i2c: i801: Improve too small kill wait time in i801_check_post

[Heiner Kallweit]

In my tests terminating a transaction took about 25ms, what is
in line with the chip-internal timeout as described in 5.21.3.2
"Bus Time Out" in [0]. Therefore the 2ms delay is too low.
Instead of a fixed delay let's use i801_wait_intr() here,
this also facilitates the status handling.

This potential issue seems to have been existing forever, but as no
related problem is known, treat it as an improvement.

[0] Intel document #326776-003, 7 Series PCH datasheet

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 751b7c6d2..5a84eb1a6 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -435,12 +435,11 @@ static int i801_check_post(struct i801_priv *priv, int status)
 		pci_err(priv->pci_dev, "Timeout, terminating transaction\n");
 		/* try to stop the current command */
 		outb_p(SMBHSTCNT_KILL, SMBHSTCNT(priv));
-		usleep_range(1000, 2000);
+		status = i801_wait_intr(priv);
 		outb_p(0, SMBHSTCNT(priv));
 
 		/* Check if it worked */
-		status = inb_p(SMBHSTSTS(priv));
-		if ((status & SMBHSTSTS_HOST_BUSY) || !(status & SMBHSTSTS_FAILED))
+		if (status < 0 || !(status & SMBHSTSTS_FAILED))
 			pci_err(priv->pci_dev, "Failed terminating the transaction\n");
 		return -ETIMEDOUT;
 	}
-- 
2.43.0

Re: [PATCH 0/3] i2c: i801: Avoid potential issue in i801_check_post and cosmetic improvements

[Andi Shyti]

Hi Heiner,

On Sat, Feb 10, 2024 at 04:25:30PM +0100, Heiner Kallweit wrote:
> This series avoids a potential issue in i801_check_post () and
> includes smaller, more cosmetic improvements.
> 
> Heiner Kallweit (3):
>   i2c: i801: Cosmetic improvements
>   i2c: i801: Move i801_wait_intr and i801_wait_byte_done in the code
>   i2c: i801: Improve too small kill wait time in i801_check_post

this series went forgotten. As I'm browing through the forgotten
patches, I want to know if this is still necessary (it looks OK
to me to bring it forward).

If so, may I ask you to please rebase it on top of the latest
i2c/host as it currently doesn't apply?

Thanks,
Andi