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 <[email protected]>
---
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;
--
2.33.0
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 <[email protected]>
Thanks, the series looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
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 <[email protected]>
> ---
> 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()?
--
With Best Regards,
Andy Shevchenko
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.
--
With Best Regards,
Andy Shevchenko
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/[email protected]/T/#u
?
--
With Best Regards,
Andy Shevchenko
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/[email protected]/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 <[email protected]>
> ---
> 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;
>
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
>> ---
>> 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;
>>
>
>
--
Damien Le Moal
Western Digital Research
On Fri, Dec 10, 2021 at 4:47 AM Damien Le Moal
<[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
[...]
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.
--
With Best Regards,
Andy Shevchenko
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.
--
With Best Regards,
Andy Shevchenko
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.
--
With Best Regards,
Andy Shevchenko
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.
--
With Best Regards,
Andy Shevchenko
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?
--
With Best Regards,
Andy Shevchenko
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? :-)
--
With Best Regards,
Andy Shevchenko
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...
--
With Best Regards,
Andy Shevchenko
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!
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> [...]
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
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
--
Damien Le Moal
Western Digital Research
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)
--
With Best Regards,
Andy Shevchenko
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...
--
With Best Regards,
Andy Shevchenko
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. :-)
--
With Best Regards,
Andy Shevchenko
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.
--
With Best Regards,
Andy Shevchenko
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 ?
--
Damien Le Moal
Western Digital Research
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.
--
With Best Regards,
Andy Shevchenko