| Message ID | 20211209145937.77719-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved | expand |
Hi, On 12/9/21 15:59, Andy Shevchenko wrote: > platform_get_irq() will print a message when it fails. > No need to repeat this. > > While at it, drop redundant check for 0 as platform_get_irq() spills > out a big WARN() in such case. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks, the series looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> for the series. Regards, Hans > --- > drivers/ata/libahci_platform.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 0910441321f7..1af642c84e7b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > int i, irq, n_ports, rc; > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - if (irq != -EPROBE_DEFER) > - dev_err(dev, "no irq\n"); > + if (irq < 0) > return irq; > - } > - if (!irq) > - return -EINVAL; > > hpriv->irq = irq; > >
On 12/9/21 5:59 PM, Andy Shevchenko wrote: > platform_get_irq() will print a message when it fails. > No need to repeat this. > > While at it, drop redundant check for 0 as platform_get_irq() spills > out a big WARN() in such case. And? IRQ0 is still returned! :-( > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/ata/libahci_platform.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 0910441321f7..1af642c84e7b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > int i, irq, n_ports, rc; > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - if (irq != -EPROBE_DEFER) > - dev_err(dev, "no irq\n"); > + if (irq < 0) > return irq; > - } > - if (!irq) > - return -EINVAL; This is prermature -- let's wait till my patch that stops returning IRQ0 from platform_get_irq() and friends gets merged.... MBR, Sergey
On Thu, Dec 09, 2021 at 08:24:59PM +0300, Sergey Shtylyov wrote: > On 12/9/21 5:59 PM, Andy Shevchenko wrote: > > > platform_get_irq() will print a message when it fails. > > No need to repeat this. > > > > While at it, drop redundant check for 0 as platform_get_irq() spills > > out a big WARN() in such case. > > And? IRQ0 is still returned! :-( It should not be returned in the first place. ... > > - if (!irq) > > - return -EINVAL; > > This is prermature -- let's wait till my patch that stops returning IRQ0 from > platform_get_irq() and friends gets merged.... What patch? Does it fix platform_get_irq_optional()?
On 12/9/21 8:42 PM, Andy Shevchenko wrote: >>> platform_get_irq() will print a message when it fails. >>> No need to repeat this. >>> >>> While at it, drop redundant check for 0 as platform_get_irq() spills >>> out a big WARN() in such case. >> >> And? IRQ0 is still returned! :-( > > It should not be returned in the first place. But it still is, despite the WARN(), right? > ... > >>> - if (!irq) >>> - return -EINVAL; >> >> This is prermature -- let's wait till my patch that stops returning IRQ0 from >> platform_get_irq() and friends gets merged.... > > What patch? https://marc.info/?l=linux-kernel&m=163623041902285 > Does it fix platform_get_irq_optional()? Of course! :-) MBR, Sergey
On Thu, Dec 09, 2021 at 09:22:32PM +0300, Sergey Shtylyov wrote: > On 12/9/21 8:42 PM, Andy Shevchenko wrote: ... > >>> No need to repeat this. > >>> > >>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>> out a big WARN() in such case. > >> > >> And? IRQ0 is still returned! :-( > > > > It should not be returned in the first place. > > But it still is, despite the WARN(), right? So, you admit that there is a code which does that? That code should be fixed first. Have you sent a patch? ... > >>> - if (!irq) > >>> - return -EINVAL; > >> > >> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >> platform_get_irq() and friends gets merged.... > > > > What patch? > > https://marc.info/?l=linux-kernel&m=163623041902285 > > > Does it fix platform_get_irq_optional()? > > Of course! :-) Can you share link to lore.kernel.org, please? It will make much easier to try and comment.
On Thu, Dec 09, 2021 at 09:22:55PM +0200, Andy Shevchenko wrote: > On Thu, Dec 09, 2021 at 09:22:32PM +0300, Sergey Shtylyov wrote: > > On 12/9/21 8:42 PM, Andy Shevchenko wrote: ... > > > Does it fix platform_get_irq_optional()? > > > > Of course! :-) > > Can you share link to lore.kernel.org, please? > It will make much easier to try and comment. For the record it does not fix platform_get_irq_optional() AFAICS. Have you read the discussion: https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u ?
On 12/9/21 10:22 PM, Andy Shevchenko wrote: [...] >>>>> No need to repeat this. >>>>> >>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>> out a big WARN() in such case. >>>> >>>> And? IRQ0 is still returned! :-( >>> >>> It should not be returned in the first place. >> >> But it still is, despite the WARN(), right? > > So, you admit that there is a code which does that? I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they shouldn't? =) > That code should be fixed first. Have you sent a patch? Which code?! You got me totally muddled. =) > ... > >>>>> - if (!irq) >>>>> - return -EINVAL; >>>> >>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from >>>> platform_get_irq() and friends gets merged.... >>> >>> What patch? >> >> https://marc.info/?l=linux-kernel&m=163623041902285 >> >>> Does it fix platform_get_irq_optional()? >> >> Of course! :-) > > Can you share link to lore.kernel.org, please? > It will make much easier to try and comment. I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, so I'm afraid you're on your own here... MBR, Sergey
On 12/9/21 10:27 PM, Andy Shevchenko wrote: [...] >>>> Does it fix platform_get_irq_optional()? >>> >>> Of course! :-) >> >> Can you share link to lore.kernel.org, please? >> It will make much easier to try and comment. > > For the record it does not fix platform_get_irq_optional() AFAICS. Again, why? My only issue is stoppping to return IRQ0 --- which it does. :-) > Have you read the discussion: > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u > ? Yes, I have. I just don't care about it much... :-) MBR, Sergey
On 2021/12/09 23:59, Andy Shevchenko wrote: > platform_get_irq() will print a message when it fails. > No need to repeat this. > > While at it, drop redundant check for 0 as platform_get_irq() spills > out a big WARN() in such case. The reason you should be able to remove the "if (!irq)" test is that platform_get_irq() never returns 0. At least, that is what the function kdoc says. But looking at platform_get_irq_optional(), which is called by platform_get_irq(), the out label is: WARN(ret == 0, "0 is an invalid IRQ number\n"); return ret; So 0 will be returned as-is. That is rather weird. That should be fixed to return -ENXIO: if (WARN(ret == 0, "0 is an invalid IRQ number\n")) return -ENXIO; return ret; Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/ata/libahci_platform.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 0910441321f7..1af642c84e7b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > int i, irq, n_ports, rc; > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - if (irq != -EPROBE_DEFER) > - dev_err(dev, "no irq\n"); > + if (irq < 0) > return irq; > - } > - if (!irq) > - return -EINVAL; > > hpriv->irq = irq; >
On 2021/12/10 7:49, Damien Le Moal wrote: > On 2021/12/09 23:59, Andy Shevchenko wrote: >> platform_get_irq() will print a message when it fails. >> No need to repeat this. >> >> While at it, drop redundant check for 0 as platform_get_irq() spills >> out a big WARN() in such case. > > The reason you should be able to remove the "if (!irq)" test is that > platform_get_irq() never returns 0. At least, that is what the function kdoc > says. But looking at platform_get_irq_optional(), which is called by > platform_get_irq(), the out label is: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > return ret; > > So 0 will be returned as-is. That is rather weird. That should be fixed to > return -ENXIO: > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > return -ENXIO; > return ret; > > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? Replying to myself :) I replied before reading Sergei replies that points this out. > >> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/ata/libahci_platform.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c >> index 0910441321f7..1af642c84e7b 100644 >> --- a/drivers/ata/libahci_platform.c >> +++ b/drivers/ata/libahci_platform.c >> @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, >> int i, irq, n_ports, rc; >> >> irq = platform_get_irq(pdev, 0); >> - if (irq < 0) { >> - if (irq != -EPROBE_DEFER) >> - dev_err(dev, "no irq\n"); >> + if (irq < 0) >> return irq; >> - } >> - if (!irq) >> - return -EINVAL; >> >> hpriv->irq = irq; >> > >
On Fri, Dec 10, 2021 at 4:47 AM Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote: > On 2021/12/09 23:59, Andy Shevchenko wrote: > > platform_get_irq() will print a message when it fails. > > No need to repeat this. > > > > While at it, drop redundant check for 0 as platform_get_irq() spills > > out a big WARN() in such case. > > The reason you should be able to remove the "if (!irq)" test is that > platform_get_irq() never returns 0. At least, that is what the function kdoc > says. But looking at platform_get_irq_optional(), which is called by > platform_get_irq(), the out label is: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > return ret; > > So 0 will be returned as-is. That is rather weird. That should be fixed to > return -ENXIO: > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > return -ENXIO; No, this is wrong for the same reasons I explained to Sergey. The problem is that this is _optional API and it has been misdesigned. Replacing things like above will increase the mess. > return ret; > > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? No. This is not a business of the caller to workaround implementation details (bugs) of the core APIs. If something goes wrong, then it's platform_get_irq() to blame, and not the libahci_platform.
On 12/10/21 1:49 AM, Damien Le Moal wrote: >> platform_get_irq() will print a message when it fails. >> No need to repeat this. >> >> While at it, drop redundant check for 0 as platform_get_irq() spills >> out a big WARN() in such case. > > The reason you should be able to remove the "if (!irq)" test is that > platform_get_irq() never returns 0. At least, that is what the function kdoc > says. But looking at platform_get_irq_optional(), which is called by > platform_get_irq(), the out label is: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > return ret; > > So 0 will be returned as-is. That is rather weird. That should be fixed to > return -ENXIO: > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > return -ENXIO; > return ret; My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this but returns -EINVAL instead. > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? Of course it isn't... >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> [...] MBR, Sergey
On Thu, Dec 09, 2021 at 11:29:07PM +0300, Sergey Shtylyov wrote: > On 12/9/21 10:22 PM, Andy Shevchenko wrote: ... > >>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>> out a big WARN() in such case. > >>>> > >>>> And? IRQ0 is still returned! :-( > >>> > >>> It should not be returned in the first place. > >> > >> But it still is, despite the WARN(), right? > > > > So, you admit that there is a code which does that? > > I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they > shouldn't? =) That there is a code beneath platform_get_irq() that returns 0, yes. > > That code should be fixed first. Have you sent a patch? > > Which code?! You got me totally muddled. =) Above mentioned. ... > >>>>> - if (!irq) > >>>>> - return -EINVAL; > >>>> > >>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >>>> platform_get_irq() and friends gets merged.... > >>> > >>> What patch? > >> > >> https://marc.info/?l=linux-kernel&m=163623041902285 > >> > >>> Does it fix platform_get_irq_optional()? > >> > >> Of course! :-) > > > > Can you share link to lore.kernel.org, please? > > It will make much easier to try and comment. > > I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > so I'm afraid you're on your own here... lore.kernel.org is the official mailing list archive for Linux kernel work AFAIU. Other sites may do whatever they want with that information, so --> they are unreliable. If you wish to follow the better process, use lore.kernel.org. Understanding how it works takes no more than 5 minutes by engineer with your kind of experience with Linux kernel development.
On Fri, Dec 10, 2021 at 11:59:00AM +0300, Sergey Shtylyov wrote: > On 12/10/21 1:49 AM, Damien Le Moal wrote: > > >> platform_get_irq() will print a message when it fails. > >> No need to repeat this. > >> > >> While at it, drop redundant check for 0 as platform_get_irq() spills > >> out a big WARN() in such case. > > > > The reason you should be able to remove the "if (!irq)" test is that > > platform_get_irq() never returns 0. At least, that is what the function kdoc > > says. But looking at platform_get_irq_optional(), which is called by > > platform_get_irq(), the out label is: > > > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > > return ret; > > > > So 0 will be returned as-is. That is rather weird. That should be fixed to > > return -ENXIO: > > > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > > return -ENXIO; > > return ret; > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > but returns -EINVAL instead. > > > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > Of course it isn't... It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of those API calls. If it is the case, go and fix them, no need to workaround in each of the callers.
Hello! On 12/10/21 1:44 PM, Andy Shevchenko wrote: >>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>> out a big WARN() in such case. >>>>>> >>>>>> And? IRQ0 is still returned! :-( >>>>> >>>>> It should not be returned in the first place. >>>> >>>> But it still is, despite the WARN(), right? >>> >>> So, you admit that there is a code which does that? >> >> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they >> shouldn't? =) > > That there is a code beneath platform_get_irq() that returns 0, yes. Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to know it much better. :-) Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... >>> That code should be fixed first. Have you sent a patch? >> >> Which code?! You got me totally muddled. =) > > Above mentioned. What needs to be fixed in this case is the interrupt controller driver. Quoting Linus (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at the end of the controller's interrupt range... I currently have no information on the platforms requiring such kind of fixing (Alchemy don't seem to need it now)... > ... > >>>>>>> - if (!irq) >>>>>>> - return -EINVAL; >>>>>> >>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from >>>>>> platform_get_irq() and friends gets merged.... >>>>> >>>>> What patch? >>>> >>>> https://marc.info/?l=linux-kernel&m=163623041902285 >>>> >>>>> Does it fix platform_get_irq_optional()? >>>> >>>> Of course! :-) >>> >>> Can you share link to lore.kernel.org, please? >>> It will make much easier to try and comment. >> >> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, A little bit, I meant to type. >> so I'm afraid you're on your own here... > > lore.kernel.org is the official mailing list archive for Linux kernel work > AFAIU. Other sites may do whatever they want with that information, so --> > they are unreliable. If you wish to follow the better process, use > lore.kernel.org. Understanding how it works takes no more than 5 minutes > by engineer with your kind of experience with Linux kernel development. OK, I'll explore this archive when I have time. BTW, does it keep the messages not posted to LKML (I tend to only CC LKML if there's no other mailing lists to post to)? MBR, Sergey
On 12/10/21 1:46 PM, Andy Shevchenko wrote: >>>> platform_get_irq() will print a message when it fails. >>>> No need to repeat this. >>>> >>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>> out a big WARN() in such case. >>> >>> The reason you should be able to remove the "if (!irq)" test is that >>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>> says. But looking at platform_get_irq_optional(), which is called by >>> platform_get_irq(), the out label is: >>> >>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>> return ret; >>> >>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>> return -ENXIO: >>> >>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>> return -ENXIO; >>> return ret; >> >> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >> but returns -EINVAL instead. >> >>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >> >> Of course it isn't... > > It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > those API calls. We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() is there... > If it is the case, go and fix them, no need to workaround > in each of the callers. There's a need to work around as long as IRQ0 ican be returned, otherwise we get partly functioning or non-functioning drivers... MBR, Sergey
On Fri, Dec 10, 2021 at 02:14:15PM +0300, Sergey Shtylyov wrote: > On 12/10/21 1:44 PM, Andy Shevchenko wrote: > > >>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>>> out a big WARN() in such case. > >>>>>> > >>>>>> And? IRQ0 is still returned! :-( > >>>>> > >>>>> It should not be returned in the first place. > >>>> > >>>> But it still is, despite the WARN(), right? > >>> > >>> So, you admit that there is a code which does that? > >> > >> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they > >> shouldn't? =) > > > > That there is a code beneath platform_get_irq() that returns 0, yes. > > Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- > I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to > know it much better. :-) And what is your point here exactly? If == 0 case happens, it will be immediately WARN() and reported (I hope) since it will mean bug in the code. > Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs > that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 > (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... You mixed up HW IRQ with vIRQ. The former one may be 0 and it's completely valid case, while the second one is not. > >>> That code should be fixed first. Have you sent a patch? > >> > >> Which code?! You got me totally muddled. =) > > > > Above mentioned. > > What needs to be fixed in this case is the interrupt controller driver. What do you mean by that? vIRQ is handled by IRQ core, IRQ controller driver just a mere provider of the resource. And those exceptions for vIRQ == 0 shouldn't be propagated to the platform code or so. > Quoting Linus > (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at > the end of the controller's interrupt range... I currently have no information on the > platforms requiring such kind of fixing (Alchemy don't seem to need it now)... Again, do not mix vIRQ (about which Linus ranted) and HW IRQ. ... > >>>>>>> - if (!irq) > >>>>>>> - return -EINVAL; > >>>>>> > >>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >>>>>> platform_get_irq() and friends gets merged.... > >>>>> > >>>>> What patch? > >>>> > >>>> https://marc.info/?l=linux-kernel&m=163623041902285 > >>>> > >>>>> Does it fix platform_get_irq_optional()? > >>>> > >>>> Of course! :-) > >>> > >>> Can you share link to lore.kernel.org, please? > >>> It will make much easier to try and comment. > >> > >> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > A little bit, I meant to type. No problem. I just haven't got what other IRQ0 issues except fixing platform_get_irq_optional() et al. could be possibly needed... > >> so I'm afraid you're on your own here... > > > > lore.kernel.org is the official mailing list archive for Linux kernel work > > AFAIU. Other sites may do whatever they want with that information, so --> > > they are unreliable. If you wish to follow the better process, use > > lore.kernel.org. Understanding how it works takes no more than 5 minutes > > by engineer with your kind of experience with Linux kernel development. > > OK, I'll explore this archive when I have time. BTW, does it keep the messages not > posted to LKML (I tend to only CC LKML if there's no other mailing lists to post to)? TL;DR: yes.
On Fri, Dec 10, 2021 at 02:19:52PM +0300, Sergey Shtylyov wrote: > On 12/10/21 1:46 PM, Andy Shevchenko wrote: > > >>>> platform_get_irq() will print a message when it fails. > >>>> No need to repeat this. > >>>> > >>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>> out a big WARN() in such case. > >>> > >>> The reason you should be able to remove the "if (!irq)" test is that > >>> platform_get_irq() never returns 0. At least, that is what the function kdoc > >>> says. But looking at platform_get_irq_optional(), which is called by > >>> platform_get_irq(), the out label is: > >>> > >>> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >>> return ret; > >>> > >>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>> return -ENXIO: > >>> > >>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>> return -ENXIO; > >>> return ret; > >> > >> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >> but returns -EINVAL instead. > >> > >>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > >> > >> Of course it isn't... > > > > It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > > those API calls. > > We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() > is there... So, have you seen this warning (being reported) related to libahci_platform? If no, what we are discussing about then? The workaround is redundant and no need to have a dead code in the driver, really. > > If it is the case, go and fix them, no need to workaround > > in each of the callers. > > There's a need to work around as long as IRQ0 ican be returned, otherwise > we get partly functioning or non-functioning drivers... You get them unfunctioning anyways and you get the big WARN() even before this patch.
On 12/10/21 11:47 AM, Andy Shevchenko wrote: >>> platform_get_irq() will print a message when it fails. >>> No need to repeat this. >>> >>> While at it, drop redundant check for 0 as platform_get_irq() spills >>> out a big WARN() in such case. >> >> The reason you should be able to remove the "if (!irq)" test is that >> platform_get_irq() never returns 0. At least, that is what the function kdoc >> says. But looking at platform_get_irq_optional(), which is called by >> platform_get_irq(), the out label is: >> >> WARN(ret == 0, "0 is an invalid IRQ number\n"); >> return ret; >> >> So 0 will be returned as-is. That is rather weird. That should be fixed to >> return -ENXIO: >> >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >> return -ENXIO; -ENXIO seems to me more fitting indeed (than -EINVAL that I used). > > No, this is wrong for the same reasons I explained to Sergey. I fail to understand you, sorry. We're going in circles, it seems... :-/ > The problem is that this is _optional API and it has been misdesigned. > Replacing things like above will increase the mess. What's wrong with replacing IRQ0 with -ENXIO now? platform_get_irq_optional() (as in your patch) could then happily return 0 ISO -ENXIO. Contrarywise, if we don't replace IRQ0 with -ENXIO, platform_get_irq_optional() will return 0 for both IRQ0 and missing IRQ! Am I clear enough? If you don't understand me now, I don't know what to say... :-/ > >> return ret; >> >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > No. This is not a business of the caller to workaround implementation > details (bugs) of the core APIs. > If something goes wrong, then it's platform_get_irq() to blame, and > not the libahci_platform. I'm repeating myself already: we don't work around the bug in platform_get_irq(), we're working around the driver subsystems that treat 0 specially (and so don't support IRQ0); libata treats 0 as an indication of the polling mode (moreover, it will curse if you pass to it both IRQ == 0 and a pointer to an interrupt handler! Am I clear enough this time? :-) MBR, Sergey
On 12/10/21 2:36 PM, Andy Shevchenko wrote: >>>>>> platform_get_irq() will print a message when it fails. >>>>>> No need to repeat this. >>>>>> >>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>> out a big WARN() in such case. >>>>> >>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>> platform_get_irq(), the out label is: >>>>> >>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>> return ret; >>>>> >>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>> return -ENXIO: >>>>> >>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>> return -ENXIO; >>>>> return ret; >>>> >>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>> but returns -EINVAL instead. >>>> >>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>> >>>> Of course it isn't... >>> >>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>> those API calls. >> >> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >> is there... > > So, have you seen this warning (being reported) related to libahci_platform? No (as if you need to really see this while it's obvious from the code review). > If no, what we are discussing about then? The workaround is redundant and I don't know. :-) Your arguments so far seem bogus (sorry! :-))... > no need to have a dead code in the driver, really. "Jazz isn't dead, it just smells funny". :-) >>> If it is the case, go and fix them, no need to workaround >>> in each of the callers. >> >> There's a need to work around as long as IRQ0 ican be returned, otherwise >> we get partly functioning or non-functioning drivers... > > You get them unfunctioning anyways The drivers would be broken in not quite obvious ways. With IRQ0 check, they just don't probe anymore. See the explanation of the IRQ0 check (in the drivers) in my previous mail... > and you get the big WARN() even before this patch. As if that was enough... The IRQ0 problem exists for at least 15 (if not 20) years... MBR, Sergey
On 12/10/21 2:28 PM, Andy Shevchenko wrote: >>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>> out a big WARN() in such case. >>>>>>>> >>>>>>>> And? IRQ0 is still returned! :-( >>>>>>> >>>>>>> It should not be returned in the first place. >>>>>> >>>>>> But it still is, despite the WARN(), right? >>>>> >>>>> So, you admit that there is a code which does that? >>>> >>>> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they >>>> shouldn't? =) >>> >>> That there is a code beneath platform_get_irq() that returns 0, yes. >> >> Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- >> I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to >> know it much better. :-) > > And what is your point here exactly? You're saying IRQ0 shouldn't be returned (by the ACPI code) -- from this fragment we can see that it may be returned... > If == 0 case happens, it will be > immediately WARN() and reported (I hope) Well, "hope dies last"... :-) > since it will mean bug in the code. > >> Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs >> that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 >> (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... > > You mixed up HW IRQ with vIRQ. I didn't. Linux expects the vIRQs (I called them Linux IRQs). In the 2.6.1x time frame those corresponded 1:1 on Alchemy. Also, there's 8259 which is always mapped at vIRQ0 (or the legacy drivers won't work). > The former one may be 0 and it's completely valid case, while > the second one is not. Well, request_irq() happilly takes vIRQ0. Moreover, there are 8253 drivers in e.g. the arch/x86/ (PPC and MIPS too) which do use vIRQ0. >>>>> That code should be fixed first. Have you sent a patch? >>>> >>>> Which code?! You got me totally muddled. =) >>> >>> Above mentioned. >> >> What needs to be fixed in this case is the interrupt controller driver. > > What do you mean by that? You better ask Linus... ;-) > vIRQ is handled by IRQ core, IRQ controller driver > just a mere provider of the resource. And those exceptions for vIRQ == 0 > shouldn't be propagated to the platform code or so. >> Quoting Linus >> (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at >> the end of the controller's interrupt range... I currently have no information on the >> platforms requiring such kind of fixing (Alchemy don't seem to need it now)... Well, actually that Linus' quote predates drivers/irqchip/, so I must confess this argument was wrong... :-) > Again, do not mix vIRQ (about which Linus ranted) and HW IRQ. > > ... > >>>>>>>>> - if (!irq) >>>>>>>>> - return -EINVAL; >>>>>>>> >>>>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from >>>>>>>> platform_get_irq() and friends gets merged.... >>>>>>> >>>>>>> What patch? >>>>>> >>>>>> https://marc.info/?l=linux-kernel&m=163623041902285 >>>>>> >>>>>>> Does it fix platform_get_irq_optional()? >>>>>> >>>>>> Of course! :-) >>>>> >>>>> Can you share link to lore.kernel.org, please? >>>>> It will make much easier to try and comment. >>>> >>>> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > >> A little bit, I meant to type. > > No problem. I just haven't got what other IRQ0 issues except fixing > platform_get_irq_optional() et al. could be possibly needed... There is other IRQ0 issue which is very old already... [...] MBR, Sergey
On Fri, Dec 10, 2021 at 08:39:38PM +0300, Sergey Shtylyov wrote: > On 12/10/21 2:28 PM, Andy Shevchenko wrote: > > >>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>>>>> out a big WARN() in such case. > >>>>>>>> > >>>>>>>> And? IRQ0 is still returned! :-( > >>>>>>> > >>>>>>> It should not be returned in the first place. > >>>>>> > >>>>>> But it still is, despite the WARN(), right? > >>>>> > >>>>> So, you admit that there is a code which does that? > >>>> > >>>> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they > >>>> shouldn't? =) > >>> > >>> That there is a code beneath platform_get_irq() that returns 0, yes. > >> > >> Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- > >> I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to > >> know it much better. :-) > > > > And what is your point here exactly? > > You're saying IRQ0 shouldn't be returned (by the ACPI code) -- from this fragment > we can see that it may be returned... Please, provide your analysis, I really don't see how it's possible. If you prove that, we must fix the severe bug then. > > If == 0 case happens, it will be > > immediately WARN() and reported (I hope) > > Well, "hope dies last"... :-) Believe, big WARNs are quite likely to be reported if not by humans, then by CIs and fuzzers. So, the hope is rather to word 'immediately'. > > since it will mean bug in the code. > > > >> Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs > >> that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 > >> (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... > > > > You mixed up HW IRQ with vIRQ. > > I didn't. Linux expects the vIRQs (I called them Linux IRQs). In the 2.6.1x time frame > those corresponded 1:1 on Alchemy. Also, there's 8259 which is always mapped at vIRQ0 (or > the legacy drivers won't work). > > > The former one may be 0 and it's completely valid case, while > > the second one is not. > > Well, request_irq() happilly takes vIRQ0. Moreover, there are 8253 drivers in e.g. the arch/x86/ > (PPC and MIPS too) which do use vIRQ0. This is an exception which is not in the scope here. Let me remind that the topic here is libahci_platform and platform_get_irq(). > >>>>> That code should be fixed first. Have you sent a patch? > >>>> > >>>> Which code?! You got me totally muddled. =) > >>> > >>> Above mentioned. > >> > >> What needs to be fixed in this case is the interrupt controller driver. > > > > What do you mean by that? > > You better ask Linus... ;-) If you cite somebody you have to understand what they said, right? Lemme repeat the question, what do you mean by that? In your own words, please. > > vIRQ is handled by IRQ core, IRQ controller driver > > just a mere provider of the resource. And those exceptions for vIRQ == 0 > > shouldn't be propagated to the platform code or so. > > >> Quoting Linus > >> (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at > >> the end of the controller's interrupt range... I currently have no information on the > >> platforms requiring such kind of fixing (Alchemy don't seem to need it now)... > > Well, actually that Linus' quote predates drivers/irqchip/, so I must confess this > argument was wrong... :-) > > > Again, do not mix vIRQ (about which Linus ranted) and HW IRQ. ... > >>>>>>>>> - if (!irq) > >>>>>>>>> - return -EINVAL; > >>>>>>>> > >>>>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >>>>>>>> platform_get_irq() and friends gets merged.... > >>>>>>> > >>>>>>> What patch? > >>>>>> > >>>>>> https://marc.info/?l=linux-kernel&m=163623041902285 > >>>>>> > >>>>>>> Does it fix platform_get_irq_optional()? > >>>>>> > >>>>>> Of course! :-) > >>>>> > >>>>> Can you share link to lore.kernel.org, please? > >>>>> It will make much easier to try and comment. > >>>> > >>>> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > > > >> A little bit, I meant to type. > > > > No problem. I just haven't got what other IRQ0 issues except fixing > > platform_get_irq_optional() et al. could be possibly needed... > > There is other IRQ0 issue which is very old already... Is it big secret? What is that issue?
On Fri, Dec 10, 2021 at 07:38:40PM +0300, Sergey Shtylyov wrote: > On 12/10/21 11:47 AM, Andy Shevchenko wrote: > > >>> platform_get_irq() will print a message when it fails. > >>> No need to repeat this. > >>> > >>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>> out a big WARN() in such case. > >> > >> The reason you should be able to remove the "if (!irq)" test is that > >> platform_get_irq() never returns 0. At least, that is what the function kdoc > >> says. But looking at platform_get_irq_optional(), which is called by > >> platform_get_irq(), the out label is: > >> > >> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >> return ret; > >> > >> So 0 will be returned as-is. That is rather weird. That should be fixed to > >> return -ENXIO: > >> > >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >> return -ENXIO; > > -ENXIO seems to me more fitting indeed (than -EINVAL that I used). > > > > > No, this is wrong for the same reasons I explained to Sergey. > > I fail to understand you, sorry. We're going in circles, it seems... :-/ platform_get_irq_optional() is supposed to return 0 when there is no IRQ found, but everything else went alright. I'm tired to waste my time to go circles. Again, the problem is that platform_get_irq_optional() has wrong set of output values. And your patch doesn't fix that. And it has nothing to do with my code here. > > The problem is that this is _optional API and it has been misdesigned. > > Replacing things like above will increase the mess. > > What's wrong with replacing IRQ0 with -ENXIO now? platform_get_irq_optional() > (as in your patch) could then happily return 0 ISO -ENXIO. Contrarywise, if we don't > replace IRQ0 with -ENXIO, platform_get_irq_optional() will return 0 for both IRQ0 > and missing IRQ! Am I clear enough? If you don't understand me now, I don't know what > to say... :-/ See above. Read my messages again, please. I'm really tired to explain again and again the same. TL;DR: You simply try to "fix" in a correct place but in a wrong way. > >> return ret; > >> > >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > > > No. This is not a business of the caller to workaround implementation > > details (bugs) of the core APIs. > > If something goes wrong, then it's platform_get_irq() to blame, and > > not the libahci_platform. > > I'm repeating myself already: we don't work around the bug in platform_get_irq(), Yes, you do. > we're working around the driver subsystems that treat 0 specially (and so don't > support IRQ0); libata treats 0 as an indication of the polling mode (moreover, > it will curse if you pass to it both IRQ == 0 and a pointer to an interrupt handler! > Am I clear enough this time? :-) Yes, and it doesn't contradict to what my patch does. Read comment against platform_get_irq(). If it returns 0, it's not a business of the callers to work around it. Am I clear enough this time? :-)
On Fri, Dec 10, 2021 at 08:15:43PM +0300, Sergei Shtylyov wrote: > On 12/10/21 2:36 PM, Andy Shevchenko wrote: > > >>>>>> platform_get_irq() will print a message when it fails. > >>>>>> No need to repeat this. > >>>>>> > >>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>> out a big WARN() in such case. > >>>>> > >>>>> The reason you should be able to remove the "if (!irq)" test is that > >>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc > >>>>> says. But looking at platform_get_irq_optional(), which is called by > >>>>> platform_get_irq(), the out label is: > >>>>> > >>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >>>>> return ret; > >>>>> > >>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>>>> return -ENXIO: > >>>>> > >>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>>>> return -ENXIO; > >>>>> return ret; > >>>> > >>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >>>> but returns -EINVAL instead. > >>>> > >>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > >>>> > >>>> Of course it isn't... > >>> > >>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > >>> those API calls. > >> > >> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() > >> is there... > > > > So, have you seen this warning (being reported) related to libahci_platform? > > No (as if you need to really see this while it's obvious from the code review). > > > If no, what we are discussing about then? The workaround is redundant and > > I don't know. :-) Your arguments so far seem bogus (sorry! :-))... It seems you haven't got them at all. The problems of platform_get_irq() et al shouldn't be worked around in the callers. > > no need to have a dead code in the driver, really. > > "Jazz isn't dead, it just smells funny". :-) > > >>> If it is the case, go and fix them, no need to workaround > >>> in each of the callers. > >> > >> There's a need to work around as long as IRQ0 ican be returned, otherwise > >> we get partly functioning or non-functioning drivers... > > > > You get them unfunctioning anyways > > The drivers would be broken in not quite obvious ways. With IRQ0 check, they just > don't probe anymore. See the explanation of the IRQ0 check (in the drivers) in my > previous mail... > > > and you get the big WARN() even before this patch. > > As if that was enough... > The IRQ0 problem exists for at least 15 (if not 20) years...
On 12/10/21 8:59 PM, Andy Shevchenko wrote: >>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>> No need to repeat this. >>>>>>>> >>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>> out a big WARN() in such case. >>>>>>> >>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>> platform_get_irq(), the out label is: >>>>>>> >>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>> return ret; >>>>>>> >>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>> return -ENXIO: >>>>>>> >>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>> return -ENXIO; >>>>>>> return ret; >>>>>> >>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>> but returns -EINVAL instead. >>>>>> >>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>> >>>>>> Of course it isn't... >>>>> >>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>> those API calls. >>>> >>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>> is there... >>> >>> So, have you seen this warning (being reported) related to libahci_platform? >> >> No (as if you need to really see this while it's obvious from the code review). >> >>> If no, what we are discussing about then? The workaround is redundant and >> >> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... > > It seems you haven't got them at all. The problems of platform_get_irq() et al > shouldn't be worked around in the callers. I have clearly explained to you what I'm working around there. If that wasn't clear enough, I don't want to continue this talk anymore. Good luck with your patch (not this one). [...] MBR, Sergey
On Fri, Dec 10, 2021 at 10:01:04PM +0300, Sergey Shtylyov wrote: > On 12/10/21 8:59 PM, Andy Shevchenko wrote: > > >>>>>>>> platform_get_irq() will print a message when it fails. > >>>>>>>> No need to repeat this. > >>>>>>>> > >>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>>>> out a big WARN() in such case. > >>>>>>> > >>>>>>> The reason you should be able to remove the "if (!irq)" test is that > >>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc > >>>>>>> says. But looking at platform_get_irq_optional(), which is called by > >>>>>>> platform_get_irq(), the out label is: > >>>>>>> > >>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >>>>>>> return ret; > >>>>>>> > >>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>>>>>> return -ENXIO: > >>>>>>> > >>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>>>>>> return -ENXIO; > >>>>>>> return ret; > >>>>>> > >>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >>>>>> but returns -EINVAL instead. > >>>>>> > >>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > >>>>>> > >>>>>> Of course it isn't... > >>>>> > >>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > >>>>> those API calls. > >>>> > >>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() > >>>> is there... > >>> > >>> So, have you seen this warning (being reported) related to libahci_platform? > >> > >> No (as if you need to really see this while it's obvious from the code review). > >> > >>> If no, what we are discussing about then? The workaround is redundant and > >> > >> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... > > > > It seems you haven't got them at all. The problems of platform_get_irq() et al > > shouldn't be worked around in the callers. > > I have clearly explained to you what I'm working around there. If that wasn't clear > enough, I don't want to continue this talk anymore. Good luck with your patch (not this > one). Good luck with yours, not the one that touches platform_get_irq_optional() though!
On 12/10/21 10:25 PM, Andy Shevchenko wrote: [...] >>>>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>>>> No need to repeat this. >>>>>>>>>> >>>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>>> out a big WARN() in such case. >>>>>>>>> >>>>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>>>> platform_get_irq(), the out label is: >>>>>>>>> >>>>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>>>> return ret; >>>>>>>>> >>>>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>>>> return -ENXIO: >>>>>>>>> >>>>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>>>> return -ENXIO; >>>>>>>>> return ret; >>>>>>>> >>>>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>>>> but returns -EINVAL instead. >>>>>>>> >>>>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>>>> >>>>>>>> Of course it isn't... >>>>>>> >>>>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>>>> those API calls. >>>>>> >>>>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>>>> is there... >>>>> >>>>> So, have you seen this warning (being reported) related to libahci_platform? >>>> >>>> No (as if you need to really see this while it's obvious from the code review). >>>> >>>>> If no, what we are discussing about then? The workaround is redundant and >>>> >>>> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... >>> >>> It seems you haven't got them at all. The problems of platform_get_irq() et al >>> shouldn't be worked around in the callers. >> >> I have clearly explained to you what I'm working around there. If that wasn't clear >> enough, I don't want to continue this talk anymore. Good luck with your patch (not this >> one). > > Good luck with yours, not the one that touches platform_get_irq_optional() though! Mmh, I'm not touching it any way that would break what your patch was trying to do, unless you've re-thopught that. It also shoudn't matter whose patch gets merged 1st other than some small adaptation). MBR, Sergey
On 12/10/21 10:30 PM, Sergey Shtylyov wrote: [...] >>>>>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>>>>> No need to repeat this. >>>>>>>>>>> >>>>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>>>> out a big WARN() in such case. >>>>>>>>>> >>>>>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>>>>> platform_get_irq(), the out label is: >>>>>>>>>> >>>>>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>>>>> return ret; >>>>>>>>>> >>>>>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>>>>> return -ENXIO: >>>>>>>>>> >>>>>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>>>>> return -ENXIO; >>>>>>>>>> return ret; >>>>>>>>> >>>>>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>>>>> but returns -EINVAL instead. >>>>>>>>> >>>>>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>>>>> >>>>>>>>> Of course it isn't... >>>>>>>> >>>>>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>>>>> those API calls. >>>>>>> >>>>>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>>>>> is there... >>>>>> >>>>>> So, have you seen this warning (being reported) related to libahci_platform? >>>>> >>>>> No (as if you need to really see this while it's obvious from the code review). >>>>> >>>>>> If no, what we are discussing about then? The workaround is redundant and >>>>> >>>>> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... >>>> >>>> It seems you haven't got them at all. The problems of platform_get_irq() et al >>>> shouldn't be worked around in the callers. >>> >>> I have clearly explained to you what I'm working around there. If that wasn't clear >>> enough, I don't want to continue this talk anymore. Good luck with your patch (not this >>> one). >> >> Good luck with yours, not the one that touches platform_get_irq_optional() though! > > Mmh, I'm not touching it any way that would break what your patch was trying to do, > unless you've re-thopught that. It also shoudn't matter whose patch gets merged 1st > other than some small adaptation). BTW, looking at [1], this comment is wrong: + * Return: non-zero IRQ number on success, negative error number on failure. It doesn't mention 0 which you return from this function. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ed7027fdf4ec41ed6df6814956dc11860232a9d5 MBR, Sergey
On 2021/12/10 17:59, Sergey Shtylyov wrote: > On 12/10/21 1:49 AM, Damien Le Moal wrote: > >>> platform_get_irq() will print a message when it fails. >>> No need to repeat this. >>> >>> While at it, drop redundant check for 0 as platform_get_irq() spills >>> out a big WARN() in such case. >> >> The reason you should be able to remove the "if (!irq)" test is that >> platform_get_irq() never returns 0. At least, that is what the function kdoc >> says. But looking at platform_get_irq_optional(), which is called by >> platform_get_irq(), the out label is: >> >> WARN(ret == 0, "0 is an invalid IRQ number\n"); >> return ret; >> >> So 0 will be returned as-is. That is rather weird. That should be fixed to >> return -ENXIO: >> >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >> return -ENXIO; >> return ret; > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > but returns -EINVAL instead. Thinking more about this, shouldn't this change go into platform_get_irq() instead of platform_get_irq_optional() ? The way I see it, I think that the intended behavior for platform_get_irq_optional() is: 1) If have IRQ, return it, always > 0 2) If no IRQ, return 0 3) If error, return < 0 no ? And for platform_get_irq(), case (2) becomes an error. Is this the intended semantic ? I am really not sure here as the functions kdoc description and the code do not match. Which one is correct ? > >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > Of course it isn't... > >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > [...] > > MBR, Sergey
On 10.12.2021 22:35, Sergei Shtylyov wrote: [...] >>>>>>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>>>>>> No need to repeat this. >>>>>>>>>>>> >>>>>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>>>>> out a big WARN() in such case. >>>>>>>>>>> >>>>>>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>>>>>> platform_get_irq(), the out label is: >>>>>>>>>>> >>>>>>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>>>>>> return ret; >>>>>>>>>>> >>>>>>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>>>>>> return -ENXIO: >>>>>>>>>>> >>>>>>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>>>>>> return -ENXIO; >>>>>>>>>>> return ret; >>>>>>>>>> >>>>>>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>>>>>> but returns -EINVAL instead. >>>>>>>>>> >>>>>>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>>>>>> >>>>>>>>>> Of course it isn't... >>>>>>>>> >>>>>>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>>>>>> those API calls. >>>>>>>> >>>>>>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>>>>>> is there... >>>>>>> >>>>>>> So, have you seen this warning (being reported) related to libahci_platform? >>>>>> >>>>>> No (as if you need to really see this while it's obvious from the code review). >>>>>> >>>>>>> If no, what we are discussing about then? The workaround is redundant and >>>>>> >>>>>> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... >>>>> >>>>> It seems you haven't got them at all. The problems of platform_get_irq() et al >>>>> shouldn't be worked around in the callers. >>>> >>>> I have clearly explained to you what I'm working around there. If that wasn't clear >>>> enough, I don't want to continue this talk anymore. Good luck with your patch (not this >>>> one). >>> >>> Good luck with yours, not the one that touches platform_get_irq_optional() though! >> >> Mmh, I'm not touching it any way that would break what your patch was trying to do, >> unless you've re-thopught that. It also shoudn't matter whose patch gets merged 1st >> other than some small adaptation). > > BTW, looking at [1], this comment is wrong: > > + * Return: non-zero IRQ number on success, negative error number on failure. > > It doesn't mention 0 which you return from this function. Also, your commit log is wrong in the description of how to handle the result: << Now: ret = platform_get_irq_optional(...); if (ret != -ENXIO) return ret; // respect deferred probe if (ret > 0) ...we get an IRQ... >> The (ret != -ENXIO) check also succeeds on the (positive) IRQ #s, so the following code becomes unreachable. :-/ > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ed7027fdf4ec41ed6df6814956dc11860232a9d5 MBR, Sergey
Hello! On 11.12.2021 2:45, Damien Le Moal wrote: [...] >>>> platform_get_irq() will print a message when it fails. >>>> No need to repeat this. >>>> >>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>> out a big WARN() in such case. >>> >>> The reason you should be able to remove the "if (!irq)" test is that >>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>> says. But looking at platform_get_irq_optional(), which is called by >>> platform_get_irq(), the out label is: >>> >>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>> return ret; >>> >>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>> return -ENXIO: >>> >>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>> return -ENXIO; >>> return ret; >> >> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >> but returns -EINVAL instead. > > Thinking more about this, shouldn't this change go into platform_get_irq() > instead of platform_get_irq_optional() ? Why? platform_get_irq() currently just calls platform_get_irq_optional()... > The way I see it, I think that the intended behavior for > platform_get_irq_optional() is: > 1) If have IRQ, return it, always > 0 > 2) If no IRQ, return 0 That does include the IRQ0 case, right? > 3) If error, return < 0 > no ? I completely agree, I (after thinking a bit) have no issues with that... > And for platform_get_irq(), case (2) becomes an error. > Is this the intended semantic ? I don't see how it's different from the current behavior. But we can do that as well, I just don't see whether it's really better... > I am really not sure here as the functions kdoc description and the code do not > match. Which one is correct ? It seems both are wrong. :-) [...] MBR, Sergey
On 2021/12/11 19:25, Sergey Shtylyov wrote: > Hello! > > On 11.12.2021 2:45, Damien Le Moal wrote: > > [...] >>>>> platform_get_irq() will print a message when it fails. >>>>> No need to repeat this. >>>>> >>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>> out a big WARN() in such case. >>>> >>>> The reason you should be able to remove the "if (!irq)" test is that >>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>> says. But looking at platform_get_irq_optional(), which is called by >>>> platform_get_irq(), the out label is: >>>> >>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>> return ret; >>>> >>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>> return -ENXIO: >>>> >>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>> return -ENXIO; >>>> return ret; >>> >>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>> but returns -EINVAL instead. >> >> Thinking more about this, shouldn't this change go into platform_get_irq() >> instead of platform_get_irq_optional() ? > > Why? platform_get_irq() currently just calls platform_get_irq_optional()... > >> The way I see it, I think that the intended behavior for >> platform_get_irq_optional() is: >> 1) If have IRQ, return it, always > 0 >> 2) If no IRQ, return 0 > > That does include the IRQ0 case, right? IRQ 0 being invalid, I think that case should be dealt with internally within platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus be case (3), an error. > >> 3) If error, return < 0 >> no ? > > I completely agree, I (after thinking a bit) have no issues with that... > >> And for platform_get_irq(), case (2) becomes an error. >> Is this the intended semantic ? > > I don't see how it's different from the current behavior. But we can do > that as well, I just don't see whether it's really better... The problem I see is that the current behavior is unclear: what does platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think it should be the latter rather than the former. Note that the function could return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, but then I do not see any difference between platform_get_irq_optional() and platform_get_irq(). If the preferred API semantic is to allow returning IRQ 0 with a warning, then the kdoc comments of platform_get_irq_optional() and platform_get_irq() are totally broken, and the code for many drivers is probably wrong too. > >> I am really not sure here as the functions kdoc description and the code do not >> match. Which one is correct ? > > It seems both are wrong. :-) > > [...] > > MBR, Sergey
On Sat, Dec 11, 2021 at 01:13:52PM +0300, Sergey Shtylyov wrote: > On 10.12.2021 22:35, Sergei Shtylyov wrote: ... > Also, your commit log is wrong in the description of how to handle the result: > > << > Now: > ret = platform_get_irq_optional(...); > if (ret != -ENXIO) > return ret; // respect deferred probe > if (ret > 0) > ...we get an IRQ... > >> > > The (ret != -ENXIO) check also succeeds on the (positive) IRQ #s, so the > following code becomes unreachable. :-/ Indeed, thanks! Should be if (ret < 0 && ret != -ENXIO)
On Sat, Dec 11, 2021 at 08:45:51AM +0900, Damien Le Moal wrote: > On 2021/12/10 17:59, Sergey Shtylyov wrote: > > On 12/10/21 1:49 AM, Damien Le Moal wrote: > > > >>> platform_get_irq() will print a message when it fails. > >>> No need to repeat this. > >>> > >>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>> out a big WARN() in such case. > >> > >> The reason you should be able to remove the "if (!irq)" test is that > >> platform_get_irq() never returns 0. At least, that is what the function kdoc > >> says. But looking at platform_get_irq_optional(), which is called by > >> platform_get_irq(), the out label is: > >> > >> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >> return ret; > >> > >> So 0 will be returned as-is. That is rather weird. That should be fixed to > >> return -ENXIO: > >> > >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >> return -ENXIO; > >> return ret; > > > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > > but returns -EINVAL instead. > > Thinking more about this, shouldn't this change go into platform_get_irq() > instead of platform_get_irq_optional() ? > > The way I see it, I think that the intended behavior for > platform_get_irq_optional() is: > 1) If have IRQ, return it, always > 0 > 2) If no IRQ, return 0 > 3) If error, return < 0 > no ? At least this is my understanding on how it _should_ be. > And for platform_get_irq(), case (2) becomes an error. Precisely! > Is this the intended semantic ? > I am really not sure here as the functions kdoc description and the code do not > match. Which one is correct ? The problem is that platform_get_irq_optional() doesn't follow above mentioned logic and needs to be fixed. While trying to fix that it appears that it's not an simple and 5 minutes task since it needs a revisiting of all callers first followed by rectifying the API itself. > >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > > > Of course it isn't...
On Sat, Dec 11, 2021 at 01:25:20PM +0300, Sergey Shtylyov wrote: > On 11.12.2021 2:45, Damien Le Moal wrote: ... > > > > > platform_get_irq() will print a message when it fails. > > > > > No need to repeat this. > > > > > > > > > > While at it, drop redundant check for 0 as platform_get_irq() spills > > > > > out a big WARN() in such case. > > > > > > > > The reason you should be able to remove the "if (!irq)" test is that > > > > platform_get_irq() never returns 0. At least, that is what the function kdoc > > > > says. But looking at platform_get_irq_optional(), which is called by > > > > platform_get_irq(), the out label is: > > > > > > > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > > > > return ret; > > > > > > > > So 0 will be returned as-is. That is rather weird. That should be fixed to > > > > return -ENXIO: > > > > > > > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > > > > return -ENXIO; > > > > return ret; > > > > > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > > > but returns -EINVAL instead. > > > > Thinking more about this, shouldn't this change go into platform_get_irq() > > instead of platform_get_irq_optional() ? > > Why? platform_get_irq() currently just calls platform_get_irq_optional()... > > > The way I see it, I think that the intended behavior for > > platform_get_irq_optional() is: > > 1) If have IRQ, return it, always > 0 > > 2) If no IRQ, return 0 > > That does include the IRQ0 case, right? What IRQ0 case? We do not expect platform APIs ever to handle vIRQ0 (mind letter v) case. Platforms, where it's the case, should handle it exceptionally on per architecture basis. > > 3) If error, return < 0 > > no ? > > I completely agree, I (after thinking a bit) have no issues with that... > > > And for platform_get_irq(), case (2) becomes an error. > > Is this the intended semantic ? > > I don't see how it's different from the current behavior. But we can do > that as well, I just don't see whether it's really better... > > > I am really not sure here as the functions kdoc description and the code do not > > match. Which one is correct ? > > It seems both are wrong. :-)
On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote: > On 2021/12/11 19:25, Sergey Shtylyov wrote: > > On 11.12.2021 2:45, Damien Le Moal wrote: ... > >>>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>>> return -ENXIO: > >>>> > >>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>>> return -ENXIO; > >>>> return ret; > >>> > >>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >>> but returns -EINVAL instead. > >> > >> Thinking more about this, shouldn't this change go into platform_get_irq() > >> instead of platform_get_irq_optional() ? > > > > Why? platform_get_irq() currently just calls platform_get_irq_optional()... > > > >> The way I see it, I think that the intended behavior for > >> platform_get_irq_optional() is: > >> 1) If have IRQ, return it, always > 0 > >> 2) If no IRQ, return 0 > > > > That does include the IRQ0 case, right? > > IRQ 0 being invalid, I think that case should be dealt with internally within > platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus > be case (3), an error. > > > > >> 3) If error, return < 0 > >> no ? > > > > I completely agree, I (after thinking a bit) have no issues with that... > > > >> And for platform_get_irq(), case (2) becomes an error. > >> Is this the intended semantic ? > > > > I don't see how it's different from the current behavior. But we can do > > that as well, I just don't see whether it's really better... > > The problem I see is that the current behavior is unclear: what does > platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think > it should be the latter rather than the former. Note that the function could > return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, > but then I do not see any difference between platform_get_irq_optional() and > platform_get_irq(). > > If the preferred API semantic is to allow returning IRQ 0 with a warning, then > the kdoc comments of platform_get_irq_optional() and platform_get_irq() are > totally broken, and the code for many drivers is probably wrong too. Yeah, what we need to do is that (roughly a roadmap): - revisit callers of platform_get_irq_optional() to be prepared for new behaviour - rewrite platform_get_irq() to return -ENOENT - rewrite platform_get_irq_optional() to return 0 on -ENOENT This is how other similar (i.e. _optional) APIs do.
On 2021/12/13 20:52, Andy Shevchenko wrote: > On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote: >> On 2021/12/11 19:25, Sergey Shtylyov wrote: >>> On 11.12.2021 2:45, Damien Le Moal wrote: > > ... > >>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>> return -ENXIO: >>>>>> >>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>> return -ENXIO; >>>>>> return ret; >>>>> >>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>> but returns -EINVAL instead. >>>> >>>> Thinking more about this, shouldn't this change go into platform_get_irq() >>>> instead of platform_get_irq_optional() ? >>> >>> Why? platform_get_irq() currently just calls platform_get_irq_optional()... >>> >>>> The way I see it, I think that the intended behavior for >>>> platform_get_irq_optional() is: >>>> 1) If have IRQ, return it, always > 0 >>>> 2) If no IRQ, return 0 >>> >>> That does include the IRQ0 case, right? >> >> IRQ 0 being invalid, I think that case should be dealt with internally within >> platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus >> be case (3), an error. >> >>> >>>> 3) If error, return < 0 >>>> no ? >>> >>> I completely agree, I (after thinking a bit) have no issues with that... >>> >>>> And for platform_get_irq(), case (2) becomes an error. >>>> Is this the intended semantic ? >>> >>> I don't see how it's different from the current behavior. But we can do >>> that as well, I just don't see whether it's really better... >> >> The problem I see is that the current behavior is unclear: what does >> platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think >> it should be the latter rather than the former. Note that the function could >> return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, >> but then I do not see any difference between platform_get_irq_optional() and >> platform_get_irq(). >> >> If the preferred API semantic is to allow returning IRQ 0 with a warning, then >> the kdoc comments of platform_get_irq_optional() and platform_get_irq() are >> totally broken, and the code for many drivers is probably wrong too. > > Yeah, what we need to do is that (roughly a roadmap): > - revisit callers of platform_get_irq_optional() to be prepared for > new behaviour > - rewrite platform_get_irq() to return -ENOENT > - rewrite platform_get_irq_optional() to return 0 on -ENOENT > > This is how other similar (i.e. _optional) APIs do. Sounds like a good plan to me. In the mean time though, your patch 1/2 should keep the "if (!irq)" test and return an error for that case. No ?
On Tue, Dec 14, 2021 at 06:36:00AM +0900, Damien Le Moal wrote: > On 2021/12/13 20:52, Andy Shevchenko wrote: > > On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote: > >> On 2021/12/11 19:25, Sergey Shtylyov wrote: ... > >> The problem I see is that the current behavior is unclear: what does > >> platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think > >> it should be the latter rather than the former. Note that the function could > >> return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, > >> but then I do not see any difference between platform_get_irq_optional() and > >> platform_get_irq(). > >> > >> If the preferred API semantic is to allow returning IRQ 0 with a warning, then > >> the kdoc comments of platform_get_irq_optional() and platform_get_irq() are > >> totally broken, and the code for many drivers is probably wrong too. > > > > Yeah, what we need to do is that (roughly a roadmap): > > - revisit callers of platform_get_irq_optional() to be prepared for > > new behaviour > > - rewrite platform_get_irq() to return -ENOENT > > - rewrite platform_get_irq_optional() to return 0 on -ENOENT > > > > This is how other similar (i.e. _optional) APIs do. > > Sounds like a good plan to me. In the mean time though, your patch 1/2 should > keep the "if (!irq)" test and return an error for that case. No ? No problem. I can send a v2.
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 0910441321f7..1af642c84e7b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, int i, irq, n_ports, rc; irq = platform_get_irq(pdev, 0); - if (irq < 0) { - if (irq != -EPROBE_DEFER) - dev_err(dev, "no irq\n"); + if (irq < 0) return irq; - } - if (!irq) - return -EINVAL; hpriv->irq = irq;
platform_get_irq() will print a message when it fails. No need to repeat this. While at it, drop redundant check for 0 as platform_get_irq() spills out a big WARN() in such case. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/ata/libahci_platform.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)