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

[PATCH] 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.

Let's handle the unaccounted case that the target time equals to jiffies
in gpu_i2c_check_status(), so the UCSI driver can let the probe fail as
it should.

Fixes: c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 62e18b4db0ed..1988e93c7925 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -88,7 +88,7 @@ static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
 		usleep_range(500, 600);
 	} while (time_is_after_jiffies(target));
 
-	if (time_is_before_jiffies(target)) {
+	if (time_is_before_eq_jiffies(target)) {
 		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
 		return -ETIMEDOUT;
 	}
-- 
2.17.1

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

[Kai-Heng Feng]

> On Mar 12, 2020, at 00:58, Kai-Heng Feng <kai.heng.feng@canonical.com> 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.
> 
> Let's handle the unaccounted case that the target time equals to jiffies
> in gpu_i2c_check_status(), so the UCSI driver can let the probe fail as
> it should.
> 
> Fixes: c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

A gentle ping...

> ---
> drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> index 62e18b4db0ed..1988e93c7925 100644
> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -88,7 +88,7 @@ static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
> 		usleep_range(500, 600);
> 	} while (time_is_after_jiffies(target));
> 
> -	if (time_is_before_jiffies(target)) {
> +	if (time_is_before_eq_jiffies(target)) {
> 		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> 		return -ETIMEDOUT;
> 	}
> -- 
> 2.17.1
> 

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

[Ajay Gupta]

Kai-Heng

> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Sunday, March 22, 2020 10:38 PM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>; 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] i2c: nvidia-gpu: Handle timeout correctly in
> gpu_i2c_check_status()
> 
> External email: Use caution opening links or attachments
> 
> 
> > On Mar 12, 2020, at 00:58, Kai-Heng Feng <kai.heng.feng@canonical.com>
> 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.
> >
> > Let's handle the unaccounted case that the target time equals to
> > jiffies in gpu_i2c_check_status(), so the UCSI driver can let the
> > probe fail as it should.
If status is not seen in 999.5 ms then I don't see any reason why it will come
exactly at 1000ms. In fact,  we expect status to be seen within 160ms as per
I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT (16 cycle) and 
I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ (10ms/cycle) programmed in 
I2C_MST_I2C0_TIMING Register. We already have enough extra time to look
For response.

Thanks
> nvpublic
> >
> > Fixes: c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> A gentle ping...
> 
> > ---
> > drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > index 62e18b4db0ed..1988e93c7925 100644
> > --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > @@ -88,7 +88,7 @@ static int gpu_i2c_check_status(struct gpu_i2c_dev
> *i2cd)
> >               usleep_range(500, 600);
> >       } while (time_is_after_jiffies(target));
> >
> > -     if (time_is_before_jiffies(target)) {
> > +     if (time_is_before_eq_jiffies(target)) {
> >               dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> >               return -ETIMEDOUT;
> >       }
> > --
> > 2.17.1
> >

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

[Kai-Heng Feng]

Hi Ajay,

> On Mar 24, 2020, at 00:47, Ajay Gupta <ajayg@nvidia.com> wrote:
> 
> Kai-Heng
> 
>> -----Original Message-----
>> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> Sent: Sunday, March 22, 2020 10:38 PM
>> To: Ajay Gupta <ajayg@nvidia.com>
>> Cc: Wolfram Sang <wsa@the-dreams.de>; 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] i2c: nvidia-gpu: Handle timeout correctly in
>> gpu_i2c_check_status()
>> 
>> External email: Use caution opening links or attachments
>> 
>> 
>>> On Mar 12, 2020, at 00:58, Kai-Heng Feng <kai.heng.feng@canonical.com>
>> 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.
>>> 
>>> Let's handle the unaccounted case that the target time equals to
>>> jiffies in gpu_i2c_check_status(), so the UCSI driver can let the
>>> probe fail as it should.
> If status is not seen in 999.5 ms then I don't see any reason why it will come
> exactly at 1000ms. In fact,  we expect status to be seen within 160ms as per
> I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT (16 cycle) and 
> I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ (10ms/cycle) programmed in 
> I2C_MST_I2C0_TIMING Register. We already have enough extra time to look
> For response.

This is to handle when there's no response.

When the while loop terminates because of "time_is_after_jiffies(target)" (i.e. target <= jiffies), we also need to to handle "target == jiffies" case in the following if clause to properly timeout.

I don't think I2C timings here can affect jiffies.

Kai-Heng

> 
> Thanks
>> nvpublic
>>> 
>>> Fixes: c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> 
>> A gentle ping...
>> 
>>> ---
>>> drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> b/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> index 62e18b4db0ed..1988e93c7925 100644
>>> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
>>> @@ -88,7 +88,7 @@ static int gpu_i2c_check_status(struct gpu_i2c_dev
>> *i2cd)
>>>              usleep_range(500, 600);
>>>      } while (time_is_after_jiffies(target));
>>> 
>>> -     if (time_is_before_jiffies(target)) {
>>> +     if (time_is_before_eq_jiffies(target)) {
>>>              dev_err(i2cd->dev, "i2c timeout error %x\n", val);
>>>              return -ETIMEDOUT;
>>>      }
>>> --
>>> 2.17.1
>>> 
> 

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

[Ajay Gupta]

Hi Kai-Heng

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org>
> On Behalf Of Kai-Heng Feng
> Sent: Monday, March 23, 2020 10:21 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>; 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] i2c: nvidia-gpu: Handle timeout correctly in
> gpu_i2c_check_status()
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Ajay,
> 
> > On Mar 24, 2020, at 00:47, Ajay Gupta <ajayg@nvidia.com> wrote:
> >
> > Kai-Heng
> >
> >> -----Original Message-----
> >> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >> Sent: Sunday, March 22, 2020 10:38 PM
> >> To: Ajay Gupta <ajayg@nvidia.com>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>; 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] i2c: nvidia-gpu: Handle timeout correctly in
> >> gpu_i2c_check_status()
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >>> On Mar 12, 2020, at 00:58, Kai-Heng Feng
> >>> <kai.heng.feng@canonical.com>
> >> 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.
> >>>
> >>> Let's handle the unaccounted case that the target time equals to
> >>> jiffies in gpu_i2c_check_status(), so the UCSI driver can let the
> >>> probe fail as it should.
> > If status is not seen in 999.5 ms then I don't see any reason why it
> > will come exactly at 1000ms. In fact,  we expect status to be seen
> > within 160ms as per I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT (16
> cycle) and
> > I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ (10ms/cycle) programmed in
> > I2C_MST_I2C0_TIMING Register. We already have enough extra time to
> > look For response.
> 
> This is to handle when there's no response.
> 
> When the while loop terminates because of "time_is_after_jiffies(target)"
> (i.e. target <= jiffies), we also need to to handle "target == jiffies" case in the
> following if clause to properly timeout.
Ok got it. The change looks fine to me.

Acked-by: Ajay Gupta <ajayg@nvidia.com>

Thanks
> nvpublic
> 
> I don't think I2C timings here can affect jiffies.
> 
> Kai-Heng
> 
> >
> > Thanks
> >> nvpublic
> >>>
> >>> Fixes: c71bcdcb42a7 ("i2c: add i2c bus driver for NVIDIA GPU")
> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>
> >> A gentle ping...
> >>
> >>> ---
> >>> drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
> >>> b/drivers/i2c/busses/i2c-nvidia-gpu.c
> >>> index 62e18b4db0ed..1988e93c7925 100644
> >>> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> >>> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> >>> @@ -88,7 +88,7 @@ static int gpu_i2c_check_status(struct gpu_i2c_dev
> >> *i2cd)
> >>>              usleep_range(500, 600);
> >>>      } while (time_is_after_jiffies(target));
> >>>
> >>> -     if (time_is_before_jiffies(target)) {
> >>> +     if (time_is_before_eq_jiffies(target)) {
> >>>              dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> >>>              return -ETIMEDOUT;
> >>>      }
> >>> --
> >>> 2.17.1
> >>>
> >

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

[Wolfram Sang]

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


>  	} while (time_is_after_jiffies(target));
>  
> -	if (time_is_before_jiffies(target)) {
> +	if (time_is_before_eq_jiffies(target)) {

While unlikely, there is a tiny race between the time_is_* calls,
jiffies could update inbetween them.

So, for the sake of good programming practice, I'd recommend to set a
flag in the do_while-loop and the have the logic above solely based on
the flag.


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

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

[Kai-Heng Feng]

Hi Wolfram,

> On Mar 24, 2020, at 19:09, Wolfram Sang <wsa@the-dreams.de> wrote:
> 
> 
>> 	} while (time_is_after_jiffies(target));
>> 
>> -	if (time_is_before_jiffies(target)) {
>> +	if (time_is_before_eq_jiffies(target)) {
> 
> While unlikely, there is a tiny race between the time_is_* calls,
> jiffies could update inbetween them.
> 
> So, for the sake of good programming practice, I'd recommend to set a
> flag in the do_while-loop and the have the logic above solely based on
> the flag.
> 

Ok, I'll send a v2 based on your suggestion.

Kai-Heng