| Message ID | 20211018183930.8448-23-s.shtylyov@omp.ru |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
18.10.2021 21:39, Sergey Shtylyov пишет: > If platform_get_irq() returns IRQ0 (considered invalid according to Linus) > the driver blithely passes it to usb_add_hcd() that treats IRQ0 as no IRQ > at all. Deny IRQ0 right away, returning -EINVAL from the probe() method... > > Fixes: e84fce0f8837 ("usb: xhci: Add NVIDIA Tegra XUSB controller driver") > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > --- > drivers/usb/host/xhci-tegra.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index 1bf494b649bd..7151b1d4f876 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -1439,6 +1439,8 @@ static int tegra_xusb_probe(struct platform_device *pdev) > tegra->xhci_irq = platform_get_irq(pdev, 0); > if (tegra->xhci_irq < 0) > return tegra->xhci_irq; > + if (!tegra->xhci_irq) > + return -ENIVAL; > > tegra->mbox_irq = platform_get_irq(pdev, 1); > if (tegra->mbox_irq < 0) > platform_get_irq() never returns zero in accordance to [1], but I see that it can return it [2]. Should be better to fix [2] and return -EINVAL. [1] https://elixir.bootlin.com/linux/v5.15-rc6/source/drivers/base/platform.c#L254 [2] https://elixir.bootlin.com/linux/v5.15-rc6/source/drivers/base/platform.c#L226
Hello! On 10/21/21 12:09 PM, Dmitry Osipenko wrote: [...] >> If platform_get_irq() returns IRQ0 (considered invalid according to Linus) >> the driver blithely passes it to usb_add_hcd() that treats IRQ0 as no IRQ >> at all. Deny IRQ0 right away, returning -EINVAL from the probe() method... >> >> Fixes: e84fce0f8837 ("usb: xhci: Add NVIDIA Tegra XUSB controller driver") >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> --- >> drivers/usb/host/xhci-tegra.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c >> index 1bf494b649bd..7151b1d4f876 100644 >> --- a/drivers/usb/host/xhci-tegra.c >> +++ b/drivers/usb/host/xhci-tegra.c >> @@ -1439,6 +1439,8 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> tegra->xhci_irq = platform_get_irq(pdev, 0); >> if (tegra->xhci_irq < 0) >> return tegra->xhci_irq; >> + if (!tegra->xhci_irq) >> + return -ENIVAL; >> >> tegra->mbox_irq = platform_get_irq(pdev, 1); >> if (tegra->mbox_irq < 0) >> > > platform_get_irq() never returns zero in accordance to [1], but I see > that it can return it [2]. Not only that, it also can be returned thru the normal path (from an IRQ descriptor). I'm not sure whether 0 means an IRQ0 returned from acpi_dev_gpio_irq_get(), looks like yes... > Should be better to fix [2] and return -EINVAL. No, we have WARN() before returning IRQ0 -- if we're going to finally declare IRQ0 invalid, it should be done after this check. > [1] > https://elixir.bootlin.com/linux/v5.15-rc6/source/drivers/base/platform.c#L254 > > [2] > https://elixir.bootlin.com/linux/v5.15-rc6/source/drivers/base/platform.c#L226 MBR, Sergey
26.10.2021 21:24, Sergey Shtylyov пишет: > Hello! > > On 10/21/21 12:09 PM, Dmitry Osipenko wrote: > [...] >>> If platform_get_irq() returns IRQ0 (considered invalid according to Linus) >>> the driver blithely passes it to usb_add_hcd() that treats IRQ0 as no IRQ >>> at all. Deny IRQ0 right away, returning -EINVAL from the probe() method... >>> >>> Fixes: e84fce0f8837 ("usb: xhci: Add NVIDIA Tegra XUSB controller driver") >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> --- >>> drivers/usb/host/xhci-tegra.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c >>> index 1bf494b649bd..7151b1d4f876 100644 >>> --- a/drivers/usb/host/xhci-tegra.c >>> +++ b/drivers/usb/host/xhci-tegra.c >>> @@ -1439,6 +1439,8 @@ static int tegra_xusb_probe(struct platform_device *pdev) >>> tegra->xhci_irq = platform_get_irq(pdev, 0); >>> if (tegra->xhci_irq < 0) >>> return tegra->xhci_irq; >>> + if (!tegra->xhci_irq) >>> + return -ENIVAL; >>> >>> tegra->mbox_irq = platform_get_irq(pdev, 1); >>> if (tegra->mbox_irq < 0) >>> >> >> platform_get_irq() never returns zero in accordance to [1], but I see >> that it can return it [2]. > > Not only that, it also can be returned thru the normal path (from an IRQ descriptor). > I'm not sure whether 0 means an IRQ0 returned from acpi_dev_gpio_irq_get(), looks like yes... > >> Should be better to fix [2] and return -EINVAL. > > No, we have WARN() before returning IRQ0 -- if we're going to finally declare IRQ0 invalid, > it should be done after this check. Warning is already explicitly saying that IRQ0 is invalid, hence IRQ0 *is* declared invalid. Either doc comment or function itself is wrong, both are bad. If function is wrong, then you're fixing symptom instead of fixing the root of the problem.
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index 1bf494b649bd..7151b1d4f876 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -1439,6 +1439,8 @@ static int tegra_xusb_probe(struct platform_device *pdev) tegra->xhci_irq = platform_get_irq(pdev, 0); if (tegra->xhci_irq < 0) return tegra->xhci_irq; + if (!tegra->xhci_irq) + return -ENIVAL; tegra->mbox_irq = platform_get_irq(pdev, 1); if (tegra->mbox_irq < 0)
If platform_get_irq() returns IRQ0 (considered invalid according to Linus) the driver blithely passes it to usb_add_hcd() that treats IRQ0 as no IRQ at all. Deny IRQ0 right away, returning -EINVAL from the probe() method... Fixes: e84fce0f8837 ("usb: xhci: Add NVIDIA Tegra XUSB controller driver") Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- drivers/usb/host/xhci-tegra.c | 2 ++ 1 file changed, 2 insertions(+)