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 <[email protected]>
---
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
> On Mar 12, 2020, at 00:58, Kai-Heng Feng <[email protected]> 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 <[email protected]>
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
>
Kai-Heng
> -----Original Message-----
> From: Kai-Heng Feng <[email protected]>
> Sent: Sunday, March 22, 2020 10:38 PM
> To: Ajay Gupta <[email protected]>
> Cc: Wolfram Sang <[email protected]>; Andy Shevchenko
> <[email protected]>; open list:I2C CONTROLLER DRIVER FOR
> NVIDIA GPU <[email protected]>; open list <linux-
> [email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
>
> 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
> >
Hi Ajay,
> On Mar 24, 2020, at 00:47, Ajay Gupta <[email protected]> wrote:
>
> Kai-Heng
>
>> -----Original Message-----
>> From: Kai-Heng Feng <[email protected]>
>> Sent: Sunday, March 22, 2020 10:38 PM
>> To: Ajay Gupta <[email protected]>
>> Cc: Wolfram Sang <[email protected]>; Andy Shevchenko
>> <[email protected]>; open list:I2C CONTROLLER DRIVER FOR
>> NVIDIA GPU <[email protected]>; open list <linux-
>> [email protected]>
>> 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 <[email protected]>
>> 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 <[email protected]>
>>
>> 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
>>>
>
Hi Kai-Heng
> -----Original Message-----
> From: [email protected] <[email protected]>
> On Behalf Of Kai-Heng Feng
> Sent: Monday, March 23, 2020 10:21 AM
> To: Ajay Gupta <[email protected]>
> Cc: Wolfram Sang <[email protected]>; Andy Shevchenko
> <[email protected]>; open list:I2C CONTROLLER DRIVER
> FOR NVIDIA GPU <[email protected]>; open list <linux-
> [email protected]>
> 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 <[email protected]> wrote:
> >
> > Kai-Heng
> >
> >> -----Original Message-----
> >> From: Kai-Heng Feng <[email protected]>
> >> Sent: Sunday, March 22, 2020 10:38 PM
> >> To: Ajay Gupta <[email protected]>
> >> Cc: Wolfram Sang <[email protected]>; Andy Shevchenko
> >> <[email protected]>; open list:I2C CONTROLLER DRIVER
> >> FOR NVIDIA GPU <[email protected]>; open list <linux-
> >> [email protected]>
> >> 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
> >>> <[email protected]>
> >> 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 <[email protected]>
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 <[email protected]>
> >>
> >> 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
> >>>
> >
> } 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.
Hi Wolfram,
> On Mar 24, 2020, at 19:09, Wolfram Sang <[email protected]> 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