[PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

[PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

[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")
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
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(-)

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 62e18b4db0ed..f5d25ce00f03 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -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;
 	}
-- 
2.17.1

Re: [PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

[Andy Shevchenko]

On Tue, Mar 24, 2020 at 11:28:11PM +0800, Kai-Heng Feng wrote:
> 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!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> Fixes: c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> 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(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> index 62e18b4db0ed..f5d25ce00f03 100644
> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -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;
>  	}
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On Tue, Mar 24, 2020 at 11:28:11PM +0800, Kai-Heng Feng wrote:
> 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")
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Looks good to me, thanks Andy for the suggestion!

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

[Ajay Gupta]

Hi Wolfram,

> -----Original Message-----
> From: Wolfram Sang <wsa@the-dreams.de>
> Sent: Tuesday, March 24, 2020 1:58 PM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: Ajay Gupta <ajayg@nvidia.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; open list:I2C CONTROLLER DRIVER
> FOR NVIDIA GPU <linux-i2c@vger.kernel.org>; open list <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in
> gpu_i2c_check_status()
> 
> On Tue, Mar 24, 2020 at 11:28:11PM +0800, Kai-Heng Feng wrote:
> > 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")
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> Looks good to me, thanks Andy for the suggestion!
> 
> Waiting for the Rev-by from Ajay (driver maintainer).
Reviewed-by: Ajay Gupta <ajayg@nvidia.com>
Tested-by: Ajay Gupta <ajayg@nvidia.com> 

Thanks
Ajay
> nvpublic

Re: [PATCH v3] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On Tue, Mar 24, 2020 at 11:28:11PM +0800, Kai-Heng Feng wrote:
> 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")
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]