Skip to contentSkip to conversation

i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

PATCH Applied Kai-Heng Feng 4 participants 5 messages Mar 24, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

Nvidia card may come with a "phantom" UCSI device, and its driver gets stuck in probe routine, prevents any system PM operations like suspend.

There's an unaccounted case that the target time can equal to jiffies in gpu_i2c_check_status(), let's solve that by using readl_poll_timeout() instead of jiffies comparison functions.

Fixes c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")

---
v3:
- Use readl_poll_timeout to make the retry loop simpler.

v2:
- Use a boolean to make sure the operation is timeout or not.

 drivers/i2c/busses/i2c-nvidia-gpu.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/i2c/busses/i2c-nvidia-gpu.c +8 -12
index 62e18b4db0ed..f5d25ce00f03 100644
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
@@ -75,20 +76,15 @@ static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
 
 static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
 {
-	unsigned long target = jiffies + msecs_to_jiffies(1000);
 	u32 val;
+	int ret;
 
-	do {
-		val = readl(i2cd->regs + I2C_MST_CNTL);
-		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
-			break;
-		if ((val & I2C_MST_CNTL_STATUS) !=
-				I2C_MST_CNTL_STATUS_BUS_BUSY)
-			break;
-		usleep_range(500, 600);
-	} while (time_is_after_jiffies(target));
-
-	if (time_is_before_jiffies(target)) {
+	ret = readl_poll_timeout(i2cd->regs + I2C_MST_CNTL, val,
+				 !(val & I2C_MST_CNTL_CYCLE_TRIGGER) ||
+				 (val & I2C_MST_CNTL_STATUS) != I2C_MST_CNTL_STATUS_BUS_BUSY,
+				 500, 1000 * USEC_PER_MSEC);
+
+	if (ret) {
 		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
 		return -ETIMEDOUT;
 	}
Signature · 1 line
2.17.1
Reviewed-by on v3 1/1
#1 lore raw
Quoted 7 lines · Kai-Heng Feng

Nvidia card may come with a "phantom" UCSI device, and its driver gets stuck in probe routine, prevents any system PM operations like suspend.

There's an unaccounted case that the target time can equal to jiffies in gpu_i2c_check_status(), let's solve that by using readl_poll_timeout() instead of jiffies comparison functions.

Thank you!

Quoted 56 lines
Fixes c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")

---
v3:
- Use readl_poll_timeout to make the retry loop simpler.

v2:
- Use a boolean to make sure the operation is timeout or not.

 drivers/i2c/busses/i2c-nvidia-gpu.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/i2c/busses/i2c-nvidia-gpu.c +8 -12
index 62e18b4db0ed..f5d25ce00f03 100644
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
@@ -75,20 +76,15 @@ static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
 
 static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
 {
-	unsigned long target = jiffies + msecs_to_jiffies(1000);
 	u32 val;
+	int ret;
 
-	do {
-		val = readl(i2cd->regs + I2C_MST_CNTL);
-		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
-			break;
-		if ((val & I2C_MST_CNTL_STATUS) !=
-				I2C_MST_CNTL_STATUS_BUS_BUSY)
-			break;
-		usleep_range(500, 600);
-	} while (time_is_after_jiffies(target));
-
-	if (time_is_before_jiffies(target)) {
+	ret = readl_poll_timeout(i2cd->regs + I2C_MST_CNTL, val,
+				 !(val & I2C_MST_CNTL_CYCLE_TRIGGER) ||
+				 (val & I2C_MST_CNTL_STATUS) != I2C_MST_CNTL_STATUS_BUS_BUSY,
+				 500, 1000 * USEC_PER_MSEC);
+
+	if (ret) {
 		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
 		return -ETIMEDOUT;
 	}
Signature · 1 line
2.17.1
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#2 lore raw
In reply to Kai-Heng Feng
Quoted 10 lines · Kai-Heng Feng

Nvidia card may come with a "phantom" UCSI device, and its driver gets stuck in probe routine, prevents any system PM operations like suspend.

There's an unaccounted case that the target time can equal to jiffies in gpu_i2c_check_status(), let's solve that by using readl_poll_timeout() instead of jiffies comparison functions.

Fixes c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")

Looks good to me, thanks Andy for the suggestion!

Waiting for the Rev-by from Ajay (driver maintainer).

maintainer Reviewed-by Tested-by
#3 lore raw

Hi Wolfram,

Quoted 19 lines

-----Original Message-----
From: Wolfram Sang <[email protected]>
Sent: Tuesday, March 24, 2020 1:58 PM
To: Kai-Heng Feng <[email protected]>
Cc: Ajay Gupta <[email protected]>; Andy Shevchenko <[email protected]>; open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU <[email protected]>; open list <linux- [email protected]>
Subject: Re: [PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

Quoted 7 lines · Kai-Heng Feng

Nvidia card may come with a "phantom" UCSI device, and its driver gets stuck in probe routine, prevents any system PM operations like suspend.

There's an unaccounted case that the target time can equal to jiffies in gpu_i2c_check_status(), let's solve that by using readl_poll_timeout() instead of jiffies comparison functions.

Quoted 7 lines
Quoted 3 lines
Fixes c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")

Looks good to me, thanks Andy for the suggestion!

Waiting for the Rev-by from Ajay (driver maintainer).

Thanks
Ajay

Quoted 1 line

nvpublic

Applied
#4 lore raw
In reply to Kai-Heng Feng
Quoted 10 lines · Kai-Heng Feng

Nvidia card may come with a "phantom" UCSI device, and its driver gets stuck in probe routine, prevents any system PM operations like suspend.

There's an unaccounted case that the target time can equal to jiffies in gpu_i2c_check_status(), let's solve that by using readl_poll_timeout() instead of jiffies comparison functions.

Fixes c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")

Applied to for-current, thanks!