Re: [PATCH v2 01/22] usb: host: ehci-exynos: deny IRQ0 - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Sergey Shtylyov <s.shtylyov@omp.ru>
Cc: linux-usb@vger.kernel.org, Alan Stern <stern@rowland.harvard.edu>,
	Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org
Subject: Re: [PATCH v2 01/22] usb: host: ehci-exynos: deny IRQ0
Date: Tue, 2 Nov 2021 14:55:49 +0100	[thread overview]
Message-ID: <YYFDZb35RK+lWdgi@kroah.com> (raw)
In-Reply-To: <46d41d2b-4701-fb7f-9a0b-f4a35e59c3d3@omp.ru>

On Mon, Nov 01, 2021 at 11:39:13PM +0300, Sergey Shtylyov wrote:
> Hello!
> 
> On 10/30/21 11:54 AM, Greg Kroah-Hartman 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: 44ed240d6273 ("usb: host: ehci-exynos: Fix error check in exynos_ehci_probe()")
> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> >> ---
> >> Changes in version 2:
> >> - added Alan's ACK.
> >>
> >>  drivers/usb/host/ehci-exynos.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> >> index 1a9b7572e17f..ff4e1261801a 100644
> >> --- a/drivers/usb/host/ehci-exynos.c
> >> +++ b/drivers/usb/host/ehci-exynos.c
> >> @@ -207,6 +207,10 @@ static int exynos_ehci_probe(struct platform_device *pdev)
> >>  		err = irq;
> >>  		goto fail_io;
> >>  	}
> >> +	if (!irq) {
> >> +		err = -EINVAL;
> >> +		goto fail_io;
> >> +	}
> > 
> > This is a huge sign that the api being used here is broken.
> 
>    And you're telling me that after I've wasted  time on v2? :-( Well, at least the series had
> couple blunders, so it couldn't merged for 5.16-rc1 anyway (not sure why these weren't detected
> in v1).

I thought about it some more and noticed it on your v2 submission.

> > Please fix the root cause here, if returning a 0 is an error, then have
> > the function you called to get this irq return an error.
> 
>    Well, technically not, although that doesn't match the kernel-doc for the function now.
> I only don't understand why returning IRQ0 hasn't been replaced still...

Then please work on that.

> > Otherwise you
> > will have to fix ALL callers, and people will always get it wrong.
> > Fix the root cause here, don't paper it over.
> 
>    As I have already told you, I won't have to do it as filtering out is only needed iff 0 is
> used as an indication for something special. IRQ0 is still perfectly valid for request_irq()
> and is even called by arch/{aplha|mips|x86}...

If it is valid, then why can it not be a valid irq for all of these
drivers that you are changing here?  What is preventing them from
running on the platforms where 0 is a valid irq value?

thanks,

greg k-h