21.10.2021 17:01, Thierry Reding пишет:

On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:

quoted

Older device-trees don't specify padctrl interrupt and xhci-tegra driver
now fails to probe with -EINVAL using those device-trees. Check interrupt
presence and disallow runtime PM suspension if it's missing to fix the
trouble.

Fixes: 971ee247060d ("usb: xhci: tegra: Enable ELPG for runtime/system PM")
Reported-by: Nicolas Chauvet <redacted>
Tested-by: Nicolas Chauvet <redacted>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

I now see that this was broken since 5.14 and not 5.15, so stable tag is
needed.

quoted

---
 drivers/usb/host/xhci-tegra.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

Thanks for typing this up. A couple of minor comments below.

quoted

diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 1bf494b649bd..47927a1df3dc 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c

@@ -1454,10 +1454,13 @@ static int tegra_xusb_probe(struct platform_device *pdev)
 		goto put_padctl;
 	}
 
-	tegra->padctl_irq = of_irq_get(np, 0);
-	if (tegra->padctl_irq <= 0) {
-		err = (tegra->padctl_irq == 0) ? -ENODEV : tegra->padctl_irq;
-		goto put_padctl;
+	/* Older device-trees don't specify padctrl interrupt */
+	if (of_property_read_bool(np, "interrupts")) {

Can't we just rely on the return value from of_irq_get() instead of
explicitly checking for the presence of the "interrupts" property? All
we really want is to make this interrupt optional. As far as I can tell,
of_irq_get() will return -EINVAL (via of_irq_parse_one() and then
of_property_read_u32_index()) if the property doesn't exist, so I'd
think it should be possible to turn this into something like this:

	tegra->padctl_irq = of_irq_get(np, 0);
	if (tegra->padctl_irq == -EINVAL)
		tegra->padctl_irq = 0;

-EINVAL is a too ambiguous error code. If of_irq_get() explicitly
returned -ENOENT, then it would be a different story. It's wrong to rely
on -EINVAL, IMO.

quoted

+		tegra->padctl_irq = of_irq_get(np, 0);
+		if (tegra->padctl_irq <= 0) {
+			err = (tegra->padctl_irq == 0) ? -ENODEV : tegra->padctl_irq;
+			goto put_padctl;
+		}
 	}
 
 	tegra->host_clk = devm_clk_get(&pdev->dev, "xusb_host");

@@ -1696,11 +1699,15 @@ static int tegra_xusb_probe(struct platform_device *pdev)
 		goto remove_usb3;
 	}
 
-	err = devm_request_threaded_irq(&pdev->dev, tegra->padctl_irq, NULL, tegra_xusb_padctl_irq,
-					IRQF_ONESHOT, dev_name(&pdev->dev), tegra);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to request padctl IRQ: %d\n", err);
-		goto remove_usb3;
+	if (tegra->padctl_irq) {
+		err = devm_request_threaded_irq(&pdev->dev, tegra->padctl_irq,
+						NULL, tegra_xusb_padctl_irq,
+						IRQF_ONESHOT, dev_name(&pdev->dev),
+						tegra);
+		if (err < 0) {
+			dev_err(&pdev->dev, "failed to request padctl IRQ: %d\n", err);
+			goto remove_usb3;
+		}
 	}
 
 	err = tegra_xusb_enable_firmware_messages(tegra);

@@ -2132,7 +2139,7 @@ static __maybe_unused int tegra_xusb_suspend(struct device *dev)
 		tegra->suspended = true;
 		pm_runtime_disable(dev);
 
-		if (device_may_wakeup(dev)) {
+		if (device_may_wakeup(dev) && tegra->padctl_irq) {

I wondered if perhaps there was a way to make device_may_wakeup() return
false if we don't have that IRQ. Intuitively I would've thought that the
calls to device_wakeup_enable() and device_init_wakeup() set this all up
but after looking at the code I'm not sure if omitting them would
actually cause device_may_wakeup() to return false. That would certainly
be nicer than these double checks.

It might be wrong to disable device_may_wakeup() because it will change
the system suspend-resume behaviour, i.e. you won't be able to resume by
USB event, see [1].

[1]
https://elixir.bootlin.com/linux/v5.15-rc6/source/drivers/usb/host/xhci-tegra.c#L1962

Although, I'm not sure whether this is a correct behaviour to start
with. Previously, before the offending commit, device_wakeup was never
enabled for tegra-xusb. Commit message doesn't explain why wakeup is now
enabled unconditionally, wakeup checks aren't needed at all then. This
makes no sense, please check it with JC Kuo.

quoted

 			if (enable_irq_wake(tegra->padctl_irq))
 				dev_err(dev, "failed to enable padctl wakes\n");
 		}

@@ -2161,7 +2168,7 @@ static __maybe_unused int tegra_xusb_resume(struct device *dev)
 		return err;
 	}
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && tegra->padctl_irq) {
 		if (disable_irq_wake(tegra->padctl_irq))
 			dev_err(dev, "failed to disable padctl wakes\n");
 	}

@@ -2179,6 +2186,9 @@ static __maybe_unused int tegra_xusb_runtime_suspend(struct device *dev)
 	struct tegra_xusb *tegra = dev_get_drvdata(dev);
 	int ret;
 
+	if (!tegra->padctl_irq)
+		return -EOPNOTSUPP;
+

Similarly, couldn't we enable all that runtime PM stuff conditionally so
that these functions would only ever get called when runtime PM is
actually available? That seems a bit nicer than having this return
-EOPNOTSUPP.

That should be a bigger change and we will need to re-test it all again.
I don't have hardware for testing.

I can delegate this patch to you. Otherwise I will prefer to stick with
the current variant. Alternatively, you can make another change on top
of this patch later on.