* [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
@ 2020-03-11 16:58 Kai-Heng Feng
2020-03-23 5:38 ` Kai-Heng Feng
2020-03-24 11:09 ` Wolfram Sang
0 siblings, 2 replies; 7+ messages in thread
From: Kai-Heng Feng @ 2020-03-11 16:58 UTC (permalink / raw)
To: ajayg
Cc: Kai-Heng Feng, Wolfram Sang, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
2020-03-11 16:58 [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status() Kai-Heng Feng
@ 2020-03-23 5:38 ` Kai-Heng Feng
2020-03-23 16:47 ` Ajay Gupta
2020-03-24 11:09 ` Wolfram Sang
1 sibling, 1 reply; 7+ messages in thread
From: Kai-Heng Feng @ 2020-03-23 5:38 UTC (permalink / raw)
To: ajayg
Cc: Wolfram Sang, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
2020-03-23 5:38 ` Kai-Heng Feng
@ 2020-03-23 16:47 ` Ajay Gupta
2020-03-23 17:20 ` Kai-Heng Feng
0 siblings, 1 reply; 7+ messages in thread
From: Ajay Gupta @ 2020-03-23 16:47 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Wolfram Sang, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
2020-03-23 16:47 ` Ajay Gupta
@ 2020-03-23 17:20 ` Kai-Heng Feng
2020-03-24 3:51 ` Ajay Gupta
0 siblings, 1 reply; 7+ messages in thread
From: Kai-Heng Feng @ 2020-03-23 17:20 UTC (permalink / raw)
To: Ajay Gupta
Cc: Wolfram Sang, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
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
>>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
2020-03-23 17:20 ` Kai-Heng Feng
@ 2020-03-24 3:51 ` Ajay Gupta
0 siblings, 0 replies; 7+ messages in thread
From: Ajay Gupta @ 2020-03-24 3:51 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Wolfram Sang, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
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
> >>>
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
2020-03-11 16:58 [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status() Kai-Heng Feng
2020-03-23 5:38 ` Kai-Heng Feng
@ 2020-03-24 11:09 ` Wolfram Sang
2020-03-24 11:12 ` Kai-Heng Feng
1 sibling, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2020-03-24 11:09 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: ajayg, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status()
2020-03-24 11:09 ` Wolfram Sang
@ 2020-03-24 11:12 ` Kai-Heng Feng
0 siblings, 0 replies; 7+ messages in thread
From: Kai-Heng Feng @ 2020-03-24 11:12 UTC (permalink / raw)
To: Wolfram Sang
Cc: ajayg, Andy Shevchenko,
open list:I2C CONTROLLER DRIVER FOR NVIDIA GPU, open list
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-24 11:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 16:58 [PATCH] i2c: nvidia-gpu: Handle timeout correctly in gpu_i2c_check_status() Kai-Heng Feng
2020-03-23 5:38 ` Kai-Heng Feng
2020-03-23 16:47 ` Ajay Gupta
2020-03-23 17:20 ` Kai-Heng Feng
2020-03-24 3:51 ` Ajay Gupta
2020-03-24 11:09 ` Wolfram Sang
2020-03-24 11:12 ` Kai-Heng Feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).