NVIDIA Tegra I2C synchronization correction

Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.

Signed-off-by Dmitry Osipenko <[email protected]>

Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Hi Dmitry,

Quoted 11 lines · Dmitry Osipenko

Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

I have noticed a regression on tegra30-cardhu-a04 when testing system suspend. Git bisect is pointing to this commit and reverting it fixes the problem. In the below console log I2C fails to resume ...

[   60.690035] PM: Device 3000.pcie failed to resume noirq: error -16

[   60.696859] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: EMEM address decode error (SMMU translation error [--S])

[   60.708876] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: Page fault (SMMU translation error [--S])

Have you seen this?

Hello Jon,

20.04.2020 22:53, Jon Hunter пишет:

Quoted 20 lines

Hi Dmitry,

Quoted 11 lines · Dmitry Osipenko

Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

I have noticed a regression on tegra30-cardhu-a04 when testing system suspend. Git bisect is pointing to this commit and reverting it fixes the problem. In the below console log I2C fails to resume ...

...

Quoted 1 line

[   60.690035] PM: Device 3000.pcie failed to resume noirq: error -16

...

Quoted 3 lines

[   60.696859] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: EMEM address decode error (SMMU translation error [--S])

[   60.708876] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: Page fault (SMMU translation error [--S])

This looks very wrong, the error tells that 3d hardware is active and doing something odd. Are you running some 3d tests?

...

Quoted 1 line

Have you seen this?

No, I haven't seen that. I'm not using PCIE and it looks like it's the problem.

Looking at the PCIE driver code, seems it's not syncing the RPM state on suspend/resume.

Please try this change:

--- >8 ---

drivers/pci/controller/pci-tegra.c +2 -2

index 3e64ba6a36a8..b1fcbae4109c 100644
@@ -2870,8 +2870,8 @@ static int __maybe_unused
 tegra_pcie_pm_resume(struct device *dev)
 
 static const struct dev_pm_ops tegra_pcie_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
-				      tegra_pcie_pm_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				      pm_runtime_force_resume)
 };
 

static struct platform_driver tegra_pcie_driver = {
--- >8 ---

Secondly, I2C driver suspends on NOIRQ level, while APBDMA driver suspends on default level. This is also wrong, please try to apply this hunk as well:

--- >8 ---

drivers/dma/tegra20-apb-dma.c +1 -1

index f6a2f42ffc51..e682ac86bd27 100644
@@ -1653,7 +1653,7 @@ static int __maybe_unused
 tegra_dma_dev_resume(struct device *dev)
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
 			   NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
 };
 

static const struct of_device_id tegra_dma_of_match[] = {
--- >8 ---

Hi Dmitry,

Quoted 31 lines · Dmitry Osipenko

21.04.2020 01:11, Dmitry Osipenko пишет:

Quoted 30 lines

Hello Jon,

20.04.2020 22:53, Jon Hunter пишет:

Quoted 19 lines

Hi Dmitry,

Quoted 11 lines · Dmitry Osipenko

Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

I have noticed a regression on tegra30-cardhu-a04 when testing system suspend. Git bisect is pointing to this commit and reverting it fixes the problem. In the below console log I2C fails to resume ...

...

Quoted 1 line

[   60.690035] PM: Device 3000.pcie failed to resume noirq: error -16

...

Quoted 3 lines

[   60.696859] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: EMEM address decode error (SMMU translation error [--S])

[   60.708876] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: Page fault (SMMU translation error [--S])

This looks very wrong, the error tells that 3d hardware is active and doing something odd. Are you running some 3d tests?

I am not running any GFX tests. However, I am not sure if the above is unrelated.

Quoted 48 lines

Quoted 48 lines

Quoted 1 line

Have you seen this?

No, I haven't seen that. I'm not using PCIE and it looks like it's the problem.

Looking at the PCIE driver code, seems it's not syncing the RPM state on suspend/resume.

Please try this change:

--- >8 ---

drivers/pci/controller/pci-tegra.c +2 -2

index 3e64ba6a36a8..b1fcbae4109c 100644
@@ -2870,8 +2870,8 @@ static int __maybe_unused
 tegra_pcie_pm_resume(struct device *dev)
 
 static const struct dev_pm_ops tegra_pcie_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_pcie_pm_suspend, tegra_pcie_pm_resume, NULL)
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_pcie_pm_suspend,
-				      tegra_pcie_pm_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				      pm_runtime_force_resume)
 };
 

static struct platform_driver tegra_pcie_driver = {
--- >8 ---

Secondly, I2C driver suspends on NOIRQ level, while APBDMA driver suspends on default level. This is also wrong, please try to apply this hunk as well:

--- >8 ---

drivers/dma/tegra20-apb-dma.c +1 -1

index f6a2f42ffc51..e682ac86bd27 100644
@@ -1653,7 +1653,7 @@ static int __maybe_unused
 tegra_dma_dev_resume(struct device *dev)
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
 			   NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
 };
 

Quoted 10 lines

Quoted 3 lines

static const struct of_device_id tegra_dma_of_match[] = {
--- >8 ---

Although, I'm now having a second though about the APBDMA change... I'm recalling that there are some complications in regards to PCIE driver suspending, requiring it to be at NOIRQ level, but this should be wrong because PCIE driver uses voltage regulator driver at NOIRQ level, while regulator drivers suspend on default level. The current behavior of the PCIE driver should be wrong, I think it needs to be moved to the default suspend-resume level somehow.

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

I am adding Manikanta to get some feedback on why we moved the PCI suspend to the NOIRQ phase because it is not clear to me if we need to do this here.

Manikanta, can you comment on whether we really need to suspend Tegra PCI during the noirq phase?

Cheers
Jon

Quoted 6 lines

I am adding Manikanta to get some feedback on why we moved the PCI suspend to the NOIRQ phase because it is not clear to me if we need to do this here.

Manikanta, can you comment on whether we really need to suspend Tegra PCI during the noirq phase?

PCIe subsystem driver implemented noirq PM callbacks, it will save & restore endpoint config space in these PM callbacks. PCIe controller should be available during this time, so noirq PM callbacks are implemented in Tegra PCIe driver.

file: drivers/pci/pci-driver.c

static const struct dev_pm_ops pci_dev_pm_ops = {
	...
        .suspend_noirq = pci_pm_suspend_noirq,
        .resume_noirq = pci_pm_resume_noirq,
        ...
};

On 21/04/2020 13:39, Manikanta Maddireddy wrote:

...

Quoted 11 lines

Quoted 6 lines

I am adding Manikanta to get some feedback on why we moved the PCI suspend to the NOIRQ phase because it is not clear to me if we need to do this here.

Manikanta, can you comment on whether we really need to suspend Tegra PCI during the noirq phase?

PCIe subsystem driver implemented noirq PM callbacks, it will save & restore endpoint config space in these PM callbacks. PCIe controller should be available during this time, so noirq PM callbacks are implemented in Tegra PCIe driver.

Quoted 7 lines

file: drivers/pci/pci-driver.c

static const struct dev_pm_ops pci_dev_pm_ops = {
	...
        .suspend_noirq = pci_pm_suspend_noirq,
        .resume_noirq = pci_pm_resume_noirq,
        ...
};

Thanks, however, it is still not clear why this needs to be done during this phase. When you say PCIe subsystem driver, specifically which driver are you referring too? Are you referring to the pci_pm_suspend_noirq() in the drivers/pci/pci-driver.c driver? If so, just out of curiosity why does this need to be handled in the noirq phase?

Thanks
Jon

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

21.04.2020 17:40, Jon Hunter пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

Perhaps the RPM's -EACCES is returned from here:

https://elixir.free-electrons.com/linux/v5.7-rc2/source/drivers/base/power/runtime.c#L723

Which suggests that I2C is accessed after being suspended. I guess the PCIe driver suspends after the I2C and somehow my change affected the suspension order, although not sure how.

Jon, could you please try to enable PM logging and post the log? Please also post log of the working kernel version, so that we could compare the PM sequence.

Something like this should enable the logging: "echo 1 > /sys/power/pm_trace" + there is RPM tracing.

Quoted 21 lines

21.04.2020 17:40, Jon Hunter пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

Perhaps the RPM's -EACCES is returned from here:

https://elixir.free-electrons.com/linux/v5.7-rc2/source/drivers/base/power/runtime.c#L723

Which suggests that I2C is accessed after being suspended. I guess the PCIe driver suspends after the I2C and somehow my change affected the suspension order, although not sure how.

Quoted 6 lines

Jon, could you please try to enable PM logging and post the log? Please also post log of the working kernel version, so that we could compare the PM sequence.

Something like this should enable the logging: "echo 1 > /sys/power/pm_trace" + there is RPM tracing.

Unfortunately, after enabling that I don't any output and so no help there.

21.04.2020 22:42, Jon Hunter пишет:

Quoted 23 lines · Dmitry Osipenko

Quoted 21 lines

21.04.2020 17:40, Jon Hunter пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

Perhaps the RPM's -EACCES is returned from here:

https://elixir.free-electrons.com/linux/v5.7-rc2/source/drivers/base/power/runtime.c#L723

Which suggests that I2C is accessed after being suspended. I guess the PCIe driver suspends after the I2C and somehow my change affected the suspension order, although not sure how.

Quoted 8 lines

Quoted 6 lines

Jon, could you please try to enable PM logging and post the log? Please also post log of the working kernel version, so that we could compare the PM sequence.

Something like this should enable the logging: "echo 1 > /sys/power/pm_trace" + there is RPM tracing.

Unfortunately, after enabling that I don't any output and so no help there.

1. I now tried to check the pm_trace myself and found that it's available only on X86, my bad.

2. Jon, could you please clarify what exactly you were trying to test?

3. Is it only the Cardhu board that is affected by this problem?

4. Could you please try to add this to the kernel's cmdline and post the logs:

tp_printk trace_event=rpm_suspend,rpm_resume,rpm_usage,rpm_idle,rpm_return_int

Quoted 43 lines

21.04.2020 22:42, Jon Hunter пишет:

Quoted 23 lines · Dmitry Osipenko

Quoted 21 lines

21.04.2020 17:40, Jon Hunter пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

Perhaps the RPM's -EACCES is returned from here:

https://elixir.free-electrons.com/linux/v5.7-rc2/source/drivers/base/power/runtime.c#L723

Which suggests that I2C is accessed after being suspended. I guess the PCIe driver suspends after the I2C and somehow my change affected the suspension order, although not sure how.

Quoted 8 lines

Quoted 6 lines

Jon, could you please try to enable PM logging and post the log? Please also post log of the working kernel version, so that we could compare the PM sequence.

Something like this should enable the logging: "echo 1 > /sys/power/pm_trace" + there is RPM tracing.

Unfortunately, after enabling that I don't any output and so no help there.

1. I now tried to check the pm_trace myself and found that it's available only on X86, my bad.

2. Jon, could you please clarify what exactly you were trying to test?

3. Is it only the Cardhu board that is affected by this problem?

4. Could you please try to add this to the kernel's cmdline and post the logs:

Quoted 2 lines

tp_printk trace_event=rpm_suspend,rpm_resume,rpm_usage,rpm_idle,rpm_return_int

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out

[   59.549036] vdd_sata,avdd_plle: failed to disable

[   59.553778] Failed to disable avdd-plle: -110

[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

However, now with your patches the i2c is failing to resume and this breaks system suspend completely for Tegra30. So far this is the only board that appears to break.

So the above issue needs to be fixed and I will chat to Thierry about this.

Quoted 4 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

Quoted 7 lines

[   59.549036] vdd_sata,avdd_plle: failed to disable

[   59.553778] Failed to disable avdd-plle: -110

[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Quoted 5 lines

However, now with your patches the i2c is failing to resume and this breaks system suspend completely for Tegra30. So far this is the only board that appears to break.

So the above issue needs to be fixed and I will chat to Thierry about this.

Okay

...

Quoted 7 lines

Quoted 4 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Quoted 7 lines

Quoted 7 lines

[   59.549036] vdd_sata,avdd_plle: failed to disable

[   59.553778] Failed to disable avdd-plle: -110

[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Quoted 7 lines

Quoted 5 lines

However, now with your patches the i2c is failing to resume and this breaks system suspend completely for Tegra30. So far this is the only board that appears to break.

So the above issue needs to be fixed and I will chat to Thierry about this.

Okay

So I have been looking at this some more and this starting to all look a bit of a mess :-(

Firstly, the Tegra PCI driver suspends late (noirq) as we know. The PCI driver will warn if it cannot disable the regulators when suspending but does not actually fail suspend. So this warning is just indicating that we were unable to disable the regulators.

Now I don't see that we can ever disable the PCI regulators currently when entering suspend because ...

1. We are in the noirq phase and so we will not get the completion interrupt for the I2C transfer. I know that you recently added the atomic I2C transfer support, but we can get the regulator framework to use this (I have not looked in much detail so far).

2. Even if the regulator framework supported atomic I2C transfers, we also have the problem that the I2C controller could be runtime- suspended and pm_runtime_get_sync() will not work during during this phase to resume it correctly. This is a problem that needs to be fixed indeed!

3. Then we also have the possible dependency on the DMA driver that is suspended during the noirq phase.

It could be argued that if the PCI regulators are never turned off (currently) then we should not even bother and this will likely resolve this for now. However, really we should try to fix that correctly.

What I still don't understand is why your patch breaks resume. Even if the I2C transfer fails, and is deemed harmless by the client driver, we should still be able to suspend and resume correctly.

23.04.2020 13:56, Jon Hunter пишет:

Quoted 38 lines · Dmitry Osipenko

...

Quoted 7 lines

Quoted 4 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Quoted 7 lines

Quoted 7 lines

[   59.549036] vdd_sata,avdd_plle: failed to disable

[   59.553778] Failed to disable avdd-plle: -110

[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Quoted 7 lines

Quoted 5 lines

However, now with your patches the i2c is failing to resume and this breaks system suspend completely for Tegra30. So far this is the only board that appears to break.

So the above issue needs to be fixed and I will chat to Thierry about this.

Okay

So I have been looking at this some more and this starting to all look a bit of a mess :-(

Firstly, the Tegra PCI driver suspends late (noirq) as we know. The PCI driver will warn if it cannot disable the regulators when suspending but does not actually fail suspend. So this warning is just indicating that we were unable to disable the regulators.

Quoted 7 lines

Now I don't see that we can ever disable the PCI regulators currently when entering suspend because ...

1. We are in the noirq phase and so we will not get the completion interrupt for the I2C transfer. I know that you recently added the atomic I2C transfer support, but we can get the regulator framework to use this (I have not looked in much detail so far).

That's not good :) I didn't realize that *all* interrupts of every device are disabled before .noirq is invoked. It appeared to me that the IRQs disabling and .noirq invocation is performed for the drivers one after another, but now I see that it's not true.

https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/base/power/main.c#L1446

Quoted 5 lines

2. Even if the regulator framework supported atomic I2C transfers, we also have the problem that the I2C controller could be runtime- suspended and pm_runtime_get_sync() will not work during during this phase to resume it correctly. This is a problem that needs to be fixed indeed!

Could you please clarify why pm_runtime_get_sync() can't be used by the I2C driver's in NOIRQ phase?

Quoted 2 lines

3. Then we also have the possible dependency on the DMA driver that is suspended during the noirq phase.

Yes, this is correct.

Again, some regulator drivers may do something on suspend too, although looks like the current upstream Tegra devices are not affected by this potential problem.

Quoted 3 lines

It could be argued that if the PCI regulators are never turned off (currently) then we should not even bother and this will likely resolve this for now. However, really we should try to fix that correctly.

Yes, keeping PCI regulators always-enabled should be a good immediate solution.

Also, the RPM's system suspend/resume needs to fixed for the pci-tegra driver, like I already suggested before.

Quoted 3 lines

What I still don't understand is why your patch breaks resume. Even if the I2C transfer fails, and is deemed harmless by the client driver, we should still be able to suspend and resume correctly.

If DMA is getting synchronized after DMA driver being suspended, then it could be a problem.

Could you please try to apply this hunk and see if it makes any difference (I'll probably make it as proper patch):

It also could be that there is more than the suspend ordering problem, but for now it is hard to tell without having a detailed log which includes I2C/DMA/RPM traces.

Quoted 2 lines

Could you please clarify why pm_runtime_get_sync() can't be used by the I2C driver's in NOIRQ phase?

Yes take a look at commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2)").

Quoted 2 lines

Yes, keeping PCI regulators always-enabled should be a good immediate solution.

I was thinking about that, and I am not sure it is. I don't think that the failure to send the I2C command should break suspend.

So I confirmed that DMA is not the issue in this case. I tested this by ensuring that DMA is never used. However, it is a potential problem indeed.

Quoted 2 lines

Could you please try to apply this hunk and see if it makes any difference (I'll probably make it as proper patch):

Per my tests, I don't believe that it will as disabling DMA does not resolve the problem.

Quoted 3 lines

It also could be that there is more than the suspend ordering problem, but for now it is hard to tell without having a detailed log which includes I2C/DMA/RPM traces.

I have taken a look and I don't see any issues with ordering. I2C is suspended after PCI. This did not change.

24.04.2020 10:10, Jon Hunter пишет:
...

Quoted 5 lines

Quoted 2 lines

Could you please clarify why pm_runtime_get_sync() can't be used by the I2C driver's in NOIRQ phase?

Yes take a look at commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2)").

I2C driver now uses irq-safe RPM since ede2299f7 ("i2c: tegra: Support atomic transfers"), and thus, the RPM's workqueue shouldn't be a problem. I guess RPM should work fine in this case, don't you think so?

Quoted 5 lines

Quoted 2 lines

Yes, keeping PCI regulators always-enabled should be a good immediate solution.

I was thinking about that, and I am not sure it is. I don't think that the failure to send the I2C command should break suspend.

It shouldn't, but looks like it should be a separate problem.

Quoted 14 lines

So I confirmed that DMA is not the issue in this case. I tested this by ensuring that DMA is never used. However, it is a potential problem indeed.

Quoted 2 lines

Could you please try to apply this hunk and see if it makes any difference (I'll probably make it as proper patch):

Per my tests, I don't believe that it will as disabling DMA does not resolve the problem.

Quoted 3 lines

It also could be that there is more than the suspend ordering problem, but for now it is hard to tell without having a detailed log which includes I2C/DMA/RPM traces.

Quoted 2 lines

I have taken a look and I don't see any issues with ordering. I2C is suspended after PCI. This did not change.

Do you see a "completion done after timeout" messages in the KMSG log of the v5.6 kernel?

Could you please try this hunk? Although, I'll be surprised if it changes anything.

Quoted 11 lines · Dmitry Osipenko

24.04.2020 10:10, Jon Hunter пишет:
...

Quoted 5 lines

Quoted 2 lines

Could you please clarify why pm_runtime_get_sync() can't be used by the I2C driver's in NOIRQ phase?

Yes take a look at commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2)").

I2C driver now uses irq-safe RPM since ede2299f7 ("i2c: tegra: Support atomic transfers"), and thus, the RPM's workqueue shouldn't be a problem. I guess RPM should work fine in this case, don't you think so?

I was testing, and I did not see it using atomic transfers. I can confirm if the RPM callbacks are called or not, but I did not think so. However, let me confirm.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Yes, keeping PCI regulators always-enabled should be a good immediate solution.

Quoted 4 lines

Quoted 2 lines

I was thinking about that, and I am not sure it is. I don't think that the failure to send the I2C command should break suspend.

It shouldn't, but looks like it should be a separate problem.

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination a900aeac2537 i2c: tegra: Better handle case where CPU0 is busy for a long time

Quoted 4 lines

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination

Quoted 1 line

a900aeac2537 i2c: tegra: Better handle case where CPU0 is busy for a long time

Sure, will do!

27.04.2020 11:44, Wolfram Sang пишет:

Quoted 4 lines

Quoted 4 lines

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination

This patch has nothing to do with your trouble, why do you want to revert it?

Quoted 8 lines · Dmitry Osipenko

27.04.2020 11:44, Wolfram Sang пишет:

Quoted 4 lines

Quoted 4 lines

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination

This patch has nothing to do with your trouble, why do you want to revert it?

I'll wait some more before pushing out, so you can discuss it.

Quoted 7 lines · Wolfram Sang

Quoted 6 lines · Dmitry Osipenko

27.04.2020 11:44, Wolfram Sang пишет:

Quoted 4 lines

Quoted 4 lines

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination

Quoted 4 lines

Quoted 2 lines

This patch has nothing to do with your trouble, why do you want to revert it?

I'll wait some more before pushing out, so you can discuss it.

Okay, let me run a quick test with that second patch still applied to make sure it really is harmless.

Quoted 8 lines · Wolfram Sang

Quoted 7 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 11:44, Wolfram Sang пишет:

Quoted 4 lines

Quoted 4 lines

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

This patch has nothing to do with your trouble, why do you want to revert it?

I'll wait some more before pushing out, so you can discuss it.

Okay, let me run a quick test with that second patch still applied to make sure it really is harmless.

Alright, I tested v5.7-rc3 with this patch reverted:

a900aeac2537 i2c: tegra: Better handle case where CPU0 is busy for a long time

and the results came back positive, so I think we can leave patch:

8814044fe0fa i2c: tegra: Synchronize DMA before termination

in. But then again, I see that Dmitry posted this yesterday:

	https://lkml.org/lkml/2020/4/26/481

which seems like it would be related to this and potentially be a follow-up fix for some corner cases?

So I'm not sure how well this whole set has been tested yet.

Maybe a better solution would be for the DMA synchronization patch to go into the 5.8 queue instead to make sure we get more testing cycles.

27.04.2020 11:44, Wolfram Sang пишет:

Quoted 4 lines

Quoted 4 lines

Wolfram, can you revert the following two patches for v5.7, please?

8814044fe0fa i2c: tegra: Synchronize DMA before termination

This patch has nothing to do with your trouble, why do you want to revert it?

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

From what I understood, there were (at least) two regressions reported. So, to me, it makes sense to revert the change, so for upstream users everything stays "the same". Of course, this does not mean it should stay like this forever and you guys can work on fixing the root causes. I'll happily apply them for this release when you are confident with the results.

27.04.2020 13:38, Wolfram Sang пишет:

Quoted 16 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

Quoted 7 lines

From what I understood, there were (at least) two regressions reported. So, to me, it makes sense to revert the change, so for upstream users everything stays "the same". Of course, this does not mean it should stay like this forever and you guys can work on fixing the root causes. I'll happily apply them for this release when you are confident with the results.

For now it's a single regression in the PCIe driver and it's actually not a regression, but a PCIe driver bug that needs to be fixed. The I2C part should be okay.

By reverting the I2C patch, we're back to the PCIe bug being papered over and I don't like this. Let's just fix the PCIe driver and the problem is gone.. it needs to be fixed anyways.

Yes, that bug should be fixed anyway. But that doesn't justify breaking suspend/resume completely, which *is* a regression.

Look, I'm not saying that we should drop this patch altogether. All I'm saying is that we should postpone it so that we can: a) get suspend and resume working again (and by doing so make sure no other suspend/resume regressions silently creep in, because that always seems to happen when you're not looking) and b) fix any preexisting issues without possibly scrambling the result with this perhaps unrelated fix.

So, again, I think the safest road forward is to back this one out for now, fix whatever this other bug is and once suspend/resume is working properly again we can revisit this patch based on a known-good baseline.

Quoted 11 lines

Yes, that bug should be fixed anyway. But that doesn't justify breaking suspend/resume completely, which *is* a regression.

Look, I'm not saying that we should drop this patch altogether. All I'm saying is that we should postpone it so that we can: a) get suspend and resume working again (and by doing so make sure no other suspend/resume regressions silently creep in, because that always seems to happen when you're not looking) and b) fix any preexisting issues without possibly scrambling the result with this perhaps unrelated fix.

Quoted 3 lines

So, again, I think the safest road forward is to back this one out for now, fix whatever this other bug is and once suspend/resume is working properly again we can revisit this patch based on a known-good baseline.

I am with you here. I want to add that the proper fix should be developed without thinking too much about stable in the first place. *When* we have a proper working fix, then we can think about making it "more" suitable for backporting. Yet, it may also be a result that older kernels need a different solution. Or have no solution at all, in case they can't do atomic_transfers and this is needed.

D'accord?

I'll try not to forget about these patches to resubmit them later on, once the problem will be resolved :)

27.04.2020 18:31, Wolfram Sang пишет:

Quoted 22 lines

Quoted 11 lines

Yes, that bug should be fixed anyway. But that doesn't justify breaking suspend/resume completely, which *is* a regression.

Look, I'm not saying that we should drop this patch altogether. All I'm saying is that we should postpone it so that we can: a) get suspend and resume working again (and by doing so make sure no other suspend/resume regressions silently creep in, because that always seems to happen when you're not looking) and b) fix any preexisting issues without possibly scrambling the result with this perhaps unrelated fix.

Quoted 3 lines

So, again, I think the safest road forward is to back this one out for now, fix whatever this other bug is and once suspend/resume is working properly again we can revisit this patch based on a known-good baseline.

I am with you here. I want to add that the proper fix should be developed without thinking too much about stable in the first place. *When* we have a proper working fix, then we can think about making it "more" suitable for backporting. Yet, it may also be a result that older kernels need a different solution. Or have no solution at all, in case they can't do atomic_transfers and this is needed.

Quoted 2 lines

D'accord?

I saw that you submitted the revert of the patches for 5.7, hopefully it won't result in putting the PCIe driver problem into the back burner. I'll try not to forget about these patches to resubmit them later on, once the problem will be resolved :)

Quoted 23 lines

27.04.2020 18:31, Wolfram Sang пишет:

Quoted 22 lines

Quoted 11 lines

Yes, that bug should be fixed anyway. But that doesn't justify breaking suspend/resume completely, which *is* a regression.

Look, I'm not saying that we should drop this patch altogether. All I'm saying is that we should postpone it so that we can: a) get suspend and resume working again (and by doing so make sure no other suspend/resume regressions silently creep in, because that always seems to happen when you're not looking) and b) fix any preexisting issues without possibly scrambling the result with this perhaps unrelated fix.

Quoted 3 lines

So, again, I think the safest road forward is to back this one out for now, fix whatever this other bug is and once suspend/resume is working properly again we can revisit this patch based on a known-good baseline.

I am with you here. I want to add that the proper fix should be developed without thinking too much about stable in the first place. *When* we have a proper working fix, then we can think about making it "more" suitable for backporting. Yet, it may also be a result that older kernels need a different solution. Or have no solution at all, in case they can't do atomic_transfers and this is needed.

Quoted 7 lines

Quoted 2 lines

D'accord?

I saw that you submitted the revert of the patches for 5.7, hopefully it won't result in putting the PCIe driver problem into the back burner. I'll try not to forget about these patches to resubmit them later on, once the problem will be resolved :)

I can put these two patches into a local development branch to keep track of them. From what I said earlier, it looks like it would be fine to apply these if we also make that runtime PM change (i.e. drop force runtime PM and instead manually invoke runtime PM callbacks, which seems to be in line with what the PM maintainers suggest, as pointed out elsewhere in this thread).

How about if I put all of that into a branch and push it to linux-next so that we can get some broader testing? I've already run it through our internal test system, which, while not perfect, is the broadest system I am aware of, and all tests came back positive.

I'm not exactly sure I see a real issue with the PCIe driver after those patches are applied. The regulator errors are gone (presumably because the regulators now do get turned off properly) and I don't observe any other issues.

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

So, sorry, I disagree. I don't think we have "plenty of time".

27.04.2020 14:00, Thierry Reding пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

Quoted 1 line

So, sorry, I disagree. I don't think we have "plenty of time".

There is about a month now before the 5.7 release. It's a bit too early to start the panic, IMO :)

Jon already proposed a reasonable simple solution: to keep PCIe regulators always-ON. In a longer run we may want to have I2C atomic transfers supported for a late suspend phase.

Quoted 33 lines

27.04.2020 14:00, Thierry Reding пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

Quoted 4 lines

Quoted 1 line

So, sorry, I disagree. I don't think we have "plenty of time".

There is about a month now before the 5.7 release. It's a bit too early to start the panic, IMO :)

There's no panic. A patch got merged and it broken something, so we revert it and try again. It's very much standard procedure.

Quoted 3 lines

Jon already proposed a reasonable simple solution: to keep PCIe regulators always-ON. In a longer run we may want to have I2C atomic transfers supported for a late suspend phase.

That's not really a solution, though, is it? It's just papering over an issue that this patch introduced or uncovered. I'm much more in favour of fixing problems at the root rather than keep papering over until we loose track of what the actual problems are.

27.04.2020 18:12, Thierry Reding пишет:

Quoted 42 lines · Dmitry Osipenko

Quoted 33 lines

27.04.2020 14:00, Thierry Reding пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

Quoted 4 lines

Quoted 1 line

So, sorry, I disagree. I don't think we have "plenty of time".

There is about a month now before the 5.7 release. It's a bit too early to start the panic, IMO :)

There's no panic. A patch got merged and it broken something, so we revert it and try again. It's very much standard procedure.

Quoted 8 lines

Quoted 3 lines

Jon already proposed a reasonable simple solution: to keep PCIe regulators always-ON. In a longer run we may want to have I2C atomic transfers supported for a late suspend phase.

That's not really a solution, though, is it? It's just papering over an issue that this patch introduced or uncovered. I'm much more in favour of fixing problems at the root rather than keep papering over until we loose track of what the actual problems are.

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Quoted 52 lines

27.04.2020 18:12, Thierry Reding пишет:

Quoted 42 lines · Dmitry Osipenko

Quoted 33 lines

27.04.2020 14:00, Thierry Reding пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

Quoted 4 lines

Quoted 1 line

So, sorry, I disagree. I don't think we have "plenty of time".

There is about a month now before the 5.7 release. It's a bit too early to start the panic, IMO :)

There's no panic. A patch got merged and it broken something, so we revert it and try again. It's very much standard procedure.

Quoted 8 lines

Quoted 3 lines

Jon already proposed a reasonable simple solution: to keep PCIe regulators always-ON. In a longer run we may want to have I2C atomic transfers supported for a late suspend phase.

That's not really a solution, though, is it? It's just papering over an issue that this patch introduced or uncovered. I'm much more in favour of fixing problems at the root rather than keep papering over until we loose track of what the actual problems are.

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

There are a few issues here, but the issue Thierry and I are referring to is the regression introduced by this change. Yes this exposes other problems, but we first need to understand why this breaks resume in general, regardless of what the PCIe driver is doing. I will look at this a bit more later this week.

27.04.2020 18:12, Thierry Reding пишет:

Quoted 42 lines · Dmitry Osipenko

Quoted 33 lines

27.04.2020 14:00, Thierry Reding пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

Quoted 4 lines

Quoted 1 line

So, sorry, I disagree. I don't think we have "plenty of time".

There is about a month now before the 5.7 release. It's a bit too early to start the panic, IMO :)

There's no panic. A patch got merged and it broken something, so we revert it and try again. It's very much standard procedure.

Quoted 8 lines

Quoted 3 lines

Jon already proposed a reasonable simple solution: to keep PCIe regulators always-ON. In a longer run we may want to have I2C atomic transfers supported for a late suspend phase.

That's not really a solution, though, is it? It's just papering over an issue that this patch introduced or uncovered. I'm much more in favour of fixing problems at the root rather than keep papering over until we loose track of what the actual problems are.

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Quoted 52 lines · Dmitry Osipenko

27.04.2020 18:12, Thierry Reding пишет:

Quoted 42 lines · Dmitry Osipenko

Quoted 33 lines

27.04.2020 14:00, Thierry Reding пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 6 lines

27.04.2020 10:48, Thierry Reding пишет:
...

Quoted 4 lines

Quoted 3 lines

Maybe but all these other problems appear to have existed for sometime now. We need to fix all, but for the moment we need to figure out what's best for v5.7.

Quoted 8 lines

Quoted 5 lines

To me it doesn't sound like we have a good handle on what exactly is going on here and we're mostly just poking around.

And even if things weren't working quite properly before, it sounds to me like this patch actually made things worse.

There is a plenty of time to work on the proper fix now. To me it sounds like you're giving up on fixing the root of the problem, sorry.

We're at -rc3 now and I haven't seen any promising progress in the last week. All the while suspend/resume is now broken on at least one board and that may end up hiding any other issues that could creep in in the meantime.

Furthermore we seem to have a preexisting issue that may very well interfere with this patch, so I think the cautious thing is to revert for now and then fix the original issue first. We can always come back to this once everything is back to normal.

Also, people are now looking at backporting this to v5.6. Unless we revert this from v5.7 it may get picked up for backports to other kernels and then I have to notify stable kernel maintainers that they shouldn't and they have to back things out again. That's going to cause a lot of wasted time for a lot of people.

Quoted 4 lines

Quoted 1 line

So, sorry, I disagree. I don't think we have "plenty of time".

There is about a month now before the 5.7 release. It's a bit too early to start the panic, IMO :)

There's no panic. A patch got merged and it broken something, so we revert it and try again. It's very much standard procedure.

Quoted 8 lines

Quoted 3 lines

Jon already proposed a reasonable simple solution: to keep PCIe regulators always-ON. In a longer run we may want to have I2C atomic transfers supported for a late suspend phase.

That's not really a solution, though, is it? It's just papering over an issue that this patch introduced or uncovered. I'm much more in favour of fixing problems at the root rather than keep papering over until we loose track of what the actual problems are.

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Quoted 17 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

Quoted 2 lines

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 20 lines

Quoted 19 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/20191213134417.222720-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Quoted 3 lines

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

Quoted 26 lines

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 23 lines

Quoted 12 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/20191213134417.222720-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

Quoted 7 lines

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

Quoted 3 lines

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

From reading the changelog for commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2))", this is the expected behaviour for runtime resume in the noirq phase.

29.04.2020 16:57, Jon Hunter пишет:

Quoted 32 lines · Dmitry Osipenko

Quoted 29 lines

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/20191213134417.222720-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

Quoted 6 lines

Quoted 2 lines

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

Quoted 3 lines

From reading the changelog for commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2))", this is the expected behaviour for runtime resume in the noirq phase.

I'm curious whether there is a way to tell RPM that it's okay to do it for a particular device, like I2C that uses IRQ-safe RPM + doesn't have parent devices that need to be resumed.

Quoted 37 lines

29.04.2020 16:57, Jon Hunter пишет:

Quoted 35 lines · Dmitry Osipenko

Quoted 22 lines

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 20 lines

Quoted 19 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/20191213134417.222720-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Quoted 7 lines

Quoted 3 lines

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

From reading the changelog for commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2))", this is the expected behaviour for runtime resume in the noirq phase.

Quoted 3 lines

I'm curious whether there is a way to tell RPM that it's okay to do it for a particular device, like I2C that uses IRQ-safe RPM + doesn't have parent devices that need to be resumed.

Been there, done that:

	http://patchwork.ozlabs.org/project/linux-tegra/patch/20191128160314.2381249-2-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

Quoted 6 lines

Quoted 2 lines

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

Anyways, again, today it's wrong to use I2C in the NOIRQ phase becasue I2C interrupt is disabled. It's the PCIe driver that should be fixed.

Quoted 30 lines

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/20191213134417.222720-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

Quoted 6 lines

Quoted 2 lines

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

Quoted 2 lines

Anyways, again, today it's wrong to use I2C in the NOIRQ phase becasue I2C interrupt is disabled. It's the PCIe driver that should be fixed.

I don't think so. Everything works perfectly fine if we fix system suspend/resume not to rely on pm_runtime_force_{suspend,resume}() and directly call the runtime suspend/resume implementations.

So can we please stop deflecting and fix the damn I2C driver. From my perspective we have two choices:

1) do what I suggested above and revert the force suspend/resume patch and apply the "manual" suspend/resume patch instead

2) revert this patch and go back to the drawing board

I suspect that with 2) we'd end up back where we started and have to do
1) anyway.

An alternative that I'd prefer even more would be to do 2) now for v5.7 and then we do 1) for v5.8 and give this some more soaking time.

29.04.2020 19:30, Thierry Reding пишет:

Quoted 31 lines · Dmitry Osipenko

Quoted 30 lines

29.04.2020 11:55, Thierry Reding пишет:
...

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

It's not "papering over an issue". The bug can't be fixed properly without introducing I2C atomic transfers support for a late suspend phase, I don't see any other solutions for now. Stable kernels do not support atomic transfers at all, that proper solution won't be backportable.

Hm... on a hunch I tried something and, lo and behold, it worked. I can get Cardhu to properly suspend/resume on top of v5.7-rc3 with the following sequence:

	revert 9f42de8d4ec2 i2c: tegra: Fix suspending in active runtime PM state
	apply http://patchwork.ozlabs.org/project/linux-tegra/patch/20191213134417.222720-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/

I also ran that through our test farm and I don't see any other issues. At the time I was already skeptical about pm_runtime_force_suspend() and pm_runtime_force_resume() and while I'm not fully certain why exactly it doesn't work, the above on top of v5.7-rc3 seems like a good option.

Quoted 6 lines

Quoted 2 lines

I'll try to do some digging if I can find out why exactly force suspend and resume doesn't work.

Ah... so it looks like pm_runtime_force_resume() never actually does anything in this case and then disable_depth remains at 1 and the first tegra_i2c_xfer() will then fail to runtime resume the controller.

That's the exactly expected behaviour of the RPM force suspend/resume. The only unexpected part for me is that the tegra_i2c_xfer() runtime resume then fails in the NOIRQ phase.

Quoted 6 lines

Quoted 2 lines

Anyways, again, today it's wrong to use I2C in the NOIRQ phase becasue I2C interrupt is disabled. It's the PCIe driver that should be fixed.

I don't think so. Everything works perfectly fine if we fix system suspend/resume not to rely on pm_runtime_force_{suspend,resume}() and directly call the runtime suspend/resume implementations.

It should "work" only in conjunction with my I2C patch, otherwise you'll get a spurious I2C timeout error. And it will "work" only because interrupt is handled manually after the timeout, meaning that yours suspending time will take few hundreds ms more.

Quoted 11 lines

So can we please stop deflecting and fix the damn I2C driver. From my perspective we have two choices:

1) do what I suggested above and revert the force suspend/resume patch and apply the "manual" suspend/resume patch instead

2) revert this patch and go back to the drawing board

I suspect that with 2) we'd end up back where we started and have to do
1) anyway.

Quoted 2 lines

An alternative that I'd prefer even more would be to do 2) now for v5.7 and then we do 1) for v5.8 and give this some more soaking time.

I2C driver isn't broken, PCIe driver is. IMO.

Both yours variants are not going to be a backportable fix for the stable kernels, they won't fix the suspended interrupt problem. What I'm missing?

Quoted 9 lines

Quoted 7 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
[   59.549036] vdd_sata,avdd_plle: failed to disable
[   59.553778] Failed to disable avdd-plle: -110
[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

23.04.2020 13:56, Jon Hunter пишет:

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
[   59.549036] vdd_sata,avdd_plle: failed to disable
[   59.553778] Failed to disable avdd-plle: -110
[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Jon, could you please confirm that you're seeing those regulator-disable errors with my patch? I don't see those errors in yours original log [1].

[1]
https://lore.kernel.org/lkml/1e259e22-c300-663a-e537-18d854e0f478-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org/

Again, could you please post the *full* logs?

If regulator's disabling was "failing" before without my patch because of the I2C interrupt being force-disabled during of NOIRQ phase, and now regulator's disabling succeeds with my patch because IRQ is manually handled after the timeout, then this could be bad. It means that regulator was actually getting disabled, but I2C driver was timing out because interrupt couldn't be handled in NOIRQ phase, which should result in a dead PCIe on a resume from suspend since regulator's core thinks that regulator is enabled (I2C said it failed to disable), while it is actually disabled.

Do you have anything plugged into the PCIe slot in yours testing farm? It wouldn't surprise me if the plugged card isn't functional after resume from suspend on a stable kernels.

27.04.2020 15:46, Dmitry Osipenko пишет:

Quoted 31 lines

23.04.2020 13:56, Jon Hunter пишет:

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
[   59.549036] vdd_sata,avdd_plle: failed to disable
[   59.553778] Failed to disable avdd-plle: -110
[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Jon, could you please confirm that you're seeing those regulator-disable errors with my patch? I don't see those errors in yours original log [1].

[1]
https://lore.kernel.org/lkml/1e259e22-c300-663a-e537-18d854e0f478-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org/

Again, could you please post the *full* logs?

If regulator's disabling was "failing" before without my patch because of the I2C interrupt being force-disabled during of NOIRQ phase, and now regulator's disabling succeeds with my patch because IRQ is manually handled after the timeout, then this could be bad. It means that regulator was actually getting disabled, but I2C driver was timing out because interrupt couldn't be handled in NOIRQ phase, which should result in a dead PCIe on a resume from suspend since regulator's core thinks that regulator is enabled (I2C said it failed to disable), while it is actually disabled.

Quoted 4 lines

Do you have anything plugged into the PCIe slot in yours testing farm? It wouldn't surprise me if the plugged card isn't functional after resume from suspend on a stable kernels.

I actually now see that interrupt is not allowed to be enabled during the NOIRQ phase:

https://elixir.bootlin.com/linux/v5.7-rc3/source/kernel/irq/manage.c#L640

it should be worthwhile to turn it into a WARN_ON.

27.04.2020 17:13, Dmitry Osipenko пишет:

Quoted 40 lines

27.04.2020 15:46, Dmitry Osipenko пишет:

Quoted 31 lines

23.04.2020 13:56, Jon Hunter пишет:

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
[   59.549036] vdd_sata,avdd_plle: failed to disable
[   59.553778] Failed to disable avdd-plle: -110
[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Jon, could you please confirm that you're seeing those regulator-disable errors with my patch? I don't see those errors in yours original log [1].

[1]
https://lore.kernel.org/lkml/1e259e22-c300-663a-e537-18d854e0f478-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org/

Again, could you please post the *full* logs?

If regulator's disabling was "failing" before without my patch because of the I2C interrupt being force-disabled during of NOIRQ phase, and now regulator's disabling succeeds with my patch because IRQ is manually handled after the timeout, then this could be bad. It means that regulator was actually getting disabled, but I2C driver was timing out because interrupt couldn't be handled in NOIRQ phase, which should result in a dead PCIe on a resume from suspend since regulator's core thinks that regulator is enabled (I2C said it failed to disable), while it is actually disabled.

Quoted 4 lines

Do you have anything plugged into the PCIe slot in yours testing farm? It wouldn't surprise me if the plugged card isn't functional after resume from suspend on a stable kernels.

I actually now see that interrupt is not allowed to be enabled during the NOIRQ phase:

Quoted 4 lines

https://elixir.bootlin.com/linux/v5.7-rc3/source/kernel/irq/manage.c#L640

it should be worthwhile to turn it into a WARN_ON.

Oh, wait! There is already a warning there.. hmm.

27.04.2020 17:45, Dmitry Osipenko пишет:

Quoted 41 lines

27.04.2020 17:13, Dmitry Osipenko пишет:

Quoted 40 lines

27.04.2020 15:46, Dmitry Osipenko пишет:

Quoted 31 lines

23.04.2020 13:56, Jon Hunter пишет:

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
[   59.549036] vdd_sata,avdd_plle: failed to disable
[   59.553778] Failed to disable avdd-plle: -110
[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Jon, could you please confirm that you're seeing those regulator-disable errors with my patch? I don't see those errors in yours original log [1].

[1]
https://lore.kernel.org/lkml/[email protected]/

Again, could you please post the *full* logs?

If regulator's disabling was "failing" before without my patch because of the I2C interrupt being force-disabled during of NOIRQ phase, and now regulator's disabling succeeds with my patch because IRQ is manually handled after the timeout, then this could be bad. It means that regulator was actually getting disabled, but I2C driver was timing out because interrupt couldn't be handled in NOIRQ phase, which should result in a dead PCIe on a resume from suspend since regulator's core thinks that regulator is enabled (I2C said it failed to disable), while it is actually disabled.

Quoted 4 lines

Do you have anything plugged into the PCIe slot in yours testing farm? It wouldn't surprise me if the plugged card isn't functional after resume from suspend on a stable kernels.

I actually now see that interrupt is not allowed to be enabled during the NOIRQ phase:

Quoted 7 lines

Quoted 4 lines

https://elixir.bootlin.com/linux/v5.7-rc3/source/kernel/irq/manage.c#L640

it should be worthwhile to turn it into a WARN_ON.

Oh, wait! There is already a warning there.. hmm.

Aha, the disable depth for the I2C interrupt is 2 after suspend_device_irq(), that's why there is no warning.

This should catch the bug and trigger the warning:

--- >8 ---

kernel/irq/manage.c +2 -1

index 453a8a0f4804..fe25104d8b22 100644
@@ -653,6 +653,8 @@ void __enable_irq(struct irq_desc *desc)
 		break;
 	}
 	default:
+		if (desc->istate & IRQS_SUSPENDED)
+			goto err_out;
 		desc->depth--;
 	}
 }
--- >8 ---

Jon could you please give it a try? Will this change produce a warning for the I2C driver on a PCIe suspend for the v5.6 kernel?

Quoted 71 lines

27.04.2020 17:45, Dmitry Osipenko пишет:

Quoted 41 lines

27.04.2020 17:13, Dmitry Osipenko пишет:

Quoted 40 lines

27.04.2020 15:46, Dmitry Osipenko пишет:

Quoted 31 lines

23.04.2020 13:56, Jon Hunter пишет:

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

So I think that part of the problem already existed prior to these patches. Without your patches I see ...

[   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
[   59.549036] vdd_sata,avdd_plle: failed to disable
[   59.553778] Failed to disable avdd-plle: -110
[   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110

Does this I2C timeout happen with my patches? Could you please post full logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

Jon, could you please confirm that you're seeing those regulator-disable errors with my patch? I don't see those errors in yours original log [1].

[1]
https://lore.kernel.org/lkml/1e259e22-c300-663a-e537-18d854e0f478-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org/

Again, could you please post the *full* logs?

If regulator's disabling was "failing" before without my patch because of the I2C interrupt being force-disabled during of NOIRQ phase, and now regulator's disabling succeeds with my patch because IRQ is manually handled after the timeout, then this could be bad. It means that regulator was actually getting disabled, but I2C driver was timing out because interrupt couldn't be handled in NOIRQ phase, which should result in a dead PCIe on a resume from suspend since regulator's core thinks that regulator is enabled (I2C said it failed to disable), while it is actually disabled.

Quoted 4 lines

Do you have anything plugged into the PCIe slot in yours testing farm? It wouldn't surprise me if the plugged card isn't functional after resume from suspend on a stable kernels.

I actually now see that interrupt is not allowed to be enabled during the NOIRQ phase:

Quoted 7 lines

Quoted 4 lines

https://elixir.bootlin.com/linux/v5.7-rc3/source/kernel/irq/manage.c#L640

it should be worthwhile to turn it into a WARN_ON.

Oh, wait! There is already a warning there.. hmm.

Aha, the disable depth for the I2C interrupt is 2 after suspend_device_irq(), that's why there is no warning.

This should catch the bug and trigger the warning:

--- >8 ---

kernel/irq/manage.c +2 -1

index 453a8a0f4804..fe25104d8b22 100644
@@ -653,6 +653,8 @@ void __enable_irq(struct irq_desc *desc)
 		break;
 	}
 	default:
+		if (desc->istate & IRQS_SUSPENDED)
+			goto err_out;
 		desc->depth--;
 	}
 }
--- >8 ---

Quoted 2 lines

Jon could you please give it a try? Will this change produce a warning for the I2C driver on a PCIe suspend for the v5.6 kernel?

Yes I can test, but I still want to know why resume is currently broken.

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

21.04.2020 17:40, Jon Hunter пишет:

Quoted 10 lines · Dmitry Osipenko

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

Quoted 1 line

I gave it a try but suspend still fails.

Is this regulator error gone with my changes?

[   60.450346] WARNING: CPU: 0 PID: 653 at
/home/jonathanh/workdir/tegra/mlt-linux_next/kernel/drivers/regulator/core.c:2603
_regulator_disable+0xb8/0x1b4
[   60.463959] unbalanced disables for vdd_pexa,vdd_pexb

Quoted 13 lines

21.04.2020 17:40, Jon Hunter пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 7 lines

21.04.2020 12:49, Jon Hunter пишет:
...

Quoted 2 lines

I can try the above, but I agree it would be best to avoid messing with the suspend levels if possible.

Will be awesome if you could try it and report back the result.

I gave it a try but suspend still fails.

Quoted 6 lines

Is this regulator error gone with my changes?

[   60.450346] WARNING: CPU: 0 PID: 653 at
/home/jonathanh/workdir/tegra/mlt-linux_next/kernel/drivers/regulator/core.c:2603
_regulator_disable+0xb8/0x1b4
[   60.463959] unbalanced disables for vdd_pexa,vdd_pexb

The above is still there with your changes.

Hi Dmitry,

Quoted 11 lines · Dmitry Osipenko

Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

I have noticed a regression on tegra30-cardhu-a04 when testing system suspend. Git bisect is pointing to this commit and reverting it fixes the problem. In the below console log I2C fails to resume ...

Quoted 1 line

[   40.888512] usb1_vbus: supplied by 5v0

[   40.892408] vddio_sdmmc,avdd_vdac: supplied by 5v0
[   40.897401] cam_1v8: disabling
[   40.900548] modem_3v3: disabling
[   40.903875] vdd_cam1_ldo: disabling
[   40.907501] vdd_cam2_ldo: disabling
[   40.911092] vdd_cam3_ldo: disabling
[   40.914714] vdd_fuse_3v3: disabling
[   40.918305] vddio_vid: disabling
[   40.921623] usb1_vbus: disabling
[   59.445032] PM: suspend entry (deep)
[   59.448852] Filesystems sync: 0.000 seconds
[   59.456161] Freezing user space processes ... (elapsed 0.001 seconds) done.
[   59.457645] OOM killer disabled.
[   59.457649] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.

[   59.764926] Disabling non-boot CPUs ...
[   59.769540] IRQ 18: no longer affine to CPU1
[   59.789070] IRQ 19: no longer affine to CPU2
[   59.808049] IRQ 20: no longer affine to CPU3
[   59.827113] Entering suspend state LP1
[   59.827163] Enabling non-boot CPUs ...
[   59.834797] CPU1 is up
[   59.840943] CPU2 is up
[   59.847378] CPU3 is up
[   59.850577] tegra-i2c 7000d000.i2c: runtime resume failed -13
[   59.856432] tegra-i2c 7000d000.i2c: runtime resume failed -13

[   59.862231] tegra-i2c 7000d000.i2c: runtime resume failed -13
[   59.868070] vdd_pexa,vdd_pexb: is_enabled() failed: -13
[   59.873334] tegra-i2c 7000d000.i2c: runtime resume failed -13
[   59.879143] vdd_pexa,vdd_pexb: is_enabled() failed: -13
[   59.884420] Failed to enable avdd-pex-pll: -13
[   59.888877] Failed to enable avdd-plle: -13
[   59.893061] Failed to enable avdd-pexb: -13
[   59.897279] Failed to enable vdd-pexb: -13

[   59.901383] tegra-pcie 3000.pcie: failed to enable regulators: -13
[   60.434185] clk_plle_training: timeout waiting for PLLE
[   60.439565] tegra-pcie 3000.pcie: failed to enable CML clock: -16

[   60.445700] ------------[ cut here ]------------
[   60.450346] WARNING: CPU: 0 PID: 653 at /home/jonathanh/workdir/tegra/mlt-linux_next/kernel/drivers/regulator/core.c:2603 _regulator_disable+0xb8/0x1b4
[   60.463959] unbalanced disables for vdd_pexa,vdd_pexb

[   60.469038] Modules linked in:
[   60.472107] CPU: 0 PID: 653 Comm: rtcwake Tainted: G        W         5.7.0-rc2-next-20200420 #2
[   60.480892] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[   60.487190] [<c0111b68>] (unwind_backtrace) from [<c010bc00>] (show_stack+0x10/0x14)
[   60.494951] [<c010bc00>] (show_stack) from [<c0480f14>] (dump_stack+0xc0/0xd4)
[   60.502189] [<c0480f14>] (dump_stack) from [<c01234a4>] (__warn+0xe0/0xf8)
[   60.509073] [<c01234a4>] (__warn) from [<c0123530>] (warn_slowpath_fmt+0x74/0xb8)
[   60.516568] [<c0123530>] (warn_slowpath_fmt) from [<c0516714>] (_regulator_disable+0xb8/0x1b4)
[   60.525191] [<c0516714>] (_regulator_disable) from [<c0516844>] (regulator_disable+0x34/0xd0)
[   60.533729] [<c0516844>] (regulator_disable) from [<c0518488>] (regulator_bulk_disable+0x28/0xb4)
[   60.542619] [<c0518488>] (regulator_bulk_disable) from [<c04dbc84>] (tegra_pcie_pm_resume+0xbb0/0x107c)
[   60.552032] [<c04dbc84>] (tegra_pcie_pm_resume) from [<c05f7e44>] (dpm_run_callback+0x38/0x1d4)
[   60.560741] [<c05f7e44>] (dpm_run_callback) from [<c05f8af8>] (device_resume_noirq+0x110/0x248)
[   60.569451] [<c05f8af8>] (device_resume_noirq) from [<c05f93e0>] (dpm_resume_noirq+0x10c/0x36c)
[   60.578162] [<c05f93e0>] (dpm_resume_noirq) from [<c017dd74>] (suspend_devices_and_enter+0x27c/0x9dc)
[   60.587393] [<c017dd74>] (suspend_devices_and_enter) from [<c017e7dc>] (pm_suspend+0x308/0x370)
[   60.596110] [<c017e7dc>] (pm_suspend) from [<c017cb30>] (state_store+0x6c/0xc8)
[   60.603440] [<c017cb30>] (state_store) from [<c03138e4>] (kernfs_fop_write+0xf8/0x210)
[   60.611379] [<c03138e4>] (kernfs_fop_write) from [<c0286c44>] (__vfs_write+0x2c/0x1c4)
[   60.619310] [<c0286c44>] (__vfs_write) from [<c02886e8>] (vfs_write+0xa4/0x188)
[   60.626632] [<c02886e8>] (vfs_write) from [<c028898c>] (ksys_write+0xa4/0xd4)
[   60.633778] [<c028898c>] (ksys_write) from [<c01000c0>] (ret_fast_syscall+0x0/0x54)
[   60.641437] Exception stack(0xeda91fa8 to 0xeda91ff0)
[   60.646497] 1fa0:                   0000006c 00498438 00000004 00498438 00000004 00000000
[   60.654683] 1fc0: 0000006c 00498438 00497228 00000004 00000004 00000004 0048478c 00497228

[   60.662866] 1fe0: 00000004 be9029b8 b6ec8c0b b6e53206
[   60.668007] ---[ end trace 5453317048e46ae9 ]---
[   60.672632] Failed to disable vdd-pexb: -5
[   60.676761] tegra-pcie 3000.pcie: tegra pcie power on fail: -16
[   60.682694] PM: dpm_run_callback(): tegra_pcie_pm_resume+0x0/0x107c returns -16
[   60.690035] PM: Device 3000.pcie failed to resume noirq: error -16
[   60.696859] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: EMEM address decode error (SMMU translation error [--S])
[   60.708876] tegra-mc 7000f000.memory-controller: fdcdwr2: write @0x877e8400: Page fault (SMMU translation error [--S])

DMA transfer could be completed, but CPU (which handles DMA interrupt) may get too busy and can't handle the interrupt in a timely manner, despite of DMA IRQ being raised. In this case the DMA state needs to synchronized before terminating DMA transfer in order not to miss the DMA transfer completion.

Signed-off-by Dmitry Osipenko <[email protected]>

Hello,

Recently I found a way to reliably reproduce I2C timeouts that happen due to improper synchronizations made by the I2C driver. It's quite easy to reproduce the problem when memory is running on a lower freq + there is some memory activity + CPU could get busy for a significant time. This is the case when KASAN is enabled and CPU is busy while accessing FS via NFS. This small series addresses the found problems.

Changelog:

v2: - The "Better handle case where CPU0 is busy for a long time" patch
now preserves the old behavior where completion is checked after disabling the interrupt, preventing potential race-condition of the completion awaiting vs interrupt syncing.

Dmitry Osipenko (2):
i2c: tegra: Better handle case where CPU0 is busy for a long time
i2c: tegra: Synchronize DMA before termination

Quoted 16 lines

Hello,

Recently I found a way to reliably reproduce I2C timeouts that happen due to improper synchronizations made by the I2C driver. It's quite easy to reproduce the problem when memory is running on a lower freq + there is some memory activity + CPU could get busy for a significant time. This is the case when KASAN is enabled and CPU is busy while accessing FS via NFS. This small series addresses the found problems.

Changelog:

v2: - The "Better handle case where CPU0 is busy for a long time" patch
now preserves the old behavior where completion is checked after disabling the interrupt, preventing potential race-condition of the completion awaiting vs interrupt syncing.

Quoted 3 lines

Dmitry Osipenko (2):
i2c: tegra: Better handle case where CPU0 is busy for a long time
i2c: tegra: Synchronize DMA before termination

Patches look good to me. I tend to apply them to for-current instead of for-next because they are fixing issues. Even a stable tag?

Quoted 4 lines

Patches look good to me. I tend to apply them to for-current instead of for-next because they are fixing issues. Even a stable tag?

Thank you, yes it should be good to apply this series to 5.7 because the Tegra APBDMA driver dependency-patches are already in 5.7.

The stable tag shouldn't be needed since this is not a critical bug fix and the DMA driver patches are not going into stable. This series should be more actual for the upcoming devices, which should be upstreamed in 5.8+.