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@xxxxxxxxx>
>> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx>
>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>

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.