usb: xhci: tegra: Check padctrl interrupt presence in device tree

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 <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

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

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

drivers/usb/host/xhci-tegra.c +0 -0

index 1bf494b649bd..47927a1df3dc 100644

drivers/usb/host/xhci-tegra.c +14 -5

+		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);

drivers/usb/host/xhci-tegra.c +1 -1

 			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");
 	}

Quoted 9 lines

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 <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

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

Quoted 3 lines

---
 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 5 lines

drivers/usb/host/xhci-tegra.c +0 -0

index 1bf494b649bd..47927a1df3dc 100644

drivers/usb/host/xhci-tegra.c static int tegra_xusb_probe(struct platform_device *pdev) · line 1458

Quoted 9 lines

drivers/usb/host/xhci-tegra.c +2 -4

@@ -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;

Quoted 33 lines

drivers/usb/host/xhci-tegra.c +14 -5

+		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);
 

drivers/usb/host/xhci-tegra.c static __maybe_unused int tegra_xusb_suspend(struct device *dev) · line 2142

Quoted 2 lines

drivers/usb/host/xhci-tegra.c +1 -1

-		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.

Quoted 16 lines

drivers/usb/host/xhci-tegra.c +1 -1

 			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;
 

drivers/usb/host/xhci-tegra.c static __maybe_unused int tegra_xusb_runtime_suspend(struct device *dev) · line 2191

Quoted 3 lines

drivers/usb/host/xhci-tegra.c +3 -0

+	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.

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.

21.10.2021 17:57, Dmitry Osipenko пишет:

Quoted 4 lines

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

Quoted 8 lines

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

Although, wakeup could be disabled via sysfs, so it makes sense. Still it's not clear whether it's a correct behaviour to enable wakeup during system suspend by default. If it wakes machine from suspend when USB device is plugged/unplugged, then it's a wrong behaviour.

Quoted 14 lines

21.10.2021 17:57, Dmitry Osipenko пишет:

Quoted 4 lines

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

Quoted 8 lines

[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 4 lines

Although, wakeup could be disabled via sysfs, so it makes sense. Still it's not clear whether it's a correct behaviour to enable wakeup during system suspend by default. If it wakes machine from suspend when USB device is plugged/unplugged, then it's a wrong behaviour.

It depends on the details of how the device works. In most cases we do want to enable wakeup by default for host controller devices. The reason is simple enough: If some USB device attached to the HC is enabled for wakeup and sends a wakeup request, we don't want the request to get lost because the HC isn't allowed to forward the request on to the CPU.

But we do not want to enable wakeup for root hubs. In particular, we don't want to wake up a suspended system merely because a USB device has been plugged or unplugged.

Clearly this arrangement depends on the hardware making a distinction between wakeup requests originating from the root hub and those simply passing through the HC.

21.10.2021 18:20, Alan Stern пишет:

Quoted 31 lines · Dmitry Osipenko

Quoted 14 lines

21.10.2021 17:57, Dmitry Osipenko пишет:

Quoted 4 lines

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

Quoted 8 lines

[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 4 lines

Although, wakeup could be disabled via sysfs, so it makes sense. Still it's not clear whether it's a correct behaviour to enable wakeup during system suspend by default. If it wakes machine from suspend when USB device is plugged/unplugged, then it's a wrong behaviour.

It depends on the details of how the device works. In most cases we do want to enable wakeup by default for host controller devices. The reason is simple enough: If some USB device attached to the HC is enabled for wakeup and sends a wakeup request, we don't want the request to get lost because the HC isn't allowed to forward the request on to the CPU.

But we do not want to enable wakeup for root hubs. In particular, we don't want to wake up a suspended system merely because a USB device has been plugged or unplugged.

Quoted 3 lines

Clearly this arrangement depends on the hardware making a distinction between wakeup requests originating from the root hub and those simply passing through the HC.

Should USB keyboard be able to wake up every HC or it's a machine-specific feature?

I'm asking because wakeup works on a typical
Intel hardware, but doesn't work on older Tegra SoCs that use Chipidea controller. It's not obvious to me whether this is something that firmware handles for Intel or it's broken on Tegra. Could you please clarify? If it should work for every HC, then I may try to take a closer look.

21.10.2021 20:13, Dmitry Osipenko пишет:

Quoted 36 lines

21.10.2021 18:20, Alan Stern пишет:

Quoted 34 lines · Dmitry Osipenko

Quoted 14 lines

21.10.2021 17:57, Dmitry Osipenko пишет:

Quoted 4 lines

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

Quoted 8 lines

[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 4 lines

Although, wakeup could be disabled via sysfs, so it makes sense. Still it's not clear whether it's a correct behaviour to enable wakeup during system suspend by default. If it wakes machine from suspend when USB device is plugged/unplugged, then it's a wrong behaviour.

It depends on the details of how the device works. In most cases we do want to enable wakeup by default for host controller devices. The reason is simple enough: If some USB device attached to the HC is enabled for wakeup and sends a wakeup request, we don't want the request to get lost because the HC isn't allowed to forward the request on to the CPU.

But we do not want to enable wakeup for root hubs. In particular, we don't want to wake up a suspended system merely because a USB device has been plugged or unplugged.

Clearly this arrangement depends on the hardware making a distinction between wakeup requests originating from the root hub and those simply passing through the HC.

Quoted 2 lines

Should USB keyboard be able to wake up every HC or it's a machine-specific feature?

I mean whether key press should wake up HC if wake-up is enabled for the keyboard device.

Quoted 6 lines

I'm asking because wakeup works on a typical
Intel hardware, but doesn't work on older Tegra SoCs that use Chipidea controller. It's not obvious to me whether this is something that firmware handles for Intel or it's broken on Tegra. Could you please clarify? If it should work for every HC, then I may try to take a closer look.

Quoted 5 lines

Quoted 2 lines

Should USB keyboard be able to wake up every HC or it's a machine-specific feature?

I mean whether key press should wake up HC if wake-up is enabled for the keyboard device.

Yes, it should. More specifically, if the keyboard is capable of generating wakeup requests, and wakeup is enabled for both the keyboard device and the HC, then the keyboard should be able to wake up the system from suspend. Some HCs or systems might not support this capability, however.

Quoted 6 lines

Quoted 6 lines

I'm asking because wakeup works on a typical
Intel hardware, but doesn't work on older Tegra SoCs that use Chipidea controller. It's not obvious to me whether this is something that firmware handles for Intel or it's broken on Tegra. Could you please clarify? If it should work for every HC, then I may try to take a closer look.

If the hardware/firmware can support it then it should work. I don't know what's going on with your older Tegra SoCs; it's possible there is a bug in the driver.

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.

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.

Quoted 4 lines

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.

[...]

Quoted 6 lines

drivers/usb/host/xhci-tegra.c +0 -0

@@ -1454,10 +1454,13 @@ static int tegra_xusb_probe(struct platform_device *pdev)
 		goto put_padctl;
 	}
 

drivers/usb/host/xhci-tegra.c static int tegra_xusb_probe(struct platform_device *pdev) · line 1458

Quoted 6 lines

drivers/usb/host/xhci-tegra.c +2 -4

-	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")) {

Does this catch "interrupts-extended"?

22.10.2021 00:37, Michał Mirosław пишет:

Quoted 12 lines · Dmitry Osipenko

Quoted 4 lines

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.

[...]

Quoted 6 lines

drivers/usb/host/xhci-tegra.c +0 -0

@@ -1454,10 +1454,13 @@ static int tegra_xusb_probe(struct platform_device *pdev)
 		goto put_padctl;
 	}
 

Quoted 8 lines

drivers/usb/host/xhci-tegra.c static int tegra_xusb_probe(struct platform_device *pdev) · line 1458

Quoted 6 lines

drivers/usb/host/xhci-tegra.c +2 -4

-	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")) {

Does this catch "interrupts-extended"?

No, Tegra doesn't use interrupts-extended.

Quoted 22 lines

22.10.2021 00:37, Michał Mirosław пишет:

Quoted 20 lines · Dmitry Osipenko

Quoted 4 lines

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.

[...]

drivers/usb/host/xhci-tegra.c static int tegra_xusb_probe(struct platform_device *pdev) · line 1458

Quoted 12 lines

drivers/usb/host/xhci-tegra.c +2 -4

@@ -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")) {

Does this catch "interrupts-extended"?

Quoted 1 line

No, Tegra doesn't use interrupts-extended.

I believe it is generic and equivalent to "interrupt-parent" + "interrupts" properties, so people might as well put this in the DT to save (or loose) a few bytes.

You could just check if of_irq_get() returned -EINVAL instead of matching "interrupts" property.

22.10.2021 01:14, Michał Mirosław пишет:

Quoted 23 lines · Dmitry Osipenko

Quoted 22 lines

22.10.2021 00:37, Michał Mirosław пишет:

Quoted 20 lines · Dmitry Osipenko

Quoted 4 lines

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.

[...]

drivers/usb/host/xhci-tegra.c static int tegra_xusb_probe(struct platform_device *pdev) · line 1458

Quoted 12 lines

drivers/usb/host/xhci-tegra.c +2 -4

@@ -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")) {

Does this catch "interrupts-extended"?

Quoted 8 lines

Quoted 1 line

No, Tegra doesn't use interrupts-extended.

I believe it is generic and equivalent to "interrupt-parent" + "interrupts" properties, so people might as well put this in the DT to save (or loose) a few bytes.

You could just check if of_irq_get() returned -EINVAL instead of matching "interrupts" property.

It should be a bad idea to rely on -EINVAL since it's ambiguous error code.

Perhaps it's fine to assume that today of_irq_get() may only return -EINVAL in a case of a missing DT property, but then it should be two patches here:

1. Use -EINVAL and backport this fix to stable kernel.
2. Change of_irq_get() to return -ENOENT for a missing property and change tegra_xusb_probe() accordingly.

Quoted 39 lines

22.10.2021 01:14, Michał Mirosław пишет:

Quoted 23 lines · Dmitry Osipenko

Quoted 22 lines

22.10.2021 00:37, Michał Mirosław пишет:

Quoted 20 lines · Dmitry Osipenko

Quoted 4 lines

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.

[...]

drivers/usb/host/xhci-tegra.c static int tegra_xusb_probe(struct platform_device *pdev) · line 1458

Quoted 12 lines

drivers/usb/host/xhci-tegra.c +2 -4

@@ -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")) {

Does this catch "interrupts-extended"?

Quoted 8 lines

Quoted 1 line

No, Tegra doesn't use interrupts-extended.

I believe it is generic and equivalent to "interrupt-parent" + "interrupts" properties, so people might as well put this in the DT to save (or loose) a few bytes.

You could just check if of_irq_get() returned -EINVAL instead of matching "interrupts" property.

It should be a bad idea to rely on -EINVAL since it's ambiguous error code.

Perhaps it's fine to assume that today of_irq_get() may only return -EINVAL in a case of a missing DT property, but then it should be two patches here:

Quoted 3 lines

1. Use -EINVAL and backport this fix to stable kernel.
2. Change of_irq_get() to return -ENOENT for a missing property and change tegra_xusb_probe() accordingly.

I would love to see the part 2 done, but I'm afraid you will need to change a lot of callsites before that can happen.