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@xxxxxx>
> >> Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
