[PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

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 <kwizart@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/host/xhci-tegra.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

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")) {
+		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) {
 			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;
+
 	synchronize_irq(tegra->mbox_irq);
 	mutex_lock(&tegra->lock);
 	ret = tegra_xusb_enter_elpg(tegra, true);
-- 
2.32.0

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4462 bytes --]

On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
> 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 <kwizart@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  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.

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

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

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

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

21.10.2021 17:01, Thierry Reding пишет:
> On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
>> 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 <kwizart@gmail.com>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> 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.

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

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

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

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

21.10.2021 17:57, Dmitry Osipenko пишет:
> 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.

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.

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Alan Stern]

On Thu, Oct 21, 2021 at 06:08:41PM +0300, Dmitry Osipenko wrote:
> 21.10.2021 17:57, Dmitry Osipenko пишет:
> > 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.
> 
> 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.

Alan Stern

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

21.10.2021 18:20, Alan Stern пишет:
> On Thu, Oct 21, 2021 at 06:08:41PM +0300, Dmitry Osipenko wrote:
>> 21.10.2021 17:57, Dmitry Osipenko пишет:
>>> 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.
>>
>> 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.

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.

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

21.10.2021 20:13, Dmitry Osipenko пишет:
> 21.10.2021 18:20, Alan Stern пишет:
>> On Thu, Oct 21, 2021 at 06:08:41PM +0300, Dmitry Osipenko wrote:
>>> 21.10.2021 17:57, Dmitry Osipenko пишет:
>>>> 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.
>>>
>>> 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.
> 
> 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.

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

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Alan Stern]

On Thu, Oct 21, 2021 at 08:16:40PM +0300, Dmitry Osipenko wrote:
> 21.10.2021 20:13, Dmitry Osipenko пишет:
> > 21.10.2021 18:20, Alan Stern пишет:
> >> On Thu, Oct 21, 2021 at 06:08:41PM +0300, Dmitry Osipenko wrote:
> >>> 21.10.2021 17:57, Dmitry Osipenko пишет:
> >>>> 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.
> >>>
> >>> 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.
> > 
> > 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.

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

Alan Stern

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

21.10.2021 22:14, Alan Stern пишет:
...
>>> 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.
> 
>>> 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.

I'll take a closer look then, thank you.

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

21.10.2021 17:57, Dmitry Osipenko пишет:
> 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.

I looked at it again and now see that disabling wakeup actually should
restore the old code behaviour properly because previously wakeup wasn't
ever enabled. Alright, I'll prepare v2.

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Michał Mirosław]

On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
> 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.
[...]
> --- 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")) {

Does this catch "interrupts-extended"?

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

Best Regards
Michał Mirosław

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

22.10.2021 00:37, Michał Mirosław пишет:
> On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
>> 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.
> [...]
>> --- 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")) {
> 
> Does this catch "interrupts-extended"?

No, Tegra doesn't use interrupts-extended.

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Michał Mirosław]

On Fri, Oct 22, 2021 at 12:46:23AM +0300, Dmitry Osipenko wrote:
> 22.10.2021 00:37, Michał Mirosław пишет:
> > On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
> >> 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.
> > [...]
> >> --- 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")) {
> > 
> > Does this catch "interrupts-extended"?
> 
> 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.

Best Regards
Michał Mirosław

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

22.10.2021 01:14, Michał Mirosław пишет:
> On Fri, Oct 22, 2021 at 12:46:23AM +0300, Dmitry Osipenko wrote:
>> 22.10.2021 00:37, Michał Mirosław пишет:
>>> On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
>>>> 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.
>>> [...]
>>>> --- 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")) {
>>>
>>> Does this catch "interrupts-extended"?
>>
>> 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.

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Michał Mirosław]

On Fri, Oct 22, 2021 at 08:58:02AM +0300, Dmitry Osipenko wrote:
> 22.10.2021 01:14, Michał Mirosław пишет:
> > On Fri, Oct 22, 2021 at 12:46:23AM +0300, Dmitry Osipenko wrote:
> >> 22.10.2021 00:37, Michał Mirosław пишет:
> >>> On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
> >>>> 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.
> >>> [...]
> >>>> --- 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")) {
> >>>
> >>> Does this catch "interrupts-extended"?
> >>
> >> 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.

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.

Best Regards
Michał Mirosław

Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in device tree

[Dmitry Osipenko]

22.10.2021 12:29, Michał Mirosław пишет:
> On Fri, Oct 22, 2021 at 08:58:02AM +0300, Dmitry Osipenko wrote:
>> 22.10.2021 01:14, Michał Mirosław пишет:
>>> On Fri, Oct 22, 2021 at 12:46:23AM +0300, Dmitry Osipenko wrote:
>>>> 22.10.2021 00:37, Michał Mirosław пишет:
>>>>> On Thu, Oct 21, 2021 at 02:55:01PM +0300, Dmitry Osipenko wrote:
>>>>>> 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.
>>>>> [...]
>>>>>> --- 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")) {
>>>>>
>>>>> Does this catch "interrupts-extended"?
>>>>
>>>> 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.
> 
> 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.

At a quick glance there are only couple drivers which explicitly check
for -EINVAL, others only check whether returned value is negative. Seems
not that bad.