| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f122caa-c810-534d-b2a1-53edef313ff0@gmail.com>
Date: Thu, 21 Oct 2021 17:57:14 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Thierry Reding <thierry.reding@...il.com>
Cc: Thierry Reding <treding@...dia.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mathias Nyman <mathias.nyman@...el.com>,
JC Kuo <jckuo@...dia.com>, Nicolas Chauvet <kwizart@...il.com>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1] usb: xhci: tegra: Check padctrl interrupt presence in
device tree
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@...il.com>
>> Tested-by: Nicolas Chauvet <kwizart@...il.com>
>> Signed-off-by: Dmitry Osipenko <digetx@...il.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.
Powered by blists - more mailing lists